Accounts Refactor PR 2: Adding address_override param to tx#2771
Conversation
1201dc0 to
5008b08
Compare
920da01 to
736575b
Compare
a887f49 to
30c8c38
Compare
theodorebugnet
left a comment
There was a problem hiding this comment.
LGTM, but is there a reason we avoid adding it to V0 transactions? Note that the theodore/multisig-upgrade branch is going to be a hard fork and require a new rollup binary regardless, with a coordinated upgrade height using the rollup manager. The previous changes on this branch already break the V0 format (UnsignedTransaction::V0 {...} is not byte-compatible with the old UnsignedTransaction, by design).
Adding it to both cases would IMO simplify things conceptually: this way, the differences between V0 and V1 are the signing protocol, while the actual transaction features remain the same.
3898e2e to
3704b1d
Compare
f94b261 to
ce7b152
Compare
theodorebugnet
left a comment
There was a problem hiding this comment.
LGTM, almost all comments are doc nits except the redundant argument to to_multisig_tx() in unsigned/v0.rs which I think needs fixing
Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
…on/payload.rs Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
…on/payload.rs Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
…v0.rs Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
…v1.rs Co-authored-by: Theodore Bugnet <24320578+theodorebugnet@users.noreply.github.com>
Thanks @theodorebugnet , keeping the bar. Many things slipped through while repurposing PR to support of the target_address for V0 tx too. |
address_override param to tx
6406ec4
into
theodore/multisig-upgrade
Description
Adds a signed
address_override:Option<S::Address>field to V0 and V1 (multisig) transactions, threaded intoAuthorizationData.address_override.Resolve_context branches on it: Some(X) verifies (X, credential_id) ∈ account_owners via PR #2770 's is_authorized;
None keeps today's resolve_sender_address flow.
CHAIN HASH BREAKING for V1.
First commit: 7d2ab42
PR #2770 froze writes to the legacy credential_id → address map;
InsertCredentialId now only marks (sender, credential) authorized, without rerouting that credential's default.
That means a rotated multisig (new keys → new credential_id.into()) has no way to keep acting as the original address.
address_overrideis the signed channel that lets a signer declare which authorized address to execute as.Design tradeoffs
CHAIN_HASH_OVERRIDEScovers signature-level grace only (post-deserialize). Adding a borsh field breaks deserialization of pre-upgrade V0/V1 bytes regardless of chain hash, so a grace period isn't meaningful.to_multisig_txandsign_utx_v1_in_placetake the target as an argument rather than defaulting to None with callers assigning the public field afterwards — avoids a half-baked API in an SDK recommended pattern.Audit invariants
Some(_)path does exactly one read of account_owners; never writes.Some(_) → TxEffect::Skipped, sequencer penalized, user never charged.Notable side-effects
test_multisig_signature_verificationin the EIP-712 integration test was already silently broken by PR Accounts Refactor PR 1: Adding account -> [credential_id] mapping #2770 (the multisig credential no longer routed to admin afterInsertCredentialId, leaving gas on an unfunded default). Fixed here by signing with address_override = Some(admin.address()) — doubles as an EIP-712-meets-target-address smoke test.I have updatedCHANGELOG.mdwith a new entry if my PR makes any breaking changes or fixes a bug. If my PR removes a feature or changes its behavior, I provide help for users on how to migrate to the new behavior.Cargo.tomlchanges before opening the PRs. (Are all new dependencies necessary? Is any module dependency leaked into the full-node (hint: it shouldn't)?)Linked Issues
sov-accountsrefactoring and extending #2772Testing
New tests are added
Docs
Updated doc-strings and comments