feat(runtime): add Op variant for claim bridge transfers#1727
Conversation
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5065a28f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Deploy { address: Vec<u8> }, | ||
| Maintain { address: Vec<u8> }, | ||
| ClaimRewards { value: u128 }, | ||
| ClaimRewards { value: u128, kind: ClaimKind }, |
There was a problem hiding this comment.
When get_decoded_transaction returns a ClaimRewards operation, this added SCALE field changes the encoded Tx ABI, but pallets/midnight/src/runtime_api.rs still advertises MidnightRuntimeApi as version 5. Clients compiled against the old v5 layout, or node code querying pre-upgrade blocks through the same API version, will decode ClaimRewards { value } and then either fail or misread the following Tx fields; add a new runtime API version/changed_in compatibility path or preserve the old v5 result type for this method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
AI has a good point. These ops aren't versioned like the ledger versions, which are separated by crate so they can get each relevant midnight-ledger version. Perhaps its worth versioning similarly?
There was a problem hiding this comment.
I changed it. This time I only add new Op variant. Since this is not persisted we don't need migration.
Op is only passed from Runtime to Node, so Node that is updated with this code prior to Runtime, will know how to decode it.
LGLO
force-pushed
the
issue-1084-add-claim-kind-to-events
branch
from
f5065a2 to
458a1e1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 458a1e12e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,11 @@ | |||
| #runtime | |||
There was a problem hiding this comment.
Technically also a sensitive node/client side change
LGLO
force-pushed
the
issue-1084-add-claim-kind-to-events
branch
from
7e36d9a to
f564907
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f56490714c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // New variants MUST be appended at the end: `Op` is SCALE-encoded across the | ||
| // `get_decoded_transaction` runtime-API boundary, so the variant indices are wire-significant. | ||
| // Appending keeps existing indices stable (an older decoder simply never sees the new index). | ||
| ClaimBridgeTransfer { value: u128 }, |
There was a problem hiding this comment.
The fresh evidence in this revision is that pallets/midnight/src/runtime_api.rs still advertises MidnightRuntimeApi as version 5 while this new Op index can now be returned by get_decoded_transaction. Appending keeps new nodes able to decode old runtimes, but the reverse direction still breaks: an old node running with --filter-deploy-txs will call the upgraded runtime for a CardanoBridge claim, fail to SCALE-decode the unknown enum index, and node/src/filtering_pool.rs drops the otherwise valid transaction on that runtime API decode error. Please version this runtime API/result type or otherwise keep the v5 return path from producing the new variant for old callers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
an old node running with --filter-deploy-txs will call the upgraded runtime for a CardanoBridge claim
We update nodes before runtime, so there is no case of old node talking to the new runtime.
LGLO
changed the title
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
LGLO
force-pushed
the
issue-1084-add-claim-kind-to-events
branch
from
185d6ad to
0d47ad6
Compare
2 participants
Overview
Ad
Opvariant forClaimBridgeTransferThe added
Op::ClaimBridgeTransfervariant.Opis passed in only from Runtime to Node,so updating Node before the Runtime makes it a safe change. Decode side is updated before Encode side.
The new variant is added as last to
Op, so existing variants SCALE encoding doesn't change.Claim of bridge transferred amount haven't yet happened in any durable environment yet, so future
consumers of
ClaimRewardsandClaimBridgeTransfercan be sure what caused these events.This is not true for lower testnets where old runtime will encode both kinds of claims as
ClaimRewards.TransactionAppliedStateRootis not changed, it has only one field for amounts of both kinds of claims.Op/Operationtypes, and the static runtime metadata is regenerated accordingly.Issue: #1084
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links