Accounts Refactor PR 3: Addinng more call messages to accounts module#2773
Accounts Refactor PR 3: Addinng more call messages to accounts module#2773citizen-stig wants to merge 9 commits intonikolai/accounts-refactor-part-2from
Conversation
citizen-stig
force-pushed
the
nikolai/accounts-refactor-part-2
branch
from
920da01 to
736575b
Compare
citizen-stig
force-pushed
the
nikolai/accounts-refactor-part-3
branch
from
a7156ff to
b48875f
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .unwrap_or(false)) | ||
| } | ||
|
|
||
| /// Returns `true` if `credential_id` is authorized to act as `address`. |
There was a problem hiding this comment.
🟡 Stale doc comment on is_authorized_for describes reversed precedence order
The doc comment on is_authorized_for at crates/module-system/module-implementations/sov-accounts/src/capabilities.rs:65-71 still describes the old precedence: "if credential_id has a legacy mapping in accounts, that mapping is authoritative". However, the implementation was reordered to check account_owners first (line 78-83), then accounts (line 85-87), then canonical fallback (line 89). Critically, an account_owners entry of false now short-circuits and denies authorization, overriding both the legacy map and canonical fallback. The doc also doesn't mention the new false-as-denial semantics at all.
This violates the AGENTS.md rule: "Keep docs/comments in sync with behavior." A developer reading this doc would incorrectly believe the legacy mapping is always authoritative, which matters for reasoning about the security-sensitive revocation feature.
(Refers to lines 65-71)
Was this helpful? React with 👍 or 👎 to provide feedback.
citizen-stig
force-pushed
the
nikolai/accounts-refactor-part-3
branch
from
3c9bfcf to
22aac29
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
citizen-stig
force-pushed
the
nikolai/accounts-refactor-part-3
branch
from
22aac29 to
bbe926a
Compare
| fn ensure_credential_not_authorized( | ||
| &self, | ||
| address: &S::Address, | ||
| credential: &CredentialId, | ||
| state: &mut impl StateReader<User>, | ||
| ) -> anyhow::Result<()> { | ||
| anyhow::ensure!( | ||
| self.account_owners | ||
| .get(&AccountOwnerKey::new(*address, *new_credential_id), state) | ||
| !self | ||
| .account_owners | ||
| .get(&AccountOwnerKey::new(*address, *credential), state) | ||
| .context("Failed to read account owner")? | ||
| .is_none(), | ||
| .unwrap_or(false), | ||
| "CredentialId already authorized for this address" | ||
| ); | ||
| Ok(()) |
There was a problem hiding this comment.
(Refers to lines 205-220)
🚩 ensure_credential_not_authorized only checks account_owners, not full is_authorized_for
The ensure_credential_not_authorized function at crates/module-system/module-implementations/sov-accounts/src/call.rs:205-220 only checks account_owners state, not the full authorization chain (legacy map + canonical address). This means AddCredentialToAddress and RotateCredentialOnAddress (for the new credential) will succeed even if the credential is already implicitly authorized via canonical address or legacy mapping. The result is a redundant explicit true in account_owners. This is benign for add (already authorized → still authorized), but creates an important side-effect: if the user later calls RemoveCredentialFromAddress, the written false will override the canonical fallback, effectively revoking implicit authorization. This asymmetry is documented in the README ("revoking the last credential leaves the address unspendable") and seems intentional, but could surprise users who add their own canonical credential and later remove it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Ok(self | ||
| .account_owners | ||
| .get(&AccountOwnerKey::new(*address, *credential_id), state)? | ||
| .is_some()) | ||
| .unwrap_or(false)) |
There was a problem hiding this comment.
📝 Info: Semantic change: is_authorized now returns false for Some(false) entries
The change from .is_some() to .unwrap_or(false) at crates/module-system/module-implementations/sov-accounts/src/capabilities.rs:62 is a meaningful behavioral change. Previously .is_some() would return true for ANY value in account_owners (including a hypothetical false). Now .unwrap_or(false) correctly distinguishes Some(true) from Some(false). This is essential for the revoke feature, since revoke_credential writes false to account_owners. The resolve_sender Some(requested) path at crates/module-system/sov-capabilities/src/lib.rs:73 relies on this to reject revoked credentials targeting a specific address. In the old code this was safe because account_owners only ever stored true, but now both values are possible.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| Ok(requested) | ||
| } | ||
| None => Ok(self.accounts.resolve_sender_address( | ||
| &auth_data.default_address, | ||
| &auth_data.credential_id, | ||
| state, | ||
| )?), | ||
| None => { | ||
| let resolved = self.accounts.resolve_sender_address( | ||
| &auth_data.default_address, | ||
| &auth_data.credential_id, | ||
| state, | ||
| )?; | ||
| if !self | ||
| .accounts | ||
| .is_authorized_for(&resolved, &auth_data.credential_id, state)? | ||
| { | ||
| anyhow::bail!( | ||
| "credential {} not authorized for resolved address {}", | ||
| auth_data.credential_id, | ||
| resolved, | ||
| ); | ||
| } | ||
| Ok(resolved) | ||
| } | ||
| } |
There was a problem hiding this comment.
(Refers to lines 69-101)
📝 Info: Asymmetry between Some and None paths in resolve_sender
The Some(requested) path at crates/module-system/sov-capabilities/src/lib.rs:70-81 uses is_authorized (checks only account_owners), while the None path at lines 83-101 uses is_authorized_for (checks account_owners → legacy → canonical). This asymmetry is intentional: target_address = Some(X) requires an explicit account_owners authorization (the user is specifically requesting a non-default address), while target_address = None uses the full fallback chain for the resolver's default. This was the behavior before this PR for the Some path; only the None path gained a new check.
Was this helpful? React with 👍 or 👎 to provide feedback.
| None => { | ||
| let resolved = self.accounts.resolve_sender_address( | ||
| &auth_data.default_address, | ||
| &auth_data.credential_id, | ||
| state, | ||
| )?; | ||
| if !self | ||
| .accounts | ||
| .is_authorized_for(&resolved, &auth_data.credential_id, state)? | ||
| { | ||
| anyhow::bail!( | ||
| "credential {} not authorized for resolved address {}", | ||
| auth_data.credential_id, | ||
| resolved, | ||
| ); | ||
| } | ||
| Ok(resolved) | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: New authorization check on resolve_sender None path is a consensus-breaking change
The added is_authorized_for check at lines 89-98 means V0 transactions (no target_address) can now be rejected if the credential has been explicitly revoked via account_owners[(resolved, credential)] = false. Previously this path unconditionally succeeded. For existing rollups, this is safe as long as no false entries exist in account_owners before upgrade (they can't, since revoke_credential is new). After upgrade, the check only triggers for credentials that have been explicitly revoked via the new RemoveCredentialFromAddress or RotateCredentialOnAddress calls. The CHANGELOG correctly flags this as a consensus-breaking change.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
New trait discussion: Context PR #2773 made is_authorized_for a mandatory gate on the V0 tx pre-exec Pre-fix, the generic blanket Before PR 3, this mismatch was latent — is_authorized_for wasn't on any What HEAD already has Commit 71e96cf ("Attempt to fix one:") at the current branch HEAD implements
Status verified locally:
The red CI is stale — it ran on bbe926a (before the fix). HEAD The three realistic options Option 1 — Keep current (TryDecodeCredentialId trait) The fix at HEAD. Already committed, compiles, tracing confirms correct. Pros
Cons
Option 2 — Inline check in sov-address + use HyperlaneAddress::from_sender in demo-stf Drop the new trait. Inline a ~3-line bytes[..12] == [0u8; 12] check directly Pros
Cons
Option 3 — Replace From for EthereumAddress with TryFrom Delete the unconditional impl From for EthereumAddress at Pros
Cons
Recommendation Option 1 — keep the current HEAD 71e96cf and push it to re-run CI.
Optional small polish inside Option 1: tighten the demo-stf From |
1 participant
Description
sov_accounts::CallMessagebecomes generic in S: Spec and gains three variants:AddCredentialToAddress { address, credential }— authorize a new credential on an owned address.RemoveCredentialFromAddress { address, credential }— revoke.RotateCredentialOnAddress { address, old_credential, new_credential }— atomic swap (functionally Remove + Add, but one tx, one signing round for a multisig).InsertCredentialIdsemantics are unchanged. All three new variants share one rule: context.sender() == message.address.Why
#2770 added the account_owners map but only wrote to it at genesis and through InsertCredentialId. PR 2 made V1 signers declare a target_address.
Without this PR, a multisig that rotates its keys can't keep its address — there's no way to add a new authorized credential or revoke the old one. Rotate is the primitive that makes M1 → M2 a single operator-friendly tx instead of two.
Tradeoffs / design calls worth reviewing
Ownership check simplified to context.sender() == address instead of the plan's "caller's credential authorizes for address" lookup. Context doesn't expose sender_credential_id(), and PR 2's resolver already proves the caller controls sender — so the simpler rule is equally safe and subsumes the "bootstrap" case (fresh V0 user → sender is their default → matches).
Revocation writes account_owners[(addr, cred)] = false, not a delete. false is a durable revocation that blocks the stateless canonical fallback (credential.into() == address). Without it, removing a credential from its own default address would be silently re-authorized by the fallback. The storage model flips from "present-means-authorized" to "explicit true/false override".
No legacy check on Add. A credential with a pre-upgrade accounts[c] = { addr } entry can be added to a different new-model address. V0 routing still uses legacy; V1+target=Some uses the new map. This avoids making PRs 3/4 useless on upgraded chains before PR 5's DrainLegacyAccounts runs.
No orphan guard on Remove/Rotate. Revoking the last credential is allowed; caller is responsible. Apps that want a guard add it at their own layer.
No events, no mirror maps. Deferred to PR 5 (along with enumeration queries).
Wire / consensus
--
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
Testing
New tests added
Docs
Docstrings are updated