From c1b3d6c0573f74b40b4e6cbf1988f5ec48d36555 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 21 Apr 2026 16:28:31 +0200 Subject: [PATCH 01/21] Adding account -> [credential_id] mapping --- .../sov-accounts/src/call.rs | 37 +-- .../sov-accounts/src/capabilities.rs | 67 ++-- .../sov-accounts/src/genesis.rs | 12 +- .../sov-accounts/src/lib.rs | 64 +++- .../sov-accounts/tests/integration/main.rs | 300 ++++++++++++------ .../module-schemas/schemas/sov-accounts.json | 2 +- 6 files changed, 343 insertions(+), 139 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 6b23a9ece5..21c1cf3a30 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -1,20 +1,22 @@ +use anyhow::anyhow; use anyhow::bail; -use anyhow::{anyhow, Result}; use schemars::JsonSchema; use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; use sov_state::namespaces::User; -use crate::{Account, Accounts}; +use crate::{AccountOwnerKey, Accounts}; /// Represents the available call messages for interacting with the sov-accounts module. #[derive(Debug, PartialEq, Eq, Clone, JsonSchema, UniversalWallet)] #[serialize(Borsh, Serde)] #[serde(rename_all = "snake_case")] pub enum CallMessage { - /// Inserts a new credential id for the corresponding Account. + /// Authorizes a credential to sign transactions that execute as the + /// caller's address. Does not affect the credential's stateless default + /// routing (`credential_id.into()`). InsertCredentialId( - /// The new credential id. + /// The credential id being authorized. CredentialId, ), } @@ -25,35 +27,30 @@ impl Accounts { new_credential_id: CredentialId, context: &Context, state: &mut impl TxState, - ) -> Result<()> { + ) -> anyhow::Result<()> { if !self.enable_custom_account_mappings.get(state)?.expect( "`enable_custom_account_mappings` should not be None; it must be set at genesis.", ) { bail!("Custom account mappings are disabled"); } - self.exit_if_credential_exists(&new_credential_id, state)?; - - // Insert the new credential id -> account mapping - let account = Account { - addr: *context.sender(), - }; - self.accounts.set(&new_credential_id, &account, state)?; - + let key = AccountOwnerKey::new(*context.sender(), new_credential_id); + self.exit_if_authorization_exists(&key, state)?; + self.account_owners.set(&key, &true, state)?; Ok(()) } - fn exit_if_credential_exists( + fn exit_if_authorization_exists( &self, - new_credential_id: &CredentialId, + key: &AccountOwnerKey, state: &mut impl StateReader, - ) -> Result<()> { + ) -> anyhow::Result<()> { anyhow::ensure!( - self.accounts - .get(new_credential_id, state) - .map_err(|err| anyhow!("Error raised while getting account: {err:?}"))? + self.account_owners + .get(key, state) + .map_err(|err| anyhow!("Error raised while getting account owner: {err:?}"))? .is_none(), - "New CredentialId already exists" + "CredentialId already authorized for this address" ); Ok(()) } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 8d84fc04d4..9dc18fde94 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -1,45 +1,70 @@ use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, StateWriter}; use sov_state::User; -use crate::{Account, Accounts}; +use crate::{AccountOwnerKey, Accounts}; impl Accounts { /// Resolve the sender's public key to an address. - /// If the sender is not registered, but a fallback address if provided, immediately registers - /// the credential to the fallback and then returns it. + /// + /// Resolution order: + /// 1. If `(default_address, credential_id)` is present in `account_owners`, return `default_address`. + /// 2. Else if the legacy `accounts` map has an entry for `credential_id`, return that address + /// (pre-upgrade fallback; never write-forwarded). + /// 3. Else auto-register the tuple in `account_owners` and return `default_address`. pub fn resolve_sender_address( &mut self, default_address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result>::Error> { - let maybe_address = self.accounts.get(credential_id, state)?.map(|a| a.addr); - - match maybe_address { - Some(address) => Ok(address), - None => { - // 1. Add the credential -> account mapping - let new_account = Account { - addr: *default_address, - }; - self.accounts.set(credential_id, &new_account, state)?; - - Ok(*default_address) - } + let default_key = AccountOwnerKey::new(*default_address, *credential_id); + if self.account_owners.get(&default_key, state)?.is_some() { + return Ok(*default_address); + } + if let Some(legacy) = self.accounts.get(credential_id, state)? { + return Ok(legacy.addr); } + self.account_owners.set(&default_key, &true, state)?; + Ok(*default_address) } - /// Resolve the sender's public key to an address. + /// Resolve the sender's public key to an address (read-only). Mirrors + /// [`Self::resolve_sender_address`] steps 1 and 2; step 3 falls through to + /// returning `default_address` without writing. pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result { - let maybe_address = self.accounts.get(credential_id, state)?.map(|a| a.addr); - match maybe_address { - Some(address) => Ok(address), - None => Ok(*default_address), + if self + .account_owners + .get( + &AccountOwnerKey::new(*default_address, *credential_id), + state, + )? + .is_some() + { + return Ok(*default_address); + } + if let Some(legacy) = self.accounts.get(credential_id, state)? { + return Ok(legacy.addr); } + Ok(*default_address) + } + + /// Returns `true` if `credential_id` is authorized to sign transactions + /// that execute as `address`. Read-only lookup against + /// [`Self::account_owners`]. + pub fn is_authorized>( + &self, + address: &S::Address, + credential_id: &CredentialId, + state: &mut ST, + ) -> Result { + Ok(self + .account_owners + .get(&AccountOwnerKey::new(*address, *credential_id), state)? + .is_some()) } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index 5bf6eefaec..4e69fb0fec 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -4,7 +4,7 @@ use serde_with::{serde_as, DisplayFromStr}; use sov_modules_api::prelude::*; use sov_modules_api::{CredentialId, GenesisState}; -use crate::{Account, Accounts}; +use crate::{AccountOwnerKey, Accounts}; /// Account data for the genesis. #[serde_as] @@ -25,7 +25,7 @@ pub struct AccountData
{ pub struct AccountConfig { /// Accounts to initialize the rollup. pub accounts: Vec>, - /// Enable custom `CredentailId` => `Account` mapping. + /// Enable custom `CredentialId` => `Account` mapping. #[serde(default = "default_true")] pub enable_custom_account_mappings: bool, } @@ -51,13 +51,11 @@ impl Accounts { } for acc in &config.accounts { - if self.accounts.get(&acc.credential_id, state)?.is_some() { + let key = AccountOwnerKey::new(acc.address, acc.credential_id); + if self.account_owners.get(&key, state)?.is_some() { bail!("Account already exists") } - - let new_account = Account { addr: acc.address }; - - self.accounts.set(&acc.credential_id, &new_account, state)?; + self.account_owners.set(&key, &true, state)?; } Ok(()) diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 6bfe8bbda0..03a1dcc4a7 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -34,6 +34,48 @@ pub struct Account { pub addr: S::Address, } +/// Composite key used by [`Accounts::account_owners`]. A present entry +/// `(address, credential_id)` means `credential_id` is authorized to spend as +/// `address`. Many credentials may authorize one address, and one credential may +/// be authorized for many addresses. +#[derive(borsh::BorshDeserialize, borsh::BorshSerialize, Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct AccountOwnerKey { + pub(crate) address: S::Address, + pub(crate) credential_id: CredentialId, +} + +impl AccountOwnerKey { + pub(crate) fn new(address: S::Address, credential_id: CredentialId) -> Self { + Self { + address, + credential_id, + } + } +} + +impl std::fmt::Display for AccountOwnerKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}/{}", self.address, self.credential_id) + } +} + +impl std::str::FromStr for AccountOwnerKey { + type Err = anyhow::Error; + fn from_str(s: &str) -> Result { + let (addr_str, cred_str) = s + .split_once('/') + .ok_or_else(|| anyhow::anyhow!("invalid AccountOwnerKey: missing '/' separator"))?; + Ok(Self { + address: addr_str + .parse() + .map_err(|e| anyhow::anyhow!("invalid address in AccountOwnerKey: {e:?}"))?, + credential_id: cred_str + .parse() + .map_err(|e| anyhow::anyhow!("invalid credential_id in AccountOwnerKey: {e}"))?, + }) + } +} + /// A module responsible for managing accounts on the rollup. #[derive(Clone, ModuleInfo, ModuleRestApi)] #[cfg_attr(feature = "arbitrary", derive(Debug))] @@ -42,13 +84,33 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// Mapping from a credential to its corresponding account. + /// Legacy `credential_id -> address` routing override. **Read-only post-PR-1: + /// no code path in this module writes to this map.** Genesis, auto-register, + /// and `InsertCredentialId` now write exclusively to [`Self::account_owners`]. + /// This field is retained to read pre-upgrade on-chain entries that were + /// written before the freeze; `resolve_sender_address` falls back to it when + /// the credential has no matching entry in `account_owners`. A follow-up PR + /// will drain these entries into `account_owners` and drop this field. #[state] pub(crate) accounts: StateMap>, /// If this field is false, `CallMessage::InsertCredentialId` messages will be rejected. #[state] enable_custom_account_mappings: StateValue, + + /// Many-to-many authorization relation. A present entry + /// `(address, credential_id)` means `credential_id` is authorized to sign + /// transactions that execute as `address`. Unlike [`Self::accounts`], the + /// same credential can own multiple addresses here. Authoritative source of + /// authorization state for all post-PR-1 writes; a future transaction + /// envelope (with a signed target address) will consult this map to decide + /// whether the credential is allowed on the target's behalf. + /// + /// The value is a `bool` sentinel rather than `()` because the state + /// backend does not preserve zero-byte values across commits — a `()` + /// value survives in-session but the key is dropped on persist. + #[state] + pub(crate) account_owners: StateMap, bool>, } impl Module for Accounts { diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index b4b0d42138..8f722bd8ec 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -1,4 +1,4 @@ -use sov_accounts::{Accounts, CallMessage, Response}; +use sov_accounts::{Accounts, CallMessage}; use sov_modules_api::transaction::{UnsignedTransactionV0, Version1}; use sov_modules_api::{ CryptoSpec, PrivateKey, PublicKey, RawTx, Runtime, SkippedTxContents, Spec, TxEffect, @@ -73,15 +73,15 @@ fn test_config_account() { runner, ) = setup(); - // The account is registered at genesis. + // The account is registered at genesis: `(user.address(), user.credential_id())` + // must appear in `account_owners`. runner.query_visible_state(|state| { let accounts = Accounts::::default(); - let response = accounts.get_account(user.credential_id(), state); - assert_eq!( - response, - Response::AccountExists { - addr: user.address() - } + assert!( + accounts + .is_authorized(&user.address(), &user.credential_id(), state) + .unwrap(), + "genesis-registered credential should be authorized for its address" ); }); } @@ -107,19 +107,20 @@ fn test_update_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. - assert_eq!( - accounts.get_account(new_credential, state), - Response::AccountExists { - addr: user.address() - } + // The new credential is authorized to spend as the sender's address. + assert!( + accounts + .is_authorized(&user.address(), &new_credential, state) + .unwrap(), + "InsertCredentialId must authorize the new credential for the sender" ); - // Account corresponding to the old credential still exists. - assert_eq!( - accounts.get_account(user.credential_id(), state), - Response::AccountExists { - addr: user.address() - } + // The sender's own credential is still authorized (auto-registered on + // first use when the tx was processed). + assert!( + accounts + .is_authorized(&user.address(), &user.credential_id(), state) + .unwrap(), + "the sender's own credential stays authorized" ); assert_ne!(new_credential, user.credential_id()); @@ -127,45 +128,75 @@ fn test_update_account() { }); } +/// In the new many-to-many model, authorizing another user's credential for +/// one's own address is a no-op (the attacker still needs the other party's +/// private key to sign). The duplicate guard only checks the exact +/// `(sender, credential)` tuple, so inserting a peer's credential succeeds. +/// A second `InsertCredentialId` with the same tuple must still revert. #[test] -fn test_update_account_fails() { +fn test_insert_peer_credential_succeeds_but_duplicate_fails() { let ( TestData { account_1, - account_2, + non_registered_account: sender, .. }, mut runner, ) = setup(); + let peer_credential = account_1.credential_id(); + let sender_address = sender.address(); + + // First insert: authorizing a peer's credential for the sender's own + // address is permitted. + runner.execute_transaction(TransactionTestCase { + input: sender.create_plain_message::>(CallMessage::InsertCredentialId( + peer_credential, + )), + assert: Box::new(move |result, state| { + assert!( + result.tx_receipt.is_successful(), + "authorizing a peer's credential for your own address should succeed" + ); + let accounts = Accounts::::default(); + assert!( + accounts + .is_authorized(&sender_address, &peer_credential, state) + .unwrap(), + "peer credential should now be authorized for the sender's address" + ); + }), + }); + + // Second insert with the same tuple reverts via the duplicate guard. runner.execute_transaction(TransactionTestCase { - input: account_1.create_plain_message::>(CallMessage::InsertCredentialId( - account_2.credential_id(), + input: sender.create_plain_message::>(CallMessage::InsertCredentialId( + peer_credential, )), - assert: Box::new(move |result, _state| { - if let TxEffect::Reverted(contents) = result.tx_receipt { + assert: Box::new(move |result, _state| match result.tx_receipt { + TxEffect::Reverted(contents) => { assert_eq!( contents.reason.to_string(), - "New CredentialId already exists" + "CredentialId already authorized for this address" ); } + _ => panic!("Expected reverted transaction on duplicate authorization"), }), }); } /// Tests the multisig functionality of the Accounts module. +/// +/// The multisig's effective signing address is its stateless default +/// (`multisig_credential_id.into()`). We seed genesis with a `TestUser` whose +/// custom `credential_id` matches the multisig, so the default address has a +/// gas balance and is already authorized in `account_owners` before any tx is +/// submitted. This keeps the focus on signature-level invariants. #[test] fn test_setup_multisig_and_act() { use sov_modules_api::Multisig; - let ( - TestData { - non_registered_account: user, - .. - }, - mut runner, - ) = setup(); - // First, create and register a multisig + // Generate the multisig first so we know its credential_id at genesis time. let multisig_keys = [ TestPrivateKey::generate(), TestPrivateKey::generate(), @@ -174,33 +205,15 @@ fn test_setup_multisig_and_act() { let multisig = Multisig::new(2, multisig_keys.iter().map(|k| k.pub_key()).collect()); let multisig_credential_id = multisig.credential_id::<<::CryptoSpec as CryptoSpec>::Hasher>(); - runner.execute_transaction(TransactionTestCase { - input: user.create_plain_message::>(CallMessage::InsertCredentialId( - multisig_credential_id, - )), - assert: Box::new(move |result, state| { - assert!(result.tx_receipt.is_successful()); - - let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. - assert_eq!( - accounts.get_account(multisig_credential_id, state), - Response::AccountExists { - addr: user.address() - } - ); - // Account corresponding to the old credential still exists. - assert_eq!( - accounts.get_account(user.credential_id(), state), - Response::AccountExists { - addr: user.address() - } - ); + // Build a funded `TestUser` whose address is the multisig's default. + let multisig_user = + TestUser::generate_with_default_balance().add_credential_id(multisig_credential_id); - assert_ne!(multisig_credential_id, user.credential_id()); - }), - }); + let genesis_config = + HighLevelOptimisticGenesisConfig::generate().add_accounts(vec![multisig_user.clone()]); + let genesis = GenesisConfig::from_minimal_config(genesis_config.into()); + let mut runner = TestRunner::new_with_genesis(genesis.into_genesis_params(), RT::default()); // Define utilities for... // - Generating a valid multisig (version 1) transaction @@ -388,8 +401,16 @@ fn test_register_new_account() { runner.query_visible_state(|state| { let accounts = Accounts::::default(); - let response = accounts.get_account(non_registered_account.credential_id(), state); - assert_eq!(response, Response::AccountEmpty); + assert!( + !accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap(), + "unregistered account should have no authorization at genesis" + ); }); let new_credential = TestPrivateKey::generate().pub_key().credential_id(); @@ -403,20 +424,25 @@ fn test_register_new_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. - assert_eq!( - accounts.get_account(new_credential, state), - Response::AccountExists { - addr: non_registered_account.address() - } + // The new credential is authorized for the sender's address. + assert!( + accounts + .is_authorized(&non_registered_account.address(), &new_credential, state) + .unwrap(), + "new credential should be authorized for the sender's address" ); - // The default credential of the account exists - assert_eq!( - accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountExists { - addr: non_registered_account.address() - } + // The sender's own credential is auto-registered for the same + // address when the tx flowed through `resolve_sender_address`. + assert!( + accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap(), + "the sender's own credential is auto-registered on first tx" ); assert_ne!(new_credential, non_registered_account.credential_id()); @@ -449,6 +475,11 @@ fn test_resolve_sender_address_with_default_address_non_registered() { }); } +/// Genesis-registered credentials are authorized for their configured address +/// in `account_owners`. The resolver returns that address when queried with it +/// as the default; passing a different fallback auto-registers a new tuple for +/// the fallback (since the many-to-many model allows a credential to own +/// multiple addresses). #[test] fn test_resolve_sender_address_registered() { let ( @@ -463,19 +494,34 @@ fn test_resolve_sender_address_registered() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); - // Ensure correct (registered) address is used even if another fallback is provided + // Resolving with account_1's registered address as the default returns + // it directly (step 1 in the resolver). assert_eq!( accounts - .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) + .resolve_sender_address(&account_1.address(), &account_1.credential_id(), state) .unwrap(), account_1.address() ); + + // Resolving with a different default for the same credential returns + // that different default (auto-registered); the many-to-many model + // allows the credential to own both addresses. + assert_eq!( + accounts + .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) + .unwrap(), + account_2.address() + ); }); } -/// Tests what happens if one tries to resolve an address when there is more than one credential available. +/// After `InsertCredentialId` from a user, the new credential is authorized +/// for that user's address but its stateless default routing +/// (`credential_id.into()`) is unchanged. Resolving with the user's address as +/// default returns it; resolving with the credential's own default returns the +/// default (auto-registered). #[test] -fn test_resolve_address_if_more_than_one_credential() { +fn test_resolve_address_with_multi_credential_ownership() { let ( TestData { non_registered_account, @@ -484,19 +530,13 @@ fn test_resolve_address_if_more_than_one_credential() { mut runner, ) = setup(); - let pub_key_1 = TestPrivateKey::generate().pub_key(); - let credential_1 = pub_key_1.credential_id(); - let default_address_1 = credential_1.into(); - - let pub_key_2 = TestPrivateKey::generate().pub_key(); - let credential_2 = pub_key_2.credential_id(); - let default_address_2 = credential_2.into(); + let credential_1 = TestPrivateKey::generate().pub_key().credential_id(); + let credential_2 = TestPrivateKey::generate().pub_key().credential_id(); runner.execute( non_registered_account .create_plain_message::>(CallMessage::InsertCredentialId(credential_1)), ); - runner.execute( non_registered_account .create_plain_message::>(CallMessage::InsertCredentialId(credential_2)), @@ -505,19 +545,28 @@ fn test_resolve_address_if_more_than_one_credential() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); + // With the user's address as default, both credentials route to it + // (they were authorized via `InsertCredentialId`). assert_eq!( accounts - .resolve_sender_address(&default_address_1, &credential_1, state) + .resolve_sender_address(&non_registered_account.address(), &credential_1, state) .unwrap(), non_registered_account.address() ); - assert_eq!( accounts - .resolve_sender_address(&default_address_2, &credential_2, state) + .resolve_sender_address(&non_registered_account.address(), &credential_2, state) .unwrap(), non_registered_account.address() ); + + // Both credentials are authorized under the user's address. + assert!(accounts + .is_authorized(&non_registered_account.address(), &credential_1, state) + .unwrap()); + assert!(accounts + .is_authorized(&non_registered_account.address(), &credential_2, state) + .unwrap()); }); } @@ -543,6 +592,79 @@ fn test_resolve_with_different_default_address() { }); } +/// Verifies the PR 1 invariant: every write to the routing `accounts` map must +/// also write the matching `(address, credential_id)` tuple into +/// `account_owners`. This covers the three write paths (genesis, +/// `InsertCredentialId`, and auto-register on first resolve). +/// Verifies the PR 1 invariant: every write path (genesis, auto-register on +/// first resolve, and `InsertCredentialId`) lands its tuple in +/// `account_owners`. The legacy `accounts` map is never written post-freeze, +/// so these paths are the only sources of post-upgrade authorization state. +#[test] +fn test_account_owners_write_paths() { + let ( + TestData { + account_1, + non_registered_account, + .. + }, + mut runner, + ) = setup(); + + // Genesis path. + runner.query_visible_state(|state| { + let accounts = Accounts::::default(); + assert!( + accounts + .is_authorized(&account_1.address(), &account_1.credential_id(), state) + .unwrap(), + "genesis-registered credential should be present in account_owners" + ); + }); + + // Auto-register path: a `resolve_sender_address` call for an unseen + // credential writes the tuple. `query_visible_state` scopes the write to + // this closure so the check is in-session. + runner.query_visible_state(|state| { + let mut accounts = Accounts::::default(); + let _ = accounts + .resolve_sender_address( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state, + ) + .unwrap(); + assert!( + accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap(), + "auto-registered credential should be present in account_owners" + ); + }); + + // InsertCredentialId path. + let new_credential = TestPrivateKey::generate().pub_key().credential_id(); + runner.execute_transaction(TransactionTestCase { + input: non_registered_account.create_plain_message::>( + CallMessage::InsertCredentialId(new_credential), + ), + assert: Box::new(move |result, state| { + assert!(result.tx_receipt.is_successful()); + let accounts = Accounts::::default(); + assert!( + accounts + .is_authorized(&non_registered_account.address(), &new_credential, state) + .unwrap(), + "InsertCredentialId should populate account_owners under sender's address" + ); + }), + }); +} + #[test] fn test_disable_custom_account_mappings() { let (user, mut runner) = setup_with_disable_custom_account_mappings(); diff --git a/crates/module-system/module-schemas/schemas/sov-accounts.json b/crates/module-system/module-schemas/schemas/sov-accounts.json index f2405b3954..a54e8f1e8a 100644 --- a/crates/module-system/module-schemas/schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/schemas/sov-accounts.json @@ -4,7 +4,7 @@ "description": "Represents the available call messages for interacting with the sov-accounts module.", "oneOf": [ { - "description": "Inserts a new credential id for the corresponding Account.", + "description": "Authorizes a credential to sign transactions that execute as the caller's address. Does not affect the credential's stateless default routing (`credential_id.into()`).", "type": "object", "required": [ "insert_credential_id" From f22eb567db4821c75fbe4db6969718530bc06ee9 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 21 Apr 2026 19:55:03 +0200 Subject: [PATCH 02/21] Review: part 1 --- .../sov-accounts/src/call.rs | 37 +++-- .../sov-accounts/src/capabilities.rs | 61 ++++---- .../sov-accounts/src/genesis.rs | 10 +- .../sov-accounts/src/lib.rs | 40 +++-- .../sov-accounts/tests/integration/main.rs | 140 ++++++++++-------- .../genesis-schemas/sov-accounts.json | 2 +- .../module-schemas/schemas/sov-accounts.json | 2 +- 7 files changed, 161 insertions(+), 131 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 21c1cf3a30..f00fd8d226 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -1,22 +1,21 @@ -use anyhow::anyhow; -use anyhow::bail; +use anyhow::{anyhow, bail, Result}; use schemars::JsonSchema; use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; use sov_state::namespaces::User; -use crate::{AccountOwnerKey, Accounts}; +use crate::{Account, AccountOwnerKey, Accounts}; /// Represents the available call messages for interacting with the sov-accounts module. #[derive(Debug, PartialEq, Eq, Clone, JsonSchema, UniversalWallet)] #[serialize(Borsh, Serde)] #[serde(rename_all = "snake_case")] pub enum CallMessage { - /// Authorizes a credential to sign transactions that execute as the - /// caller's address. Does not affect the credential's stateless default - /// routing (`credential_id.into()`). + /// Registers `credential_id` as an authorized signer whose transactions + /// execute as the caller's address. Fails if the credential is already + /// mapped to any address in this module (credentials are globally unique). InsertCredentialId( - /// The credential id being authorized. + /// The credential id being registered. CredentialId, ), } @@ -27,30 +26,36 @@ impl Accounts { new_credential_id: CredentialId, context: &Context, state: &mut impl TxState, - ) -> anyhow::Result<()> { + ) -> Result<()> { if !self.enable_custom_account_mappings.get(state)?.expect( "`enable_custom_account_mappings` should not be None; it must be set at genesis.", ) { bail!("Custom account mappings are disabled"); } + self.exit_if_credential_exists(&new_credential_id, state)?; + + let account = Account { + addr: *context.sender(), + }; + self.accounts.set(&new_credential_id, &account, state)?; + let key = AccountOwnerKey::new(*context.sender(), new_credential_id); - self.exit_if_authorization_exists(&key, state)?; self.account_owners.set(&key, &true, state)?; Ok(()) } - fn exit_if_authorization_exists( + fn exit_if_credential_exists( &self, - key: &AccountOwnerKey, + new_credential_id: &CredentialId, state: &mut impl StateReader, - ) -> anyhow::Result<()> { + ) -> Result<()> { anyhow::ensure!( - self.account_owners - .get(key, state) - .map_err(|err| anyhow!("Error raised while getting account owner: {err:?}"))? + self.accounts + .get(new_credential_id, state) + .map_err(|err| anyhow!("Error raised while getting account: {err:?}"))? .is_none(), - "CredentialId already authorized for this address" + "New CredentialId already exists" ); Ok(()) } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 9dc18fde94..ed61362482 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -1,70 +1,71 @@ use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, StateWriter}; use sov_state::User; -use crate::{AccountOwnerKey, Accounts}; +use crate::{Account, AccountOwnerKey, Accounts}; impl Accounts { /// Resolve the sender's public key to an address. /// /// Resolution order: - /// 1. If `(default_address, credential_id)` is present in `account_owners`, return `default_address`. - /// 2. Else if the legacy `accounts` map has an entry for `credential_id`, return that address - /// (pre-upgrade fallback; never write-forwarded). - /// 3. Else auto-register the tuple in `account_owners` and return `default_address`. + /// 1. If `accounts` has an entry for `credential_id`, return that address. + /// 2. Else auto-register `credential_id` to `default_address` in both + /// `accounts` and `account_owners`, then return `default_address`. pub fn resolve_sender_address( &mut self, default_address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result>::Error> { - let default_key = AccountOwnerKey::new(*default_address, *credential_id); - if self.account_owners.get(&default_key, state)?.is_some() { - return Ok(*default_address); - } - if let Some(legacy) = self.accounts.get(credential_id, state)? { - return Ok(legacy.addr); + if let Some(account) = self.accounts.get(credential_id, state)? { + return Ok(account.addr); } + + let default_key = AccountOwnerKey::new(*default_address, *credential_id); + self.accounts.set( + credential_id, + &Account { + addr: *default_address, + }, + state, + )?; self.account_owners.set(&default_key, &true, state)?; Ok(*default_address) } - /// Resolve the sender's public key to an address (read-only). Mirrors - /// [`Self::resolve_sender_address`] steps 1 and 2; step 3 falls through to - /// returning `default_address` without writing. + /// Resolve the sender's public key to an address (read-only). Mirrors the + /// lookup in [`Self::resolve_sender_address`]; auto-registration falls + /// through to returning `default_address` without writing. pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result { - if self - .account_owners - .get( - &AccountOwnerKey::new(*default_address, *credential_id), - state, - )? - .is_some() - { - return Ok(*default_address); - } - if let Some(legacy) = self.accounts.get(credential_id, state)? { - return Ok(legacy.addr); + if let Some(account) = self.accounts.get(credential_id, state)? { + return Ok(account.addr); } Ok(*default_address) } /// Returns `true` if `credential_id` is authorized to sign transactions - /// that execute as `address`. Read-only lookup against - /// [`Self::account_owners`]. + /// that execute as `address`. pub fn is_authorized>( &self, address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result { - Ok(self + if self .account_owners .get(&AccountOwnerKey::new(*address, *credential_id), state)? - .is_some()) + .is_some() + { + return Ok(true); + } + + Ok(matches!( + self.accounts.get(credential_id, state)?, + Some(Account { addr }) if addr == *address + )) } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index 4e69fb0fec..e75e1e415f 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -4,7 +4,7 @@ use serde_with::{serde_as, DisplayFromStr}; use sov_modules_api::prelude::*; use sov_modules_api::{CredentialId, GenesisState}; -use crate::{AccountOwnerKey, Accounts}; +use crate::{Account, AccountOwnerKey, Accounts}; /// Account data for the genesis. #[serde_as] @@ -51,10 +51,14 @@ impl Accounts { } for acc in &config.accounts { - let key = AccountOwnerKey::new(acc.address, acc.credential_id); - if self.account_owners.get(&key, state)?.is_some() { + if self.accounts.get(&acc.credential_id, state)?.is_some() { bail!("Account already exists") } + + let new_account = Account { addr: acc.address }; + self.accounts.set(&acc.credential_id, &new_account, state)?; + + let key = AccountOwnerKey::new(acc.address, acc.credential_id); self.account_owners.set(&key, &true, state)?; } diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 03a1dcc4a7..fee221639a 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -36,9 +36,12 @@ pub struct Account { /// Composite key used by [`Accounts::account_owners`]. A present entry /// `(address, credential_id)` means `credential_id` is authorized to spend as -/// `address`. Many credentials may authorize one address, and one credential may -/// be authorized for many addresses. -#[derive(borsh::BorshDeserialize, borsh::BorshSerialize, Debug, Clone, PartialEq, Eq, Hash)] +/// `address`. The key shape supports many-to-many authorization, while the +/// current compatibility index in [`Accounts::accounts`] preserves one primary +/// address per credential for existing resolution and query consumers. +#[derive( + borsh::BorshDeserialize, borsh::BorshSerialize, Debug, Clone, Copy, PartialEq, Eq, Hash, +)] pub(crate) struct AccountOwnerKey { pub(crate) address: S::Address, pub(crate) credential_id: CredentialId, @@ -53,6 +56,9 @@ impl AccountOwnerKey { } } +// `Display` and `FromStr` are required by `StateMap`'s struct-level trait bound +// (see `sov-modules-api/src/containers/map.rs`). The encoding is only used for +// REST API paths and debug output; on-chain keys are Borsh-serialized. impl std::fmt::Display for AccountOwnerKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}/{}", self.address, self.credential_id) @@ -71,7 +77,7 @@ impl std::str::FromStr for AccountOwnerKey { .map_err(|e| anyhow::anyhow!("invalid address in AccountOwnerKey: {e:?}"))?, credential_id: cred_str .parse() - .map_err(|e| anyhow::anyhow!("invalid credential_id in AccountOwnerKey: {e}"))?, + .map_err(|e| anyhow::anyhow!("invalid credential_id in AccountOwnerKey: {e:?}"))?, }) } } @@ -84,13 +90,9 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// Legacy `credential_id -> address` routing override. **Read-only post-PR-1: - /// no code path in this module writes to this map.** Genesis, auto-register, - /// and `InsertCredentialId` now write exclusively to [`Self::account_owners`]. - /// This field is retained to read pre-upgrade on-chain entries that were - /// written before the freeze; `resolve_sender_address` falls back to it when - /// the credential has no matching entry in `account_owners`. A follow-up PR - /// will drain these entries into `account_owners` and drop this field. + /// `credential_id -> address` routing index. This map is maintained with + /// [`Self::account_owners`] so existing consumers can resolve and query the + /// primary address for a credential without scanning the authorization map. #[state] pub(crate) accounts: StateMap>, @@ -98,17 +100,11 @@ pub struct Accounts { #[state] enable_custom_account_mappings: StateValue, - /// Many-to-many authorization relation. A present entry - /// `(address, credential_id)` means `credential_id` is authorized to sign - /// transactions that execute as `address`. Unlike [`Self::accounts`], the - /// same credential can own multiple addresses here. Authoritative source of - /// authorization state for all post-PR-1 writes; a future transaction - /// envelope (with a signed target address) will consult this map to decide - /// whether the credential is allowed on the target's behalf. - /// - /// The value is a `bool` sentinel rather than `()` because the state - /// backend does not preserve zero-byte values across commits — a `()` - /// value survives in-session but the key is dropped on persist. + /// Authorization relation. A present entry `(address, credential_id)` means + /// `credential_id` is authorized to sign transactions that execute as + /// `address`. Current write paths also maintain [`Self::accounts`] as the + /// primary address index for compatibility with existing resolution and + /// query APIs. #[state] pub(crate) account_owners: StateMap, bool>, } diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 8f722bd8ec..56642b1b80 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -1,4 +1,4 @@ -use sov_accounts::{Accounts, CallMessage}; +use sov_accounts::{Accounts, CallMessage, Response}; use sov_modules_api::transaction::{UnsignedTransactionV0, Version1}; use sov_modules_api::{ CryptoSpec, PrivateKey, PublicKey, RawTx, Runtime, SkippedTxContents, Spec, TxEffect, @@ -74,7 +74,8 @@ fn test_config_account() { ) = setup(); // The account is registered at genesis: `(user.address(), user.credential_id())` - // must appear in `account_owners`. + // must appear in `account_owners`, and `get_account` must see the + // credential's primary address. runner.query_visible_state(|state| { let accounts = Accounts::::default(); assert!( @@ -83,6 +84,12 @@ fn test_config_account() { .unwrap(), "genesis-registered credential should be authorized for its address" ); + assert_eq!( + accounts.get_account(user.credential_id(), state), + Response::AccountExists { + addr: user.address() + } + ); }); } @@ -114,6 +121,12 @@ fn test_update_account() { .unwrap(), "InsertCredentialId must authorize the new credential for the sender" ); + assert_eq!( + accounts.get_account(new_credential, state), + Response::AccountExists { + addr: user.address() + } + ); // The sender's own credential is still authorized (auto-registered on // first use when the tx was processed). assert!( @@ -122,19 +135,22 @@ fn test_update_account() { .unwrap(), "the sender's own credential stays authorized" ); + assert_eq!( + accounts.get_account(user.credential_id(), state), + Response::AccountExists { + addr: user.address() + } + ); assert_ne!(new_credential, user.credential_id()); }), }); } -/// In the new many-to-many model, authorizing another user's credential for -/// one's own address is a no-op (the attacker still needs the other party's -/// private key to sign). The duplicate guard only checks the exact -/// `(sender, credential)` tuple, so inserting a peer's credential succeeds. -/// A second `InsertCredentialId` with the same tuple must still revert. +/// A credential already mapped to one address cannot be inserted for another +/// address. Existing consumers rely on this global credential uniqueness check. #[test] -fn test_insert_peer_credential_succeeds_but_duplicate_fails() { +fn test_insert_existing_credential_fails() { let ( TestData { account_1, @@ -145,30 +161,7 @@ fn test_insert_peer_credential_succeeds_but_duplicate_fails() { ) = setup(); let peer_credential = account_1.credential_id(); - let sender_address = sender.address(); - - // First insert: authorizing a peer's credential for the sender's own - // address is permitted. - runner.execute_transaction(TransactionTestCase { - input: sender.create_plain_message::>(CallMessage::InsertCredentialId( - peer_credential, - )), - assert: Box::new(move |result, state| { - assert!( - result.tx_receipt.is_successful(), - "authorizing a peer's credential for your own address should succeed" - ); - let accounts = Accounts::::default(); - assert!( - accounts - .is_authorized(&sender_address, &peer_credential, state) - .unwrap(), - "peer credential should now be authorized for the sender's address" - ); - }), - }); - // Second insert with the same tuple reverts via the duplicate guard. runner.execute_transaction(TransactionTestCase { input: sender.create_plain_message::>(CallMessage::InsertCredentialId( peer_credential, @@ -177,10 +170,10 @@ fn test_insert_peer_credential_succeeds_but_duplicate_fails() { TxEffect::Reverted(contents) => { assert_eq!( contents.reason.to_string(), - "CredentialId already authorized for this address" + "New CredentialId already exists" ); } - _ => panic!("Expected reverted transaction on duplicate authorization"), + _ => panic!("Expected reverted transaction for existing credential"), }), }); } @@ -190,8 +183,8 @@ fn test_insert_peer_credential_succeeds_but_duplicate_fails() { /// The multisig's effective signing address is its stateless default /// (`multisig_credential_id.into()`). We seed genesis with a `TestUser` whose /// custom `credential_id` matches the multisig, so the default address has a -/// gas balance and is already authorized in `account_owners` before any tx is -/// submitted. This keeps the focus on signature-level invariants. +/// gas balance and is already mapped before any tx is submitted. This keeps the +/// focus on signature-level invariants. #[test] fn test_setup_multisig_and_act() { use sov_modules_api::Multisig; @@ -401,6 +394,10 @@ fn test_register_new_account() { runner.query_visible_state(|state| { let accounts = Accounts::::default(); + assert_eq!( + accounts.get_account(non_registered_account.credential_id(), state), + Response::AccountEmpty + ); assert!( !accounts .is_authorized( @@ -431,6 +428,12 @@ fn test_register_new_account() { .unwrap(), "new credential should be authorized for the sender's address" ); + assert_eq!( + accounts.get_account(new_credential, state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); // The sender's own credential is auto-registered for the same // address when the tx flowed through `resolve_sender_address`. @@ -444,6 +447,12 @@ fn test_register_new_account() { .unwrap(), "the sender's own credential is auto-registered on first tx" ); + assert_eq!( + accounts.get_account(non_registered_account.credential_id(), state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); assert_ne!(new_credential, non_registered_account.credential_id()); }), @@ -475,11 +484,8 @@ fn test_resolve_sender_address_with_default_address_non_registered() { }); } -/// Genesis-registered credentials are authorized for their configured address -/// in `account_owners`. The resolver returns that address when queried with it -/// as the default; passing a different fallback auto-registers a new tuple for -/// the fallback (since the many-to-many model allows a credential to own -/// multiple addresses). +/// Genesis-registered credentials resolve to their configured address even when +/// a different fallback address is supplied. #[test] fn test_resolve_sender_address_registered() { let ( @@ -503,23 +509,17 @@ fn test_resolve_sender_address_registered() { account_1.address() ); - // Resolving with a different default for the same credential returns - // that different default (auto-registered); the many-to-many model - // allows the credential to own both addresses. assert_eq!( accounts .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) .unwrap(), - account_2.address() + account_1.address() ); }); } /// After `InsertCredentialId` from a user, the new credential is authorized -/// for that user's address but its stateless default routing -/// (`credential_id.into()`) is unchanged. Resolving with the user's address as -/// default returns it; resolving with the credential's own default returns the -/// default (auto-registered). +/// for that user's address and resolves to that user's address. #[test] fn test_resolve_address_with_multi_credential_ownership() { let ( @@ -567,11 +567,23 @@ fn test_resolve_address_with_multi_credential_ownership() { assert!(accounts .is_authorized(&non_registered_account.address(), &credential_2, state) .unwrap()); + assert_eq!( + accounts.get_account(credential_1, state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); + assert_eq!( + accounts.get_account(credential_2, state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); }); } /// This test should verify that when a new credential is specified with an existing account's -/// address as fallback, that the credential is appended to that address. However +/// address as fallback, the credential is registered to that address. However /// query_visible_state doesn't mutate the state so it simply verifies that the fallback address is /// returned correctly #[test] @@ -592,16 +604,10 @@ fn test_resolve_with_different_default_address() { }); } -/// Verifies the PR 1 invariant: every write to the routing `accounts` map must -/// also write the matching `(address, credential_id)` tuple into -/// `account_owners`. This covers the three write paths (genesis, -/// `InsertCredentialId`, and auto-register on first resolve). -/// Verifies the PR 1 invariant: every write path (genesis, auto-register on -/// first resolve, and `InsertCredentialId`) lands its tuple in -/// `account_owners`. The legacy `accounts` map is never written post-freeze, -/// so these paths are the only sources of post-upgrade authorization state. +/// Verifies every write path (genesis, auto-register on first resolve, and +/// `InsertCredentialId`) updates both the `accounts` index and `account_owners`. #[test] -fn test_account_owners_write_paths() { +fn test_account_write_paths() { let ( TestData { account_1, @@ -620,6 +626,12 @@ fn test_account_owners_write_paths() { .unwrap(), "genesis-registered credential should be present in account_owners" ); + assert_eq!( + accounts.get_account(account_1.credential_id(), state), + Response::AccountExists { + addr: account_1.address() + } + ); }); // Auto-register path: a `resolve_sender_address` call for an unseen @@ -644,6 +656,12 @@ fn test_account_owners_write_paths() { .unwrap(), "auto-registered credential should be present in account_owners" ); + assert_eq!( + accounts.get_account(non_registered_account.credential_id(), state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); }); // InsertCredentialId path. @@ -661,6 +679,12 @@ fn test_account_owners_write_paths() { .unwrap(), "InsertCredentialId should populate account_owners under sender's address" ); + assert_eq!( + accounts.get_account(new_credential, state), + Response::AccountExists { + addr: non_registered_account.address() + } + ); }), }); } diff --git a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json index 5b4c04b15a..867858f505 100644 --- a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json @@ -15,7 +15,7 @@ } }, "enable_custom_account_mappings": { - "description": "Enable custom `CredentailId` => `Account` mapping.", + "description": "Enable custom `CredentialId` => `Account` mapping.", "default": true, "type": "boolean" } diff --git a/crates/module-system/module-schemas/schemas/sov-accounts.json b/crates/module-system/module-schemas/schemas/sov-accounts.json index a54e8f1e8a..b67f579a91 100644 --- a/crates/module-system/module-schemas/schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/schemas/sov-accounts.json @@ -4,7 +4,7 @@ "description": "Represents the available call messages for interacting with the sov-accounts module.", "oneOf": [ { - "description": "Authorizes a credential to sign transactions that execute as the caller's address. Does not affect the credential's stateless default routing (`credential_id.into()`).", + "description": "Registers `credential_id` as an authorized signer whose transactions execute as the caller's address. Fails if the credential is already mapped to any address in this module (credentials are globally unique).", "type": "object", "required": [ "insert_credential_id" From ef0701def1e33cc0d866e7d8b8ee07bf3f4c23a3 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 21 Apr 2026 21:54:16 +0200 Subject: [PATCH 03/21] Updating docs and more simplification --- .../sov-accounts/README.md | 70 +++++- .../sov-accounts/src/call.rs | 10 +- .../sov-accounts/src/capabilities.rs | 54 ++--- .../sov-accounts/src/genesis.rs | 9 +- .../sov-accounts/src/lib.rs | 33 ++- .../sov-accounts/tests/integration/main.rs | 213 ++++-------------- 6 files changed, 151 insertions(+), 238 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index 08b73c9c37..bcad5e3e51 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -1,15 +1,71 @@ # `sov-accounts` module -The `sov-accounts` module is responsible for managing accounts on the rollup. +The `sov-accounts` module resolves transaction credentials to rollup +addresses and records which credentials may act for which addresses. +### The `sov-accounts` module offers the following functionality -### The `sov-accounts` module offers the following functionality: +1. A credential has a deterministic canonical address, computed as + `credential_id.into::()`. This relation is stateless: using the + canonical address does not require an account entry to be written. -1. When a sender sends their first message, the `sov-accounts` module will create a new address by deriving it from the sender's credential. - The module will then add a mapping between the credential id and the address to its state. For all subsequent messages that include the sender's credential, - the module will retrieve the sender's address from the mapping and pass it along with the original message to an intended module. +1. A credential can be explicitly mapped to a primary address in module state. + This is used for custom account mappings, duplicate credential checks, + credential-only resolution, and the `get_account` query. -1. It is possible to add new credential to a given address using the `CallMessage::InsertCredentialId(..)` message. +1. It is possible to register another credential for the caller's address using + the `CallMessage::InsertCredentialId(..)` message. The call fails if that + credential is already explicitly mapped to any address. -1. It is possible to query the `sov-accounts` module using the `get_account` method and get the account corresponding to the given credential id. +1. It is possible to query the `sov-accounts` module using the `get_account` + method and get the explicitly mapped account corresponding to the given + credential id. +## Credential and Address Relations + +The module has three credential/address relations. They answer different +questions and should not be treated as interchangeable. + +### Stateless canonical address + +```text +credential_id -> credential_id.into::() +``` + +This is the default address for a credential. It is deterministic and requires +no state write. If a credential has no explicit state mapping, this canonical +address is the natural fallback for credential-only routing. + +### Credential-to-account map + +```text +accounts[credential_id] = Account { addr } +``` + +This state map records the primary address explicitly associated with a +credential. A credential has at most one primary address in this map, while one +address may be the primary address for many credentials. + +This map is used when callers only know a `CredentialId` and need an address: +`resolve_sender_address`, `get_account`, duplicate credential checks, and +legacy/custom account mappings all depend on this credential-indexed lookup. +`get_account` reports entries from this explicit map; it does not mean that +every possible stateless canonical address has a stored account entry. + +### Account-credential authorization map + +```text +account_owners[(address, credential_id)] = true +``` + +This state map records authorization. A present entry means the credential is +authorized to sign transactions that execute as the given address. + +This relation answers "may this credential act as this address?" once the target +address is known. It is not a replacement for the credential-to-account map, +because it is keyed by `(address, credential_id)` and cannot efficiently answer +"which address is this credential registered to?" with a single point lookup. + +In short: routing from only a credential uses the explicit +credential-to-account map or the stateless canonical fallback. Authorization for +a known address uses `account_owners`. diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index f00fd8d226..2bb306d3b5 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -4,7 +4,7 @@ use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; use sov_state::namespaces::User; -use crate::{Account, AccountOwnerKey, Accounts}; +use crate::Accounts; /// Represents the available call messages for interacting with the sov-accounts module. #[derive(Debug, PartialEq, Eq, Clone, JsonSchema, UniversalWallet)] @@ -35,13 +35,7 @@ impl Accounts { self.exit_if_credential_exists(&new_credential_id, state)?; - let account = Account { - addr: *context.sender(), - }; - self.accounts.set(&new_credential_id, &account, state)?; - - let key = AccountOwnerKey::new(*context.sender(), new_credential_id); - self.account_owners.set(&key, &true, state)?; + self.register_credential(context.sender(), &new_credential_id, state)?; Ok(()) } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index ed61362482..df526b858e 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -4,12 +4,26 @@ use sov_state::User; use crate::{Account, AccountOwnerKey, Accounts}; impl Accounts { - /// Resolve the sender's public key to an address. - /// - /// Resolution order: - /// 1. If `accounts` has an entry for `credential_id`, return that address. - /// 2. Else auto-register `credential_id` to `default_address` in both - /// `accounts` and `account_owners`, then return `default_address`. + /// Writes both indices that together assert `credential_id` is authorized + /// to sign as `address`. Callers must maintain this pairing so that + /// [`Self::is_authorized`] can rely on `account_owners` alone. + pub(crate) fn register_credential( + &mut self, + address: &S::Address, + credential_id: &CredentialId, + state: &mut ST, + ) -> Result<(), >::Error> { + self.accounts + .set(credential_id, &Account { addr: *address }, state)?; + self.account_owners.set( + &AccountOwnerKey::new(*address, *credential_id), + &true, + state, + ) + } + + /// Resolve the sender's public key to an address. If `credential_id` is + /// unknown, register it to `default_address` and return that. pub fn resolve_sender_address( &mut self, default_address: &S::Address, @@ -19,22 +33,12 @@ impl Accounts { if let Some(account) = self.accounts.get(credential_id, state)? { return Ok(account.addr); } - - let default_key = AccountOwnerKey::new(*default_address, *credential_id); - self.accounts.set( - credential_id, - &Account { - addr: *default_address, - }, - state, - )?; - self.account_owners.set(&default_key, &true, state)?; + self.register_credential(default_address, credential_id, state)?; Ok(*default_address) } - /// Resolve the sender's public key to an address (read-only). Mirrors the - /// lookup in [`Self::resolve_sender_address`]; auto-registration falls - /// through to returning `default_address` without writing. + /// Read-only variant of [`Self::resolve_sender_address`]: returns + /// `default_address` when the credential is unknown, without writing. pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, @@ -55,17 +59,9 @@ impl Accounts { credential_id: &CredentialId, state: &mut ST, ) -> Result { - if self + Ok(self .account_owners .get(&AccountOwnerKey::new(*address, *credential_id), state)? - .is_some() - { - return Ok(true); - } - - Ok(matches!( - self.accounts.get(credential_id, state)?, - Some(Account { addr }) if addr == *address - )) + .is_some()) } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index e75e1e415f..f03efecd2d 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -4,7 +4,7 @@ use serde_with::{serde_as, DisplayFromStr}; use sov_modules_api::prelude::*; use sov_modules_api::{CredentialId, GenesisState}; -use crate::{Account, AccountOwnerKey, Accounts}; +use crate::Accounts; /// Account data for the genesis. #[serde_as] @@ -54,12 +54,7 @@ impl Accounts { if self.accounts.get(&acc.credential_id, state)?.is_some() { bail!("Account already exists") } - - let new_account = Account { addr: acc.address }; - self.accounts.set(&acc.credential_id, &new_account, state)?; - - let key = AccountOwnerKey::new(acc.address, acc.credential_id); - self.account_owners.set(&key, &true, state)?; + self.register_credential(&acc.address, &acc.credential_id, state)?; } Ok(()) diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index fee221639a..c5b0f6cd35 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -34,17 +34,15 @@ pub struct Account { pub addr: S::Address, } -/// Composite key used by [`Accounts::account_owners`]. A present entry -/// `(address, credential_id)` means `credential_id` is authorized to spend as -/// `address`. The key shape supports many-to-many authorization, while the -/// current compatibility index in [`Accounts::accounts`] preserves one primary -/// address per credential for existing resolution and query consumers. +/// Composite key for [`Accounts::account_owners`]. A present entry +/// `(address, credential_id)` means `credential_id` is authorized to sign +/// transactions that execute as `address`. #[derive( borsh::BorshDeserialize, borsh::BorshSerialize, Debug, Clone, Copy, PartialEq, Eq, Hash, )] pub(crate) struct AccountOwnerKey { - pub(crate) address: S::Address, - pub(crate) credential_id: CredentialId, + address: S::Address, + credential_id: CredentialId, } impl AccountOwnerKey { @@ -56,9 +54,8 @@ impl AccountOwnerKey { } } -// `Display` and `FromStr` are required by `StateMap`'s struct-level trait bound -// (see `sov-modules-api/src/containers/map.rs`). The encoding is only used for -// REST API paths and debug output; on-chain keys are Borsh-serialized. +// `Display` / `FromStr` exist only to satisfy `StateMap`'s trait bound; on-chain +// keys are Borsh-serialized. impl std::fmt::Display for AccountOwnerKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}/{}", self.address, self.credential_id) @@ -69,7 +66,7 @@ impl std::str::FromStr for AccountOwnerKey { type Err = anyhow::Error; fn from_str(s: &str) -> Result { let (addr_str, cred_str) = s - .split_once('/') + .rsplit_once('/') .ok_or_else(|| anyhow::anyhow!("invalid AccountOwnerKey: missing '/' separator"))?; Ok(Self { address: addr_str @@ -90,9 +87,9 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// `credential_id -> address` routing index. This map is maintained with - /// [`Self::account_owners`] so existing consumers can resolve and query the - /// primary address for a credential without scanning the authorization map. + /// `credential_id -> address` routing index, kept in lockstep with + /// [`Self::account_owners`] so `get_account` and `resolve_sender_address` + /// can answer with one lookup instead of scanning the authorization map. #[state] pub(crate) accounts: StateMap>, @@ -100,11 +97,9 @@ pub struct Accounts { #[state] enable_custom_account_mappings: StateValue, - /// Authorization relation. A present entry `(address, credential_id)` means - /// `credential_id` is authorized to sign transactions that execute as - /// `address`. Current write paths also maintain [`Self::accounts`] as the - /// primary address index for compatibility with existing resolution and - /// query APIs. + /// Authorization set: a present entry means `credential_id` may sign as + /// `address`. Every write is paired with [`Self::accounts`], so + /// [`Self::is_authorized`] can rely on this map alone. #[state] pub(crate) account_owners: StateMap, bool>, } diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 56642b1b80..112c4ee858 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -73,17 +73,12 @@ fn test_config_account() { runner, ) = setup(); - // The account is registered at genesis: `(user.address(), user.credential_id())` - // must appear in `account_owners`, and `get_account` must see the - // credential's primary address. + // The account is registered at genesis. runner.query_visible_state(|state| { let accounts = Accounts::::default(); - assert!( - accounts - .is_authorized(&user.address(), &user.credential_id(), state) - .unwrap(), - "genesis-registered credential should be authorized for its address" - ); + assert!(accounts + .is_authorized(&user.address(), &user.credential_id(), state) + .unwrap()); assert_eq!( accounts.get_account(user.credential_id(), state), Response::AccountExists { @@ -115,26 +110,19 @@ fn test_update_account() { let accounts = Accounts::::default(); // The new credential is authorized to spend as the sender's address. - assert!( - accounts - .is_authorized(&user.address(), &new_credential, state) - .unwrap(), - "InsertCredentialId must authorize the new credential for the sender" - ); + assert!(accounts + .is_authorized(&user.address(), &new_credential, state) + .unwrap()); assert_eq!( accounts.get_account(new_credential, state), Response::AccountExists { addr: user.address() } ); - // The sender's own credential is still authorized (auto-registered on - // first use when the tx was processed). - assert!( - accounts - .is_authorized(&user.address(), &user.credential_id(), state) - .unwrap(), - "the sender's own credential stays authorized" - ); + // The sender's own credential is auto-registered on first use. + assert!(accounts + .is_authorized(&user.address(), &user.credential_id(), state) + .unwrap()); assert_eq!( accounts.get_account(user.credential_id(), state), Response::AccountExists { @@ -147,8 +135,7 @@ fn test_update_account() { }); } -/// A credential already mapped to one address cannot be inserted for another -/// address. Existing consumers rely on this global credential uniqueness check. +/// A credential already mapped to one address cannot be inserted for another. #[test] fn test_insert_existing_credential_fails() { let ( @@ -180,16 +167,13 @@ fn test_insert_existing_credential_fails() { /// Tests the multisig functionality of the Accounts module. /// -/// The multisig's effective signing address is its stateless default -/// (`multisig_credential_id.into()`). We seed genesis with a `TestUser` whose -/// custom `credential_id` matches the multisig, so the default address has a -/// gas balance and is already mapped before any tx is submitted. This keeps the -/// focus on signature-level invariants. +/// Seeds genesis with a `TestUser` whose custom `credential_id` matches the +/// multisig, so the multisig's default address is funded and mapped before any +/// tx is submitted. This keeps the focus on signature-level invariants. #[test] fn test_setup_multisig_and_act() { use sov_modules_api::Multisig; - // Generate the multisig first so we know its credential_id at genesis time. let multisig_keys = [ TestPrivateKey::generate(), TestPrivateKey::generate(), @@ -398,16 +382,13 @@ fn test_register_new_account() { accounts.get_account(non_registered_account.credential_id(), state), Response::AccountEmpty ); - assert!( - !accounts - .is_authorized( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state - ) - .unwrap(), - "unregistered account should have no authorization at genesis" - ); + assert!(!accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap()); }); let new_credential = TestPrivateKey::generate().pub_key().credential_id(); @@ -422,12 +403,9 @@ fn test_register_new_account() { let accounts = Accounts::::default(); // The new credential is authorized for the sender's address. - assert!( - accounts - .is_authorized(&non_registered_account.address(), &new_credential, state) - .unwrap(), - "new credential should be authorized for the sender's address" - ); + assert!(accounts + .is_authorized(&non_registered_account.address(), &new_credential, state) + .unwrap()); assert_eq!( accounts.get_account(new_credential, state), Response::AccountExists { @@ -435,18 +413,14 @@ fn test_register_new_account() { } ); - // The sender's own credential is auto-registered for the same - // address when the tx flowed through `resolve_sender_address`. - assert!( - accounts - .is_authorized( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state - ) - .unwrap(), - "the sender's own credential is auto-registered on first tx" - ); + // The sender's own credential is auto-registered on first tx. + assert!(accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap()); assert_eq!( accounts.get_account(non_registered_account.credential_id(), state), Response::AccountExists { @@ -500,15 +474,13 @@ fn test_resolve_sender_address_registered() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); - // Resolving with account_1's registered address as the default returns - // it directly (step 1 in the resolver). assert_eq!( accounts .resolve_sender_address(&account_1.address(), &account_1.credential_id(), state) .unwrap(), account_1.address() ); - + // Ensure correct (registered) address is used even if another fallback is provided. assert_eq!( accounts .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) @@ -518,8 +490,8 @@ fn test_resolve_sender_address_registered() { }); } -/// After `InsertCredentialId` from a user, the new credential is authorized -/// for that user's address and resolves to that user's address. +/// After `InsertCredentialId` from a user, each inserted credential resolves +/// to and is authorized under that user's address. #[test] fn test_resolve_address_with_multi_credential_ownership() { let ( @@ -544,40 +516,30 @@ fn test_resolve_address_with_multi_credential_ownership() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); + let addr = non_registered_account.address(); - // With the user's address as default, both credentials route to it - // (they were authorized via `InsertCredentialId`). assert_eq!( accounts - .resolve_sender_address(&non_registered_account.address(), &credential_1, state) + .resolve_sender_address(&addr, &credential_1, state) .unwrap(), - non_registered_account.address() + addr ); assert_eq!( accounts - .resolve_sender_address(&non_registered_account.address(), &credential_2, state) + .resolve_sender_address(&addr, &credential_2, state) .unwrap(), - non_registered_account.address() + addr ); - // Both credentials are authorized under the user's address. - assert!(accounts - .is_authorized(&non_registered_account.address(), &credential_1, state) - .unwrap()); - assert!(accounts - .is_authorized(&non_registered_account.address(), &credential_2, state) - .unwrap()); + assert!(accounts.is_authorized(&addr, &credential_1, state).unwrap()); + assert!(accounts.is_authorized(&addr, &credential_2, state).unwrap()); assert_eq!( accounts.get_account(credential_1, state), - Response::AccountExists { - addr: non_registered_account.address() - } + Response::AccountExists { addr } ); assert_eq!( accounts.get_account(credential_2, state), - Response::AccountExists { - addr: non_registered_account.address() - } + Response::AccountExists { addr } ); }); } @@ -604,91 +566,6 @@ fn test_resolve_with_different_default_address() { }); } -/// Verifies every write path (genesis, auto-register on first resolve, and -/// `InsertCredentialId`) updates both the `accounts` index and `account_owners`. -#[test] -fn test_account_write_paths() { - let ( - TestData { - account_1, - non_registered_account, - .. - }, - mut runner, - ) = setup(); - - // Genesis path. - runner.query_visible_state(|state| { - let accounts = Accounts::::default(); - assert!( - accounts - .is_authorized(&account_1.address(), &account_1.credential_id(), state) - .unwrap(), - "genesis-registered credential should be present in account_owners" - ); - assert_eq!( - accounts.get_account(account_1.credential_id(), state), - Response::AccountExists { - addr: account_1.address() - } - ); - }); - - // Auto-register path: a `resolve_sender_address` call for an unseen - // credential writes the tuple. `query_visible_state` scopes the write to - // this closure so the check is in-session. - runner.query_visible_state(|state| { - let mut accounts = Accounts::::default(); - let _ = accounts - .resolve_sender_address( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state, - ) - .unwrap(); - assert!( - accounts - .is_authorized( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state - ) - .unwrap(), - "auto-registered credential should be present in account_owners" - ); - assert_eq!( - accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountExists { - addr: non_registered_account.address() - } - ); - }); - - // InsertCredentialId path. - let new_credential = TestPrivateKey::generate().pub_key().credential_id(); - runner.execute_transaction(TransactionTestCase { - input: non_registered_account.create_plain_message::>( - CallMessage::InsertCredentialId(new_credential), - ), - assert: Box::new(move |result, state| { - assert!(result.tx_receipt.is_successful()); - let accounts = Accounts::::default(); - assert!( - accounts - .is_authorized(&non_registered_account.address(), &new_credential, state) - .unwrap(), - "InsertCredentialId should populate account_owners under sender's address" - ); - assert_eq!( - accounts.get_account(new_credential, state), - Response::AccountExists { - addr: non_registered_account.address() - } - ); - }), - }); -} - #[test] fn test_disable_custom_account_mappings() { let (user, mut runner) = setup_with_disable_custom_account_mappings(); From 7ade9ef3a044649811e70dc6e03130d921aad0ca Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Wed, 22 Apr 2026 10:16:18 +0200 Subject: [PATCH 04/21] Reverting dual-write --- .../hyperlane-solana-register/README.md | 7 +- .../hyperlane-solana-register/src/lib.rs | 22 +-- .../tests/integration/registration.rs | 41 ++---- .../sov-accounts/README.md | 27 ++-- .../sov-accounts/src/call.rs | 20 ++- .../sov-accounts/src/capabilities.rs | 38 +++-- .../sov-accounts/src/genesis.rs | 7 +- .../sov-accounts/src/lib.rs | 8 +- .../sov-accounts/src/query.rs | 2 +- .../sov-accounts/tests/integration/main.rs | 130 ++++++++++++------ 10 files changed, 181 insertions(+), 121 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md index 6ddd90a2c2..2363bc92d0 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md @@ -132,7 +132,7 @@ pub enum Event { The module defines several error types: -- `AlreadyRegistered`: The embedded public key is already linked to a different address +- `Unauthorized`: The embedded public key is not authorized for the payer address - `InvalidBodyLength`: The message body doesn't contain exactly 64 bytes - `ExtractPubKey`: Failed to parse public keys from the message body - `AdminNotFound`: Admin address not configured @@ -146,8 +146,8 @@ See `src/lib.rs:196-213` for the `handle` implementation: 2. Verify the sender matches the trusted Solana program ID 3. Extract the two 32-byte public keys from the message body 4. Convert the payer public key to a rollup address -5. Use `sov-accounts` to resolve or create the address-credential mapping -6. Reject if the embedded wallet is already linked to a different address +5. Use `sov-accounts` to verify the embedded credential is authorized for the payer address +6. Reject if the embedded credential is not authorized for that address 7. Emit a `UserRegistered` event --- @@ -372,4 +372,3 @@ cargo test ``` Program tests are located in `solana/program-tests/src/tests.rs`. - diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs index d5c20fa675..e43ed6afb7 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs @@ -53,10 +53,10 @@ where pub enum SolanaRegistrationError { #[error("Core module error: {0}")] CoreModuleError(#[from] CoreModuleError), - #[error("Embedded pubkey already registered to different address. Attempted: {attempted_address}, Registered: {registered_address}")] - AlreadyRegistered { - attempted_address: String, - registered_address: String, + #[error("Embedded pubkey is not authorized for address. Address: {address}, CredentialId: {credential_id}")] + Unauthorized { + address: String, + credential_id: String, }, #[error("Invalid body length. Expected {expected}, found {found}")] InvalidBodyLength { expected: usize, found: usize }, @@ -297,15 +297,15 @@ where let (user_pubkey, embedded_pubkey) = self.unpack_body(body.as_ref())?; let credential_id = CredentialId::from(embedded_pubkey); let address = S::Address::try_from(&user_pubkey).map_err(CoreModuleError::from)?; - let resolved_address = self + let is_authorized = self .accounts - .resolve_sender_address(&address, &credential_id, state) - .map_err(CoreModuleError::state_write)?; + .is_authorized_for(&address, &credential_id, state) + .map_err(CoreModuleError::state_read)?; - if address != resolved_address { - Err(SolanaRegistrationError::AlreadyRegistered { - attempted_address: address.to_string(), - registered_address: resolved_address.to_string(), + if !is_authorized { + Err(SolanaRegistrationError::Unauthorized { + address: address.to_string(), + credential_id: credential_id.to_string(), }) } else { self.emit_event( diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index 189340d28a..11df7e9012 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -23,13 +23,13 @@ fn test_user_is_registered_correctly() { let route_id = register_basic_warp_route(&mut runner, &admin); let payer = [1u8; 32]; - let embedded = [2u8; 32]; + let embedded = payer; let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); let credential = CredentialId::from(embedded); - // Sanity check, ensure account definently doesnt already exist + // Sanity check, ensure registration does not rely on an accounts map entry. runner.query_state(|state| { let account = sov_accounts::Accounts::default().get_account(credential, state); assert!(matches!(account, sov_accounts::Response::AccountEmpty)); @@ -60,15 +60,12 @@ fn test_user_is_registered_correctly() { runner.query_state(|state| { let account = sov_accounts::Accounts::default().get_account(credential, state); - assert!(matches!( - account, - sov_accounts::Response::AccountExists { .. } - )); + assert!(matches!(account, sov_accounts::Response::AccountEmpty)); }); } #[test] -fn test_errors_if_user_already_registered() { +fn test_errors_if_embedded_pubkey_is_not_authorized_for_payer() { let SetupParams { mut runner, admin, @@ -82,26 +79,11 @@ fn test_errors_if_user_already_registered() { let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); - - runner.execute_transaction(TransactionTestCase { - input: user.create_plain_message::>(CallMessage::Process { - metadata: HexString::new(SafeVec::new()), - message: message.clone(), - }), - assert: Box::new(move |result, _| { - assert!( - result.tx_receipt.is_successful(), - "Recipient was not registered successfully" - ); - }), - }); - - // payer is different so will try to register to different address - let payer = [3u8; 32]; - let embedded = [2u8; 32]; - let body = [payer, embedded].concat(); - let valid_message = make_valid_message(1, route_id, HexString::new(body)); - let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); + let expected_error = format!( + "Embedded pubkey is not authorized for address. Address: {}, CredentialId: {}", + ::Address::from(payer), + CredentialId::from(embedded) + ); runner.execute_transaction(TransactionTestCase { input: user.create_plain_message::>(CallMessage::Process { @@ -110,10 +92,7 @@ fn test_errors_if_user_already_registered() { }), assert: Box::new(move |result, _| match result.tx_receipt { sov_rollup_interface::stf::TxEffect::Reverted(contents) => { - assert_eq!( - contents.reason.to_string(), - "Embedded pubkey already registered to different address. Attempted: CktRuQ2mttgRGkXJtyksdKHjUdc2C4TgDzyB98oEzy8, Registered: 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi".to_string() - ); + assert_eq!(contents.reason.to_string(), expected_error); } _ => panic!("Registration should have reverted: {:?}", result.tx_receipt), }), diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index bcad5e3e51..4352aeb9ca 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -10,15 +10,15 @@ addresses and records which credentials may act for which addresses. canonical address does not require an account entry to be written. 1. A credential can be explicitly mapped to a primary address in module state. - This is used for custom account mappings, duplicate credential checks, - credential-only resolution, and the `get_account` query. + This legacy/custom map is used for credential-only resolution and the + `get_account` query. 1. It is possible to register another credential for the caller's address using - the `CallMessage::InsertCredentialId(..)` message. The call fails if that - credential is already explicitly mapped to any address. + the `CallMessage::InsertCredentialId(..)` message. This writes an + `account_owners` authorization, not a credential-indexed account entry. 1. It is possible to query the `sov-accounts` module using the `get_account` - method and get the explicitly mapped account corresponding to the given + method and get the legacy/custom mapped account corresponding to the given credential id. ## Credential and Address Relations @@ -47,10 +47,11 @@ credential. A credential has at most one primary address in this map, while one address may be the primary address for many credentials. This map is used when callers only know a `CredentialId` and need an address: -`resolve_sender_address`, `get_account`, duplicate credential checks, and -legacy/custom account mappings all depend on this credential-indexed lookup. -`get_account` reports entries from this explicit map; it does not mean that -every possible stateless canonical address has a stored account entry. +`resolve_sender_address`, `get_account`, and legacy/custom account mappings all +depend on this credential-indexed lookup. New `InsertCredentialId` calls do not +write this map. `get_account` reports entries from this explicit map; it does +not mean that every possible stateless canonical address or explicit +authorization has a stored account entry. ### Account-credential authorization map @@ -59,13 +60,17 @@ account_owners[(address, credential_id)] = true ``` This state map records authorization. A present entry means the credential is -authorized to sign transactions that execute as the given address. +authorized to sign transactions that execute as the given address. The key is +the exact `(address, credential_id)` pair, so this relation does not provide a +credential-only lookup by itself. This relation answers "may this credential act as this address?" once the target address is known. It is not a replacement for the credential-to-account map, because it is keyed by `(address, credential_id)` and cannot efficiently answer "which address is this credential registered to?" with a single point lookup. +New `InsertCredentialId` calls write this relation. In short: routing from only a credential uses the explicit credential-to-account map or the stateless canonical fallback. Authorization for -a known address uses `account_owners`. +a known address uses the combined `is_authorized_for` check: legacy/custom +mapping, stateless canonical address, or `account_owners`. diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 2bb306d3b5..696b6bba38 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -4,16 +4,16 @@ use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; use sov_state::namespaces::User; -use crate::Accounts; +use crate::{AccountOwnerKey, Accounts}; /// Represents the available call messages for interacting with the sov-accounts module. #[derive(Debug, PartialEq, Eq, Clone, JsonSchema, UniversalWallet)] #[serialize(Borsh, Serde)] #[serde(rename_all = "snake_case")] pub enum CallMessage { - /// Registers `credential_id` as an authorized signer whose transactions - /// execute as the caller's address. Fails if the credential is already - /// mapped to any address in this module (credentials are globally unique). + /// Authorizes `credential_id` as a signer for the caller's address. + /// Fails if the credential has a legacy/custom account mapping or is + /// already authorized for the caller's address. InsertCredentialId( /// The credential id being registered. CredentialId, @@ -33,15 +33,16 @@ impl Accounts { bail!("Custom account mappings are disabled"); } - self.exit_if_credential_exists(&new_credential_id, state)?; + self.exit_if_credential_exists(&new_credential_id, context.sender(), state)?; - self.register_credential(context.sender(), &new_credential_id, state)?; + self.authorize_credential(context.sender(), &new_credential_id, state)?; Ok(()) } fn exit_if_credential_exists( &self, new_credential_id: &CredentialId, + address: &S::Address, state: &mut impl StateReader, ) -> Result<()> { anyhow::ensure!( @@ -51,6 +52,13 @@ impl Accounts { .is_none(), "New CredentialId already exists" ); + anyhow::ensure!( + self.account_owners + .get(&AccountOwnerKey::new(*address, *new_credential_id), state) + .map_err(|err| anyhow!("Error raised while getting account owner: {err:?}"))? + .is_none(), + "CredentialId already authorized for this address" + ); Ok(()) } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index df526b858e..f1cf214f92 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -1,20 +1,16 @@ use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, StateWriter}; use sov_state::User; -use crate::{Account, AccountOwnerKey, Accounts}; +use crate::{AccountOwnerKey, Accounts}; impl Accounts { - /// Writes both indices that together assert `credential_id` is authorized - /// to sign as `address`. Callers must maintain this pairing so that - /// [`Self::is_authorized`] can rely on `account_owners` alone. - pub(crate) fn register_credential( + /// Authorizes `credential_id` to sign as `address`. + pub(crate) fn authorize_credential>( &mut self, address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result<(), >::Error> { - self.accounts - .set(credential_id, &Account { addr: *address }, state)?; self.account_owners.set( &AccountOwnerKey::new(*address, *credential_id), &true, @@ -22,8 +18,9 @@ impl Accounts { ) } - /// Resolve the sender's public key to an address. If `credential_id` is - /// unknown, register it to `default_address` and return that. + /// Resolve the sender's public key to an address. If `credential_id` has a + /// legacy/custom mapping in `accounts`, return it. Otherwise, return the + /// supplied `default_address` without writing account state. pub fn resolve_sender_address( &mut self, default_address: &S::Address, @@ -33,7 +30,6 @@ impl Accounts { if let Some(account) = self.accounts.get(credential_id, state)? { return Ok(account.addr); } - self.register_credential(default_address, credential_id, state)?; Ok(*default_address) } @@ -64,4 +60,26 @@ impl Accounts { .get(&AccountOwnerKey::new(*address, *credential_id), state)? .is_some()) } + + /// Returns `true` if `credential_id` is authorized to act as `address` + /// under any supported relation: legacy/custom `accounts` mapping, + /// stateless canonical address, or explicit `account_owners` + /// authorization. + pub fn is_authorized_for>( + &self, + address: &S::Address, + credential_id: &CredentialId, + state: &mut ST, + ) -> Result { + if let Some(account) = self.accounts.get(credential_id, state)? { + return Ok(account.addr == *address); + } + + let canonical_address: S::Address = (*credential_id).into(); + if canonical_address == *address { + return Ok(true); + } + + self.is_authorized(address, credential_id, state) + } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index f03efecd2d..1631a731fa 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -4,7 +4,7 @@ use serde_with::{serde_as, DisplayFromStr}; use sov_modules_api::prelude::*; use sov_modules_api::{CredentialId, GenesisState}; -use crate::Accounts; +use crate::{AccountOwnerKey, Accounts}; /// Account data for the genesis. #[serde_as] @@ -51,10 +51,11 @@ impl Accounts { } for acc in &config.accounts { - if self.accounts.get(&acc.credential_id, state)?.is_some() { + let key = AccountOwnerKey::new(acc.address, acc.credential_id); + if self.account_owners.get(&key, state)?.is_some() { bail!("Account already exists") } - self.register_credential(&acc.address, &acc.credential_id, state)?; + self.authorize_credential(&acc.address, &acc.credential_id, state)?; } Ok(()) diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index c5b0f6cd35..06986dc3c1 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -87,9 +87,8 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// `credential_id -> address` routing index, kept in lockstep with - /// [`Self::account_owners`] so `get_account` and `resolve_sender_address` - /// can answer with one lookup instead of scanning the authorization map. + /// Legacy/custom `credential_id -> address` routing index. New + /// authorization writes use [`Self::account_owners`] instead. #[state] pub(crate) accounts: StateMap>, @@ -98,8 +97,7 @@ pub struct Accounts { enable_custom_account_mappings: StateValue, /// Authorization set: a present entry means `credential_id` may sign as - /// `address`. Every write is paired with [`Self::accounts`], so - /// [`Self::is_authorized`] can rely on this map alone. + /// `address`. #[state] pub(crate) account_owners: StateMap, bool>, } diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs index 15ae9c039d..ed11a7ad0f 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/query.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/query.rs @@ -21,7 +21,7 @@ pub enum Response { } impl Accounts { - /// Get the account corresponding to the given credential id. + /// Get the legacy/custom account mapping corresponding to the given credential id. pub fn get_account( &self, credential_id: CredentialId, diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 112c4ee858..7b7f59281d 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -73,17 +73,19 @@ fn test_config_account() { runner, ) = setup(); - // The account is registered at genesis. + // The account is authorized at genesis, but no new `accounts` map entry is + // written for canonical credentials. runner.query_visible_state(|state| { let accounts = Accounts::::default(); assert!(accounts .is_authorized(&user.address(), &user.credential_id(), state) .unwrap()); + assert!(accounts + .is_authorized_for(&user.address(), &user.credential_id(), state) + .unwrap()); assert_eq!( accounts.get_account(user.credential_id(), state), - Response::AccountExists { - addr: user.address() - } + Response::AccountEmpty ); }); } @@ -113,21 +115,24 @@ fn test_update_account() { assert!(accounts .is_authorized(&user.address(), &new_credential, state) .unwrap()); + assert!(accounts + .is_authorized_for(&user.address(), &new_credential, state) + .unwrap()); assert_eq!( accounts.get_account(new_credential, state), - Response::AccountExists { - addr: user.address() - } + Response::AccountEmpty ); - // The sender's own credential is auto-registered on first use. - assert!(accounts + // The sender's own credential uses stateless canonical ownership; + // resolving the tx no longer writes account authorization state. + assert!(!accounts .is_authorized(&user.address(), &user.credential_id(), state) .unwrap()); + assert!(accounts + .is_authorized_for(&user.address(), &user.credential_id(), state) + .unwrap()); assert_eq!( accounts.get_account(user.credential_id(), state), - Response::AccountExists { - addr: user.address() - } + Response::AccountEmpty ); assert_ne!(new_credential, user.credential_id()); @@ -135,29 +140,37 @@ fn test_update_account() { }); } -/// A credential already mapped to one address cannot be inserted for another. +/// A credential already authorized for an address cannot be inserted twice. #[test] fn test_insert_existing_credential_fails() { let ( TestData { - account_1, non_registered_account: sender, .. }, mut runner, ) = setup(); - let peer_credential = account_1.credential_id(); + let new_credential = TestPrivateKey::generate().pub_key().credential_id(); + + runner.execute_transaction(TransactionTestCase { + input: sender.create_plain_message::>(CallMessage::InsertCredentialId( + new_credential, + )), + assert: Box::new(move |result, _state| { + assert!(result.tx_receipt.is_successful()); + }), + }); runner.execute_transaction(TransactionTestCase { input: sender.create_plain_message::>(CallMessage::InsertCredentialId( - peer_credential, + new_credential, )), assert: Box::new(move |result, _state| match result.tx_receipt { TxEffect::Reverted(contents) => { assert_eq!( contents.reason.to_string(), - "New CredentialId already exists" + "CredentialId already authorized for this address" ); } _ => panic!("Expected reverted transaction for existing credential"), @@ -168,8 +181,8 @@ fn test_insert_existing_credential_fails() { /// Tests the multisig functionality of the Accounts module. /// /// Seeds genesis with a `TestUser` whose custom `credential_id` matches the -/// multisig, so the multisig's default address is funded and mapped before any -/// tx is submitted. This keeps the focus on signature-level invariants. +/// multisig, so the multisig's canonical address is funded before any tx is +/// submitted. This keeps the focus on signature-level invariants. #[test] fn test_setup_multisig_and_act() { use sov_modules_api::Multisig; @@ -389,6 +402,13 @@ fn test_register_new_account() { state ) .unwrap()); + assert!(accounts + .is_authorized_for( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap()); }); let new_credential = TestPrivateKey::generate().pub_key().credential_id(); @@ -406,26 +426,33 @@ fn test_register_new_account() { assert!(accounts .is_authorized(&non_registered_account.address(), &new_credential, state) .unwrap()); + assert!(accounts + .is_authorized_for(&non_registered_account.address(), &new_credential, state) + .unwrap()); assert_eq!( accounts.get_account(new_credential, state), - Response::AccountExists { - addr: non_registered_account.address() - } + Response::AccountEmpty ); - // The sender's own credential is auto-registered on first tx. - assert!(accounts + // The sender's own credential uses stateless canonical ownership; + // resolving the tx no longer writes account authorization state. + assert!(!accounts .is_authorized( &non_registered_account.address(), &non_registered_account.credential_id(), state ) .unwrap()); + assert!(accounts + .is_authorized_for( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap()); assert_eq!( accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountExists { - addr: non_registered_account.address() - } + Response::AccountEmpty ); assert_ne!(new_credential, non_registered_account.credential_id()); @@ -455,11 +482,22 @@ fn test_resolve_sender_address_with_default_address_non_registered() { .unwrap(), non_registered_account.address() ); + assert!(!accounts + .is_authorized( + &non_registered_account.address(), + &non_registered_account.credential_id(), + state + ) + .unwrap()); + assert_eq!( + accounts.get_account(non_registered_account.credential_id(), state), + Response::AccountEmpty + ); }); } -/// Genesis-registered credentials resolve to their configured address even when -/// a different fallback address is supplied. +/// Genesis-authorized credentials have no `accounts` entry, so credential-only +/// resolution falls back to the supplied address. #[test] fn test_resolve_sender_address_registered() { let ( @@ -480,18 +518,21 @@ fn test_resolve_sender_address_registered() { .unwrap(), account_1.address() ); - // Ensure correct (registered) address is used even if another fallback is provided. assert_eq!( accounts .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) .unwrap(), - account_1.address() + account_2.address() ); + assert!(accounts + .is_authorized_for(&account_1.address(), &account_1.credential_id(), state) + .unwrap()); }); } -/// After `InsertCredentialId` from a user, each inserted credential resolves -/// to and is authorized under that user's address. +/// After `InsertCredentialId` from a user, each inserted credential is +/// authorized under that user's address without getting a credential-indexed +/// account entry. #[test] fn test_resolve_address_with_multi_credential_ownership() { let ( @@ -533,21 +574,25 @@ fn test_resolve_address_with_multi_credential_ownership() { assert!(accounts.is_authorized(&addr, &credential_1, state).unwrap()); assert!(accounts.is_authorized(&addr, &credential_2, state).unwrap()); + assert!(accounts + .is_authorized_for(&addr, &credential_1, state) + .unwrap()); + assert!(accounts + .is_authorized_for(&addr, &credential_2, state) + .unwrap()); assert_eq!( accounts.get_account(credential_1, state), - Response::AccountExists { addr } + Response::AccountEmpty ); assert_eq!( accounts.get_account(credential_2, state), - Response::AccountExists { addr } + Response::AccountEmpty ); }); } -/// This test should verify that when a new credential is specified with an existing account's -/// address as fallback, the credential is registered to that address. However -/// query_visible_state doesn't mutate the state so it simply verifies that the fallback address is -/// returned correctly +/// Resolving an unknown credential returns the supplied fallback without +/// writing account state. #[test] fn test_resolve_with_different_default_address() { let (TestData { account_1, .. }, runner) = setup(); @@ -563,6 +608,13 @@ fn test_resolve_with_different_default_address() { .unwrap(), account_1.address() ); + assert!(!accounts + .is_authorized(&account_1.address(), &random_credential, state) + .unwrap()); + assert_eq!( + accounts.get_account(random_credential, state), + Response::AccountEmpty + ); }); } From 256f9a076fe03c53326b35d6dce098df0bacbac9 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Wed, 22 Apr 2026 10:27:55 +0200 Subject: [PATCH 05/21] Updating documentation --- .../sov-accounts/README.md | 31 ++++++++++--------- .../sov-accounts/src/call.rs | 2 +- .../sov-accounts/src/capabilities.rs | 13 ++++---- .../sov-accounts/src/fuzz.rs | 2 +- .../sov-accounts/src/genesis.rs | 10 +++--- .../sov-accounts/src/lib.rs | 10 +++--- .../sov-accounts/src/query.rs | 6 ++-- .../sov-accounts/tests/integration/main.rs | 15 ++++----- 8 files changed, 47 insertions(+), 42 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index 4352aeb9ca..c5e4f66632 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -9,16 +9,15 @@ addresses and records which credentials may act for which addresses. `credential_id.into::()`. This relation is stateless: using the canonical address does not require an account entry to be written. -1. A credential can be explicitly mapped to a primary address in module state. - This legacy/custom map is used for credential-only resolution and the - `get_account` query. +1. Legacy state can contain an explicit `credential_id -> address` mapping. + This map is used for credential-only resolution and the `get_account` query. -1. It is possible to register another credential for the caller's address using +1. It is possible to authorize another credential for the caller's address using the `CallMessage::InsertCredentialId(..)` message. This writes an `account_owners` authorization, not a credential-indexed account entry. 1. It is possible to query the `sov-accounts` module using the `get_account` - method and get the legacy/custom mapped account corresponding to the given + method and get the legacy/custom mapped address corresponding to the given credential id. ## Credential and Address Relations @@ -33,8 +32,9 @@ credential_id -> credential_id.into::() ``` This is the default address for a credential. It is deterministic and requires -no state write. If a credential has no explicit state mapping, this canonical -address is the natural fallback for credential-only routing. +no state write. If a credential has no explicit credential-indexed state +mapping, this canonical address is the natural fallback for credential-only +routing. ### Credential-to-account map @@ -42,9 +42,9 @@ address is the natural fallback for credential-only routing. accounts[credential_id] = Account { addr } ``` -This state map records the primary address explicitly associated with a -credential. A credential has at most one primary address in this map, while one -address may be the primary address for many credentials. +This legacy/custom state map records the primary address explicitly associated +with a credential. A credential has at most one primary address in this map, +while one address may be the primary address for many credentials. This map is used when callers only know a `CredentialId` and need an address: `resolve_sender_address`, `get_account`, and legacy/custom account mappings all @@ -67,10 +67,11 @@ credential-only lookup by itself. This relation answers "may this credential act as this address?" once the target address is known. It is not a replacement for the credential-to-account map, because it is keyed by `(address, credential_id)` and cannot efficiently answer -"which address is this credential registered to?" with a single point lookup. +"which address is this credential mapped to?" with a single point lookup. New `InsertCredentialId` calls write this relation. -In short: routing from only a credential uses the explicit -credential-to-account map or the stateless canonical fallback. Authorization for -a known address uses the combined `is_authorized_for` check: legacy/custom -mapping, stateless canonical address, or `account_owners`. +In short: routing from only a credential uses the explicit credential-to-account +map or the stateless canonical fallback. Callers that need to verify whether a +known address may be used with a credential should use the combined +`is_authorized_for` check: legacy/custom mapping, stateless canonical address, +or `account_owners`. diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 696b6bba38..076d8a40a1 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -15,7 +15,7 @@ pub enum CallMessage { /// Fails if the credential has a legacy/custom account mapping or is /// already authorized for the caller's address. InsertCredentialId( - /// The credential id being registered. + /// The credential id being authorized. CredentialId, ), } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index f1cf214f92..a964587945 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -18,9 +18,9 @@ impl Accounts { ) } - /// Resolve the sender's public key to an address. If `credential_id` has a - /// legacy/custom mapping in `accounts`, return it. Otherwise, return the - /// supplied `default_address` without writing account state. + /// Resolve the sender's credential to an address. + /// If `credential_id` has a legacy/custom mapping in `accounts`, return it. + /// Otherwise, return the supplied `default_address` without writing account state. pub fn resolve_sender_address( &mut self, default_address: &S::Address, @@ -34,7 +34,8 @@ impl Accounts { } /// Read-only variant of [`Self::resolve_sender_address`]: returns - /// `default_address` when the credential is unknown, without writing. + /// `default_address` when the credential has no legacy/custom mapping, + /// without writing. pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, @@ -47,8 +48,8 @@ impl Accounts { Ok(*default_address) } - /// Returns `true` if `credential_id` is authorized to sign transactions - /// that execute as `address`. + /// Returns `true` if `credential_id` has an explicit `account_owners` + /// authorization for `address`. pub fn is_authorized>( &self, address: &S::Address, diff --git a/crates/module-system/module-implementations/sov-accounts/src/fuzz.rs b/crates/module-system/module-implementations/sov-accounts/src/fuzz.rs index b00b8422e9..2abc5e43a3 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/fuzz.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/fuzz.rs @@ -51,7 +51,7 @@ where ::BlockHeader: Default, ::PublicKey: Arbitrary<'a>, { - /// Creates an arbitrary set of accounts and stores it under `state`. + /// Creates arbitrary genesis credential/address authorizations under `state`. pub fn arbitrary_workset( u: &mut Unstructured<'a>, state: &mut StateCheckpoint, diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index 1631a731fa..5d18e27beb 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -6,15 +6,15 @@ use sov_modules_api::{CredentialId, GenesisState}; use crate::{AccountOwnerKey, Accounts}; -/// Account data for the genesis. +/// Credential/address authorization data for genesis. #[serde_as] #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize, JsonSchema)] #[serde(deny_unknown_fields)] pub struct AccountData
{ - /// Credential ID of the account. + /// Credential ID to authorize. #[serde_as(as = "DisplayFromStr")] pub credential_id: CredentialId, - /// Address of the account. + /// Address the credential may act as. pub address: Address, } @@ -23,9 +23,9 @@ pub struct AccountData
{ #[serde(deny_unknown_fields)] #[schemars(bound = "S: ::sov_modules_api::Spec", rename = "AccountConfig")] pub struct AccountConfig { - /// Accounts to initialize the rollup. + /// Credential/address authorizations to initialize. pub accounts: Vec>, - /// Enable custom `CredentialId` => `Account` mapping. + /// Enable configured credential authorizations and `InsertCredentialId`. #[serde(default = "default_true")] pub enable_custom_account_mappings: bool, } diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 06986dc3c1..247ff3e718 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -18,7 +18,7 @@ use sov_modules_api::{ StateMap, StateValue, TxState, }; -/// An account on the rollup. +/// Stored address for a legacy/custom credential-indexed account mapping. #[derive( borsh::BorshDeserialize, borsh::BorshSerialize, @@ -30,7 +30,7 @@ use sov_modules_api::{ Clone, )] pub struct Account { - /// The address of the account. + /// The mapped address. pub addr: S::Address, } @@ -79,7 +79,8 @@ impl std::str::FromStr for AccountOwnerKey { } } -/// A module responsible for managing accounts on the rollup. +/// A module responsible for resolving credentials to addresses and recording +/// credential authorizations. #[derive(Clone, ModuleInfo, ModuleRestApi)] #[cfg_attr(feature = "arbitrary", derive(Debug))] pub struct Accounts { @@ -92,7 +93,8 @@ pub struct Accounts { #[state] pub(crate) accounts: StateMap>, - /// If this field is false, `CallMessage::InsertCredentialId` messages will be rejected. + /// If this field is false, configured genesis authorizations and + /// `CallMessage::InsertCredentialId` messages will be rejected. #[state] enable_custom_account_mappings: StateValue, diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs index ed11a7ad0f..27833ff3f0 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/query.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/query.rs @@ -11,12 +11,12 @@ use crate::{Account, Accounts}; rename_all = "snake_case" )] pub enum Response { - /// The account corresponding to the given credential id exists. + /// A legacy/custom mapping for the given credential id exists. AccountExists { - /// The address of the account, + /// The mapped address. addr: Addr, }, - /// The account corresponding to the credential id does not exist. + /// No legacy/custom mapping for the credential id exists. AccountEmpty, } diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 7b7f59281d..5657875773 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -25,7 +25,8 @@ struct TestData { non_registered_account: TestUser, } -/// We setup genesis with three accounts, two of which are registered at genesis. +/// We set up genesis with three accounts, two of which have custom credentials +/// authorized at genesis. fn setup() -> (TestData, TestRunner) { let genesis_config = HighLevelOptimisticGenesisConfig::generate().add_accounts(vec![ TestUser::generate_with_default_balance().add_credential_id([0u8; 32].into()), @@ -73,8 +74,8 @@ fn test_config_account() { runner, ) = setup(); - // The account is authorized at genesis, but no new `accounts` map entry is - // written for canonical credentials. + // The credential/address pair is authorized at genesis, but no new + // `accounts` map entry is written for canonical credentials. runner.query_visible_state(|state| { let accounts = Accounts::::default(); assert!(accounts @@ -196,7 +197,7 @@ fn test_setup_multisig_and_act() { let multisig_credential_id = multisig.credential_id::<<::CryptoSpec as CryptoSpec>::Hasher>(); - // Build a funded `TestUser` whose address is the multisig's default. + // Build a funded `TestUser` whose address is the multisig's canonical address. let multisig_user = TestUser::generate_with_default_balance().add_credential_id(multisig_credential_id); @@ -386,7 +387,7 @@ fn test_register_new_account() { mut runner, ) = setup(); - // The account is empty at the start because it is not registered at genesis. + // The credential has no legacy/custom account-map entry at genesis. assert_eq!(non_registered_account.custom_credential_id, None); runner.query_visible_state(|state| { @@ -591,8 +592,8 @@ fn test_resolve_address_with_multi_credential_ownership() { }); } -/// Resolving an unknown credential returns the supplied fallback without -/// writing account state. +/// Resolving a credential with no legacy/custom mapping returns the supplied +/// fallback without writing account state. #[test] fn test_resolve_with_different_default_address() { let (TestData { account_1, .. }, runner) = setup(); From ae53c6e2a60674bfa2ee300677cb8cf762a9ec43 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Wed, 22 Apr 2026 11:41:00 +0200 Subject: [PATCH 06/21] Update json schemas --- .../module-schemas/genesis-schemas/sov-accounts.json | 10 +++++----- .../module-schemas/schemas/sov-accounts.json | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json index 867858f505..fec517182f 100644 --- a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json @@ -8,14 +8,14 @@ ], "properties": { "accounts": { - "description": "Accounts to initialize the rollup.", + "description": "Credential/address authorizations to initialize.", "type": "array", "items": { "$ref": "#/definitions/AccountData_for_Address" } }, "enable_custom_account_mappings": { - "description": "Enable custom `CredentialId` => `Account` mapping.", + "description": "Enable configured credential authorizations and `InsertCredentialId`.", "default": true, "type": "boolean" } @@ -23,7 +23,7 @@ "additionalProperties": false, "definitions": { "AccountData_for_Address": { - "description": "Account data for the genesis.", + "description": "Credential/address authorization data for genesis.", "type": "object", "required": [ "address", @@ -31,7 +31,7 @@ ], "properties": { "address": { - "description": "Address of the account.", + "description": "Address the credential may act as.", "allOf": [ { "$ref": "#/definitions/Address" @@ -39,7 +39,7 @@ ] }, "credential_id": { - "description": "Credential ID of the account.", + "description": "Credential ID to authorize.", "type": "string" } }, diff --git a/crates/module-system/module-schemas/schemas/sov-accounts.json b/crates/module-system/module-schemas/schemas/sov-accounts.json index b67f579a91..a4d990f3cf 100644 --- a/crates/module-system/module-schemas/schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/schemas/sov-accounts.json @@ -4,7 +4,7 @@ "description": "Represents the available call messages for interacting with the sov-accounts module.", "oneOf": [ { - "description": "Registers `credential_id` as an authorized signer whose transactions execute as the caller's address. Fails if the credential is already mapped to any address in this module (credentials are globally unique).", + "description": "Authorizes `credential_id` as a signer for the caller's address. Fails if the credential has a legacy/custom account mapping or is already authorized for the caller's address.", "type": "object", "required": [ "insert_credential_id" From 37efa417b2750530934c13d6f19a98be5810dabb Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Wed, 22 Apr 2026 14:23:45 +0200 Subject: [PATCH 07/21] PR feedback fixes part 1 --- .../tests/integration/registration.rs | 6 +++- .../stf_blueprint/operator/auth_eip712.rs | 36 ++++++++++--------- .../sov-accounts/src/call.rs | 10 +++--- .../sov-accounts/src/capabilities.rs | 11 +++--- 4 files changed, 37 insertions(+), 26 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index 11df7e9012..ec0463925e 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -23,13 +23,17 @@ fn test_user_is_registered_correctly() { let route_id = register_basic_warp_route(&mut runner, &admin); let payer = [1u8; 32]; + // When `embedded == payer`, the embedded credential's canonical address equals + // the payer address, so `is_authorized_for` succeeds via the canonical-address + // arm without needing any stored `accounts` or `account_owners` entry. This + // exercises the stateless happy path. let embedded = payer; let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); let credential = CredentialId::from(embedded); - // Sanity check, ensure registration does not rely on an accounts map entry. + // Sanity check: no legacy accounts entry is required for registration. runner.query_state(|state| { let account = sov_accounts::Accounts::default().get_account(credential, state); assert!(matches!(account, sov_accounts::Response::AccountEmpty)); diff --git a/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs b/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs index 452cfd7257..e8c7a369ed 100644 --- a/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs +++ b/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs @@ -1,4 +1,3 @@ -use sov_accounts::{Accounts, CallMessage as AccountsCallMessage}; use sov_address::{EthereumAddress, EvmCryptoSpec}; use sov_eip712_auth::{ Eip712Authenticator, Eip712AuthenticatorInput, Eip712AuthenticatorTrait, SchemaProvider, @@ -22,7 +21,6 @@ use sov_rollup_interface::da::RelevantBlobs; use sov_rollup_interface::stf::{TxEffect, TxReceiptContents}; use sov_test_utils::runtime::genesis::optimistic::HighLevelOptimisticGenesisConfig; use sov_test_utils::runtime::{TestRunner, ValueSetter}; -use sov_test_utils::TransactionTestCase; use sov_test_utils::{generate_runtime, EncodeCall, TestStorage, TestUser, TEST_DEFAULT_MAX_FEE}; use sov_value_setter::CallMessage; @@ -274,28 +272,34 @@ fn correct_signature_is_accepted() { #[test] fn test_multisig_signature_verification() { - use sov_test_utils::AsUser; - let (mut runner, admin) = setup(); - - // First, create and register a multisig + // Build the multisig first so we can seed genesis with its canonical address + // funded. After the accounts refactor, `resolve_sender_address` no longer + // writes an `accounts` entry, so the multisig must either be a canonical- + // address owner with gas, or be covered by a legacy mapping. Here we pick + // the canonical path. let multisig_keys = [ TestPrivateKey::generate(), TestPrivateKey::generate(), TestPrivateKey::generate(), ]; - - // Create the multisig and register it let multisig = Multisig::new(2, multisig_keys.iter().map(|k| k.pub_key()).collect()); let multisig_credential_id = multisig.credential_id::<<::CryptoSpec as CryptoSpec>::Hasher>(); - runner.execute_transaction(TransactionTestCase { - input: admin.create_plain_message::>( - AccountsCallMessage::InsertCredentialId(multisig_credential_id), - ), - assert: Box::new(move |result, _state| { - assert!(result.tx_receipt.is_successful()); - }), - }); + let multisig_user = + TestUser::generate_with_default_balance().add_credential_id(multisig_credential_id); + + // The multisig is the sender of every `SetValue` tx below, so ValueSetter's + // admin must be the multisig canonical address for the successful-path + // assertions to actually reach ValueSetter. + let multisig_address = multisig_user.address(); + let genesis_config = HighLevelOptimisticGenesisConfig::generate() + .add_accounts_with_default_balance(2) + .add_accounts(vec![multisig_user]); + let module_config = sov_value_setter::ValueSetterConfig { + admin: multisig_address, + }; + let genesis = GenesisConfig::from_minimal_config(genesis_config.clone().into(), module_config); + let mut runner = TestRunner::new_with_genesis(genesis.into_genesis_params(), RT::default()); // Generate a signature from a random private key that's not part of the multisig. We'll use this in some of the test cases. let random_private_key = TestPrivateKey::generate(); diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 076d8a40a1..7af211009d 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Context as _}; use schemars::JsonSchema; use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; @@ -26,7 +26,7 @@ impl Accounts { new_credential_id: CredentialId, context: &Context, state: &mut impl TxState, - ) -> Result<()> { + ) -> anyhow::Result<()> { if !self.enable_custom_account_mappings.get(state)?.expect( "`enable_custom_account_mappings` should not be None; it must be set at genesis.", ) { @@ -44,18 +44,18 @@ impl Accounts { new_credential_id: &CredentialId, address: &S::Address, state: &mut impl StateReader, - ) -> Result<()> { + ) -> anyhow::Result<()> { anyhow::ensure!( self.accounts .get(new_credential_id, state) - .map_err(|err| anyhow!("Error raised while getting account: {err:?}"))? + .context("Failed to read legacy account mapping")? .is_none(), "New CredentialId already exists" ); anyhow::ensure!( self.account_owners .get(&AccountOwnerKey::new(*address, *new_credential_id), state) - .map_err(|err| anyhow!("Error raised while getting account owner: {err:?}"))? + .context("Failed to read account owner")? .is_none(), "CredentialId already authorized for this address" ); diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index a964587945..30de1f7103 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -62,10 +62,13 @@ impl Accounts { .is_some()) } - /// Returns `true` if `credential_id` is authorized to act as `address` - /// under any supported relation: legacy/custom `accounts` mapping, - /// stateless canonical address, or explicit `account_owners` - /// authorization. + /// Returns `true` if `credential_id` is authorized to act as `address`. + /// + /// Precedence: if `credential_id` has a legacy mapping in `accounts`, that + /// mapping is authoritative and only the mapped address is considered + /// authorized. Otherwise, returns `true` if `address` is the canonical + /// address of `credential_id`, or if an explicit `account_owners` + /// authorization exists. pub fn is_authorized_for>( &self, address: &S::Address, From 06da3e19ff87994599d48c62ec31ea8694fec0b6 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 11:33:44 +0200 Subject: [PATCH 08/21] Revert and print receipt on error --- .../tests/integration/registration.rs | 48 +++++++++++++------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index ec0463925e..cfce148713 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -23,17 +23,13 @@ fn test_user_is_registered_correctly() { let route_id = register_basic_warp_route(&mut runner, &admin); let payer = [1u8; 32]; - // When `embedded == payer`, the embedded credential's canonical address equals - // the payer address, so `is_authorized_for` succeeds via the canonical-address - // arm without needing any stored `accounts` or `account_owners` entry. This - // exercises the stateless happy path. - let embedded = payer; + let embedded = [2u8; 32]; let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); let credential = CredentialId::from(embedded); - // Sanity check: no legacy accounts entry is required for registration. + // Sanity check, ensure account definently doesnt already exist runner.query_state(|state| { let account = sov_accounts::Accounts::default().get_account(credential, state); assert!(matches!(account, sov_accounts::Response::AccountEmpty)); @@ -45,9 +41,10 @@ fn test_user_is_registered_correctly() { message, }), assert: Box::new(move |result, _| { + let receipt = &result.tx_receipt; assert!( result.tx_receipt.is_successful(), - "Recipient was not registered successfully" + "Recipient was not registered successfully: {receipt:?}" ); assert_eq!( @@ -64,12 +61,15 @@ fn test_user_is_registered_correctly() { runner.query_state(|state| { let account = sov_accounts::Accounts::default().get_account(credential, state); - assert!(matches!(account, sov_accounts::Response::AccountEmpty)); + assert!(matches!( + account, + sov_accounts::Response::AccountExists { .. } + )); }); } #[test] -fn test_errors_if_embedded_pubkey_is_not_authorized_for_payer() { +fn test_errors_if_user_already_registered() { let SetupParams { mut runner, admin, @@ -83,11 +83,26 @@ fn test_errors_if_embedded_pubkey_is_not_authorized_for_payer() { let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); - let expected_error = format!( - "Embedded pubkey is not authorized for address. Address: {}, CredentialId: {}", - ::Address::from(payer), - CredentialId::from(embedded) - ); + + runner.execute_transaction(TransactionTestCase { + input: user.create_plain_message::>(CallMessage::Process { + metadata: HexString::new(SafeVec::new()), + message: message.clone(), + }), + assert: Box::new(move |result, _| { + assert!( + result.tx_receipt.is_successful(), + "Recipient was not registered successfully" + ); + }), + }); + + // payer is different so will try to register to different address + let payer = [3u8; 32]; + let embedded = [2u8; 32]; + let body = [payer, embedded].concat(); + let valid_message = make_valid_message(1, route_id, HexString::new(body)); + let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); runner.execute_transaction(TransactionTestCase { input: user.create_plain_message::>(CallMessage::Process { @@ -96,7 +111,10 @@ fn test_errors_if_embedded_pubkey_is_not_authorized_for_payer() { }), assert: Box::new(move |result, _| match result.tx_receipt { sov_rollup_interface::stf::TxEffect::Reverted(contents) => { - assert_eq!(contents.reason.to_string(), expected_error); + assert_eq!( + contents.reason.to_string(), + "Embedded pubkey already registered to different address. Attempted: CktRuQ2mttgRGkXJtyksdKHjUdc2C4TgDzyB98oEzy8, Registered: 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi".to_string() + ); } _ => panic!("Registration should have reverted: {:?}", result.tx_receipt), }), From 6f046546ddecce4100f180d2115e0f13a38c27ba Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 14:19:02 +0200 Subject: [PATCH 09/21] Explicitly authorize embedded credential in sov-hyperlane-solana-register As it was done before --- Cargo.toml | 1 - .../hyperlane-solana-register/README.md | 11 +- .../hyperlane-solana-register/src/lib.rs | 45 +++--- .../tests/integration/registration.rs | 138 +++++++++++++----- .../sov-accounts/src/capabilities.rs | 2 +- 5 files changed, 128 insertions(+), 69 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 19a82b690d..7dc7334ccd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -98,7 +98,6 @@ members = [ "crates/web3", "python/py_sovereign_web3/rust" , "crates/utils/sov-evm-test-utils"] -default-members = ["crates/rollup-interface", "crates/adapters/mock-da", "crates/adapters/mock-zkvm", "crates/full-node/sov-blob-sender", "crates/full-node/sov-db", "crates/full-node/sov-sequencer", "crates/full-node/sov-ledger-apis", "crates/full-node/sov-rollup-apis", "crates/full-node/sov-stf-runner", "crates/full-node/sov-metrics", "crates/full-node/sov-api-spec", "crates/full-node/full-node-configs", "crates/utils/sov-rest-utils", "crates/utils/nearly-linear", "crates/utils/sov-zkvm-utils", "crates/utils/sov-build", "crates/module-system/hyperlane", "crates/module-system/sov-cli", "crates/module-system/sov-modules-stf-blueprint", "crates/module-system/sov-modules-rollup-blueprint", "crates/module-system/sov-modules-macros", "crates/module-system/sov-kernels", "crates/module-system/sov-state", "crates/module-system/sov-modules-api", "crates/module-system/sov-address", "crates/utils/sov-test-utils", "crates/utils/sov-evm-test-utils", "crates/module-system/module-implementations/sov-accounts", "crates/module-system/module-implementations/sov-bank", "crates/module-system/module-implementations/sov-chain-state", "crates/module-system/module-implementations/sov-blob-storage", "crates/module-system/module-implementations/sov-evm", "crates/module-system/module-implementations/sov-paymaster", "crates/module-system/module-implementations/sov-prover-incentives", "crates/module-system/module-implementations/sov-attester-incentives", "crates/module-system/module-implementations/sov-sequencer-registry", "crates/module-system/module-implementations/sov-value-setter", "crates/module-system/module-implementations/sov-revenue-share", "crates/module-system/module-implementations/sov-synthetic-load", "crates/module-system/module-implementations/module-template", "crates/module-system/module-implementations/integration-tests", "crates/module-system/sov-capabilities", "crates/module-system/module-implementations/sov-uniqueness", "crates/utils/sov-node-client", "crates/universal-wallet/schema", "crates/universal-wallet/macros", "crates/universal-wallet/macro-helpers", "crates/utils/workspace-hack", "crates/web3", "python/py_sovereign_web3/rust", "crates/module-system/module-implementations/extern/hyperlane-solana-register"] [workspace.package] version = "0.3.0" edition = "2021" diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md index 2363bc92d0..7c22f4df6b 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md @@ -62,7 +62,6 @@ The `SolanaRegistration` module is a Sovereign SDK module that implements the - **Selective Message Handling**: Only processes messages from the configured Solana domain and trusted program ID - **Account Linking**: Associates Solana embedded wallets with rollup addresses via the `sov-accounts` module -- **Duplicate Prevention**: Rejects attempts to register an embedded wallet that's already linked to a different address - **Admin Controls**: Allows configuration updates via admin-only calls - **Fallback to Warp**: Non-Solana messages are forwarded to the underlying `Warp` module @@ -132,7 +131,6 @@ pub enum Event { The module defines several error types: -- `Unauthorized`: The embedded public key is not authorized for the payer address - `InvalidBodyLength`: The message body doesn't contain exactly 64 bytes - `ExtractPubKey`: Failed to parse public keys from the message body - `AdminNotFound`: Admin address not configured @@ -146,9 +144,8 @@ See `src/lib.rs:196-213` for the `handle` implementation: 2. Verify the sender matches the trusted Solana program ID 3. Extract the two 32-byte public keys from the message body 4. Convert the payer public key to a rollup address -5. Use `sov-accounts` to verify the embedded credential is authorized for the payer address -6. Reject if the embedded credential is not authorized for that address -7. Emit a `UserRegistered` event +5. Authorize the embedded credential to act as the payer's address by recording `(payer, embedded)` in `sov-accounts` +6. Emit a `UserRegistered` event --- @@ -353,6 +350,10 @@ Returns: 3. **ISM Verification**: The Sovereign module uses an ISM (Interchain Security Module) to verify message authenticity. Configure an appropriate ISM for your security requirements. +### Notes on the registration flow + +The current message format establishes the payer's intent on-chain. Down the line, additional verification on the embedded side may be considered to broaden the set of supported deployment scenarios; today's flow assumes the embedded key handling on the Solana program side is the source of truth for that direction. + --- ## Testing diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs index e43ed6afb7..22e0f7d7e1 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs @@ -53,11 +53,6 @@ where pub enum SolanaRegistrationError { #[error("Core module error: {0}")] CoreModuleError(#[from] CoreModuleError), - #[error("Embedded pubkey is not authorized for address. Address: {address}, CredentialId: {credential_id}")] - Unauthorized { - address: String, - credential_id: String, - }, #[error("Invalid body length. Expected {expected}, found {found}")] InvalidBodyLength { expected: usize, found: usize }, #[error("Failed to extract public key from body")] @@ -297,26 +292,26 @@ where let (user_pubkey, embedded_pubkey) = self.unpack_body(body.as_ref())?; let credential_id = CredentialId::from(embedded_pubkey); let address = S::Address::try_from(&user_pubkey).map_err(CoreModuleError::from)?; - let is_authorized = self - .accounts - .is_authorized_for(&address, &credential_id, state) - .map_err(CoreModuleError::state_read)?; - - if !is_authorized { - Err(SolanaRegistrationError::Unauthorized { - address: address.to_string(), - credential_id: credential_id.to_string(), - }) - } else { - self.emit_event( - state, - Event::UserRegistered { - address, - credential_id, - }, - ); - Ok(()) - } + + // The Solana message body is signed by `payer` only; the `embedded` + // half is unsigned material in this format. The authorization we + // record here is a many-to-many entry in `account_owners` that is + // unusable without the embedded credential's private key, so a + // payer who copies someone else's `embedded` only writes inert + // state. Tightening this further is a property of the upstream + // message format rather than this handler. + self.accounts + .authorize_credential(&address, &credential_id, state) + .map_err(CoreModuleError::state_write)?; + + self.emit_event( + state, + Event::UserRegistered { + address, + credential_id, + }, + ); + Ok(()) } pub fn admin( diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index cfce148713..231991a984 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -4,7 +4,10 @@ use sov_hyperlane_integration::{CallMessage, Ism, Recipient}; use sov_hyperlane_register_module::{ CallMessage as RegistrationCallMessage, SolanaDeployment, SolanaRegistration, }; -use sov_modules_api::{Base58Address, CredentialId, HexString, SafeVec, Spec}; +use sov_modules_api::prelude::UnwrapInfallible; +use sov_modules_api::{ + Base58Address, CredentialId, CryptoSpec, HexString, PrivateKey, PublicKey, SafeVec, Spec, +}; use sov_test_utils::{AsUser, TransactionTestCase}; use crate::setup::{ @@ -12,6 +15,20 @@ use crate::setup::{ SetupParams, TestRuntimeEvent, RT, S, SOLANA_PROGRAM_ID, }; +/// Deterministically derives an embedded credential from a fixed seed. +/// +/// Using a derived value (rather than an arbitrary `[u8; 32]`) gives us a +/// credential we can prove is ours and unlocks future tests that exercise +/// the embedded → payer signing path end-to-end once V1 `target_address` +/// (PR #2771) lands. +fn embedded_credential_from_seed(seed: [u8; 32]) -> ([u8; 32], CredentialId) { + let private_key = <::CryptoSpec as CryptoSpec>::PrivateKey::try_from(seed.to_vec()) + .expect("32-byte seed is a valid private key"); + let credential = private_key.pub_key().credential_id(); + let bytes = credential.0 .0; + (bytes, credential) +} + #[test] fn test_user_is_registered_correctly() { let SetupParams { @@ -23,16 +40,19 @@ fn test_user_is_registered_correctly() { let route_id = register_basic_warp_route(&mut runner, &admin); let payer = [1u8; 32]; - let embedded = [2u8; 32]; + let payer_addr = ::Address::from(payer); + let (embedded, credential) = embedded_credential_from_seed([42u8; 32]); let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); - let credential = CredentialId::from(embedded); - // Sanity check, ensure account definently doesnt already exist runner.query_state(|state| { - let account = sov_accounts::Accounts::default().get_account(credential, state); - assert!(matches!(account, sov_accounts::Response::AccountEmpty)); + assert!( + !sov_accounts::Accounts::::default() + .is_authorized(&payer_addr, &credential, state) + .unwrap_infallible(), + "Embedded credential should not yet be authorized for the payer address" + ); }); runner.execute_transaction(TransactionTestCase { @@ -51,8 +71,8 @@ fn test_user_is_registered_correctly() { result.events.last().unwrap(), &TestRuntimeEvent::SolanaRegister( sov_hyperlane_register_module::Event::UserRegistered { - address: ::Address::from(payer), - credential_id: CredentialId::from(embedded), + address: payer_addr, + credential_id: credential, } ) ); @@ -60,16 +80,23 @@ fn test_user_is_registered_correctly() { }); runner.query_state(|state| { - let account = sov_accounts::Accounts::default().get_account(credential, state); - assert!(matches!( - account, - sov_accounts::Response::AccountExists { .. } - )); + assert!( + sov_accounts::Accounts::::default() + .is_authorized(&payer_addr, &credential, state) + .unwrap_infallible(), + "Embedded credential should be authorized for the payer address after registration" + ); }); + + // TODO(follow-up): once V1 `target_address` lands (PR #2771), extend + // this test to also sign a V1 transaction with the embedded private + // key and `target_address = Some(payer_addr)`, asserting the rollup + // processes the tx as if from `payer_addr`. That covers the end-to-end + // embedded-wallet signing path this registration is enabling. } #[test] -fn test_errors_if_user_already_registered() { +fn test_two_payers_registering_same_embedded_both_succeed() { let SetupParams { mut runner, admin, @@ -78,47 +105,84 @@ fn test_errors_if_user_already_registered() { } = setup(); let route_id = register_basic_warp_route(&mut runner, &admin); - let payer = [1u8; 32]; - let embedded = [2u8; 32]; - let body = [payer, embedded].concat(); - let valid_message = make_valid_message(0, route_id, HexString::new(body)); - let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); + let payer_a = [1u8; 32]; + let payer_a_addr = ::Address::from(payer_a); + let payer_b = [3u8; 32]; + let payer_b_addr = ::Address::from(payer_b); + let (embedded, credential) = embedded_credential_from_seed([42u8; 32]); + // First registration: (payer_a, embedded). + let body_a = [payer_a, embedded].concat(); + let msg_a = make_valid_message(0, route_id, HexString::new(body_a)); + let envelope_a = HexString::new(SafeVec::try_from(msg_a.encode().0).unwrap()); runner.execute_transaction(TransactionTestCase { input: user.create_plain_message::>(CallMessage::Process { metadata: HexString::new(SafeVec::new()), - message: message.clone(), + message: envelope_a, }), assert: Box::new(move |result, _| { + let receipt = &result.tx_receipt; assert!( result.tx_receipt.is_successful(), - "Recipient was not registered successfully" + "First registration should succeed: {receipt:?}" ); }), }); - // payer is different so will try to register to different address - let payer = [3u8; 32]; - let embedded = [2u8; 32]; - let body = [payer, embedded].concat(); - let valid_message = make_valid_message(1, route_id, HexString::new(body)); - let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); - + // Second registration: SAME embedded but a DIFFERENT payer. Under the + // new `account_owners` model this is allowed — many-to-many + // authorization records both pairings without conflict. + // + // Why no exploit follows from accepting this second write: + // - `account_owners[(payer_b_addr, embedded)] = true` only declares + // that `embedded` MAY sign as `payer_b_addr`; it does not by + // itself reroute anyone's transactions. + // - The legitimate holder of the embedded private key remains the + // only entity that can produce a signature with that key. Without + // that signature the entry is inert state: it cannot move funds, + // change ownership of any pre-existing address, or redirect a + // victim's traffic. + // - The activation channel is V1 `target_address` (PR #2771), which + // requires the signer to actually sign with the credential — an + // attacker who only knows the public bytes cannot produce that + // signature. + // + // TODO(follow-up): a later change to the upstream message format will + // require the embedded side to participate in the bind, after which + // the second pairing here could not be created at all by an entity + // that doesn't hold the embedded private key. + let body_b = [payer_b, embedded].concat(); + let msg_b = make_valid_message(1, route_id, HexString::new(body_b)); + let envelope_b = HexString::new(SafeVec::try_from(msg_b.encode().0).unwrap()); runner.execute_transaction(TransactionTestCase { input: user.create_plain_message::>(CallMessage::Process { metadata: HexString::new(SafeVec::new()), - message, + message: envelope_b, }), - assert: Box::new(move |result, _| match result.tx_receipt { - sov_rollup_interface::stf::TxEffect::Reverted(contents) => { - assert_eq!( - contents.reason.to_string(), - "Embedded pubkey already registered to different address. Attempted: CktRuQ2mttgRGkXJtyksdKHjUdc2C4TgDzyB98oEzy8, Registered: 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi".to_string() - ); - } - _ => panic!("Registration should have reverted: {:?}", result.tx_receipt), + assert: Box::new(move |result, _| { + let receipt = &result.tx_receipt; + assert!( + result.tx_receipt.is_successful(), + "Second registration with same embedded but different payer should also succeed: {receipt:?}" + ); }), }); + + runner.query_state(|state| { + let accounts = sov_accounts::Accounts::::default(); + assert!( + accounts + .is_authorized(&payer_a_addr, &credential, state) + .unwrap_infallible(), + "First (payer_a, embedded) authorization should be recorded" + ); + assert!( + accounts + .is_authorized(&payer_b_addr, &credential, state) + .unwrap_infallible(), + "Second (payer_b, embedded) authorization should also be recorded" + ); + }); } #[test] diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 30de1f7103..e5b052a6de 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -5,7 +5,7 @@ use crate::{AccountOwnerKey, Accounts}; impl Accounts { /// Authorizes `credential_id` to sign as `address`. - pub(crate) fn authorize_credential>( + pub fn authorize_credential>( &mut self, address: &S::Address, credential_id: &CredentialId, From 3af9f589f8aa22766123bbc35ae4b391f8f262bc Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 14:32:05 +0200 Subject: [PATCH 10/21] Rollback fluff part 1 --- .../tests/integration/registration.rs | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index 231991a984..ca30ed515c 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -5,9 +5,7 @@ use sov_hyperlane_register_module::{ CallMessage as RegistrationCallMessage, SolanaDeployment, SolanaRegistration, }; use sov_modules_api::prelude::UnwrapInfallible; -use sov_modules_api::{ - Base58Address, CredentialId, CryptoSpec, HexString, PrivateKey, PublicKey, SafeVec, Spec, -}; +use sov_modules_api::{Base58Address, CredentialId, HexString, SafeVec, Spec}; use sov_test_utils::{AsUser, TransactionTestCase}; use crate::setup::{ @@ -15,20 +13,6 @@ use crate::setup::{ SetupParams, TestRuntimeEvent, RT, S, SOLANA_PROGRAM_ID, }; -/// Deterministically derives an embedded credential from a fixed seed. -/// -/// Using a derived value (rather than an arbitrary `[u8; 32]`) gives us a -/// credential we can prove is ours and unlocks future tests that exercise -/// the embedded → payer signing path end-to-end once V1 `target_address` -/// (PR #2771) lands. -fn embedded_credential_from_seed(seed: [u8; 32]) -> ([u8; 32], CredentialId) { - let private_key = <::CryptoSpec as CryptoSpec>::PrivateKey::try_from(seed.to_vec()) - .expect("32-byte seed is a valid private key"); - let credential = private_key.pub_key().credential_id(); - let bytes = credential.0 .0; - (bytes, credential) -} - #[test] fn test_user_is_registered_correctly() { let SetupParams { @@ -41,7 +25,8 @@ fn test_user_is_registered_correctly() { let payer = [1u8; 32]; let payer_addr = ::Address::from(payer); - let (embedded, credential) = embedded_credential_from_seed([42u8; 32]); + let embedded = [2u8; 32]; + let credential = CredentialId::from(embedded); let body = [payer, embedded].concat(); let valid_message = make_valid_message(0, route_id, HexString::new(body)); let message = HexString::new(SafeVec::try_from(valid_message.encode().0).unwrap()); @@ -109,7 +94,8 @@ fn test_two_payers_registering_same_embedded_both_succeed() { let payer_a_addr = ::Address::from(payer_a); let payer_b = [3u8; 32]; let payer_b_addr = ::Address::from(payer_b); - let (embedded, credential) = embedded_credential_from_seed([42u8; 32]); + let embedded = [2u8; 32]; + let credential = CredentialId::from(embedded); // First registration: (payer_a, embedded). let body_a = [payer_a, embedded].concat(); From 60cc62e3e70ddc9468a1b04e6beab16a0165d1f7 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 14:33:00 +0200 Subject: [PATCH 11/21] Revert accidental switcheroo --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index 7dc7334ccd..19a82b690d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -98,6 +98,7 @@ members = [ "crates/web3", "python/py_sovereign_web3/rust" , "crates/utils/sov-evm-test-utils"] +default-members = ["crates/rollup-interface", "crates/adapters/mock-da", "crates/adapters/mock-zkvm", "crates/full-node/sov-blob-sender", "crates/full-node/sov-db", "crates/full-node/sov-sequencer", "crates/full-node/sov-ledger-apis", "crates/full-node/sov-rollup-apis", "crates/full-node/sov-stf-runner", "crates/full-node/sov-metrics", "crates/full-node/sov-api-spec", "crates/full-node/full-node-configs", "crates/utils/sov-rest-utils", "crates/utils/nearly-linear", "crates/utils/sov-zkvm-utils", "crates/utils/sov-build", "crates/module-system/hyperlane", "crates/module-system/sov-cli", "crates/module-system/sov-modules-stf-blueprint", "crates/module-system/sov-modules-rollup-blueprint", "crates/module-system/sov-modules-macros", "crates/module-system/sov-kernels", "crates/module-system/sov-state", "crates/module-system/sov-modules-api", "crates/module-system/sov-address", "crates/utils/sov-test-utils", "crates/utils/sov-evm-test-utils", "crates/module-system/module-implementations/sov-accounts", "crates/module-system/module-implementations/sov-bank", "crates/module-system/module-implementations/sov-chain-state", "crates/module-system/module-implementations/sov-blob-storage", "crates/module-system/module-implementations/sov-evm", "crates/module-system/module-implementations/sov-paymaster", "crates/module-system/module-implementations/sov-prover-incentives", "crates/module-system/module-implementations/sov-attester-incentives", "crates/module-system/module-implementations/sov-sequencer-registry", "crates/module-system/module-implementations/sov-value-setter", "crates/module-system/module-implementations/sov-revenue-share", "crates/module-system/module-implementations/sov-synthetic-load", "crates/module-system/module-implementations/module-template", "crates/module-system/module-implementations/integration-tests", "crates/module-system/sov-capabilities", "crates/module-system/module-implementations/sov-uniqueness", "crates/utils/sov-node-client", "crates/universal-wallet/schema", "crates/universal-wallet/macros", "crates/universal-wallet/macro-helpers", "crates/utils/workspace-hack", "crates/web3", "python/py_sovereign_web3/rust", "crates/module-system/module-implementations/extern/hyperlane-solana-register"] [workspace.package] version = "0.3.0" edition = "2021" From e3bb2c9eef46836f010813af0dc2a60fc8e54b55 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 14:53:01 +0200 Subject: [PATCH 12/21] Clean up --- .../hyperlane-solana-register/README.md | 4 --- .../hyperlane-solana-register/src/lib.rs | 7 ---- .../tests/integration/registration.rs | 29 +-------------- .../sov-accounts/src/capabilities.rs | 4 +-- .../sov-accounts/tests/integration/main.rs | 35 +++---------------- 5 files changed, 7 insertions(+), 72 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md index 7c22f4df6b..f1d2f7551a 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/README.md @@ -350,10 +350,6 @@ Returns: 3. **ISM Verification**: The Sovereign module uses an ISM (Interchain Security Module) to verify message authenticity. Configure an appropriate ISM for your security requirements. -### Notes on the registration flow - -The current message format establishes the payer's intent on-chain. Down the line, additional verification on the embedded side may be considered to broaden the set of supported deployment scenarios; today's flow assumes the embedded key handling on the Solana program side is the source of truth for that direction. - --- ## Testing diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs index 22e0f7d7e1..a37a6b8445 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/src/lib.rs @@ -293,13 +293,6 @@ where let credential_id = CredentialId::from(embedded_pubkey); let address = S::Address::try_from(&user_pubkey).map_err(CoreModuleError::from)?; - // The Solana message body is signed by `payer` only; the `embedded` - // half is unsigned material in this format. The authorization we - // record here is a many-to-many entry in `account_owners` that is - // unusable without the embedded credential's private key, so a - // payer who copies someone else's `embedded` only writes inert - // state. Tightening this further is a property of the upstream - // message format rather than this handler. self.accounts .authorize_credential(&address, &credential_id, state) .map_err(CoreModuleError::state_write)?; diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index ca30ed515c..0baa89fe7f 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -72,12 +72,6 @@ fn test_user_is_registered_correctly() { "Embedded credential should be authorized for the payer address after registration" ); }); - - // TODO(follow-up): once V1 `target_address` lands (PR #2771), extend - // this test to also sign a V1 transaction with the embedded private - // key and `target_address = Some(payer_addr)`, asserting the rollup - // processes the tx as if from `payer_addr`. That covers the end-to-end - // embedded-wallet signing path this registration is enabling. } #[test] @@ -115,28 +109,7 @@ fn test_two_payers_registering_same_embedded_both_succeed() { }), }); - // Second registration: SAME embedded but a DIFFERENT payer. Under the - // new `account_owners` model this is allowed — many-to-many - // authorization records both pairings without conflict. - // - // Why no exploit follows from accepting this second write: - // - `account_owners[(payer_b_addr, embedded)] = true` only declares - // that `embedded` MAY sign as `payer_b_addr`; it does not by - // itself reroute anyone's transactions. - // - The legitimate holder of the embedded private key remains the - // only entity that can produce a signature with that key. Without - // that signature the entry is inert state: it cannot move funds, - // change ownership of any pre-existing address, or redirect a - // victim's traffic. - // - The activation channel is V1 `target_address` (PR #2771), which - // requires the signer to actually sign with the credential — an - // attacker who only knows the public bytes cannot produce that - // signature. - // - // TODO(follow-up): a later change to the upstream message format will - // require the embedded side to participate in the bind, after which - // the second pairing here could not be created at all by an entity - // that doesn't hold the embedded private key. + // Same embedded credential, different payer — many-to-many authorization is allowed. let body_b = [payer_b, embedded].concat(); let msg_b = make_valid_message(1, route_id, HexString::new(body_b)); let envelope_b = HexString::new(SafeVec::try_from(msg_b.encode().0).unwrap()); diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index e5b052a6de..90e7a05e57 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -33,9 +33,7 @@ impl Accounts { Ok(*default_address) } - /// Read-only variant of [`Self::resolve_sender_address`]: returns - /// `default_address` when the credential has no legacy/custom mapping, - /// without writing. + /// Read-only variant of [`Self::resolve_sender_address`]. pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 5657875773..aaaa4e9124 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -74,20 +74,11 @@ fn test_config_account() { runner, ) = setup(); - // The credential/address pair is authorized at genesis, but no new - // `accounts` map entry is written for canonical credentials. + // The account is registered at genesis. runner.query_visible_state(|state| { let accounts = Accounts::::default(); - assert!(accounts - .is_authorized(&user.address(), &user.credential_id(), state) - .unwrap()); - assert!(accounts - .is_authorized_for(&user.address(), &user.credential_id(), state) - .unwrap()); - assert_eq!( - accounts.get_account(user.credential_id(), state), - Response::AccountEmpty - ); + let response = accounts.get_account(user.credential_id(), state); + assert_eq!(response, Response::AccountEmpty); }); } @@ -112,25 +103,12 @@ fn test_update_account() { let accounts = Accounts::::default(); - // The new credential is authorized to spend as the sender's address. - assert!(accounts - .is_authorized(&user.address(), &new_credential, state) - .unwrap()); - assert!(accounts - .is_authorized_for(&user.address(), &new_credential, state) - .unwrap()); + // New credential has no `accounts` map entry under the new model. assert_eq!( accounts.get_account(new_credential, state), Response::AccountEmpty ); - // The sender's own credential uses stateless canonical ownership; - // resolving the tx no longer writes account authorization state. - assert!(!accounts - .is_authorized(&user.address(), &user.credential_id(), state) - .unwrap()); - assert!(accounts - .is_authorized_for(&user.address(), &user.credential_id(), state) - .unwrap()); + // Sender's own credential also has no `accounts` map entry. assert_eq!( accounts.get_account(user.credential_id(), state), Response::AccountEmpty @@ -423,7 +401,6 @@ fn test_register_new_account() { let accounts = Accounts::::default(); - // The new credential is authorized for the sender's address. assert!(accounts .is_authorized(&non_registered_account.address(), &new_credential, state) .unwrap()); @@ -435,8 +412,6 @@ fn test_register_new_account() { Response::AccountEmpty ); - // The sender's own credential uses stateless canonical ownership; - // resolving the tx no longer writes account authorization state. assert!(!accounts .is_authorized( &non_registered_account.address(), From 8aad53e844c662b2fdcb8a885c363317d8992a35 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 15:05:14 +0200 Subject: [PATCH 13/21] Reformat readme for readability --- .../sov-accounts/README.md | 60 +++++++++---------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index c5e4f66632..6f54511a17 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -5,25 +5,23 @@ addresses and records which credentials may act for which addresses. ### The `sov-accounts` module offers the following functionality -1. A credential has a deterministic canonical address, computed as - `credential_id.into::()`. This relation is stateless: using the - canonical address does not require an account entry to be written. +1. A credential has a deterministic canonical address, computed as `credential_id.into::()`. + This relation is stateless: using the canonical address does not require an account entry to be written. -1. Legacy state can contain an explicit `credential_id -> address` mapping. +2. Legacy state can contain an explicit `credential_id -> address` mapping. This map is used for credential-only resolution and the `get_account` query. -1. It is possible to authorize another credential for the caller's address using - the `CallMessage::InsertCredentialId(..)` message. This writes an - `account_owners` authorization, not a credential-indexed account entry. +3. It is possible to authorize another credential for the caller's address using + the `CallMessage::InsertCredentialId(..)` message. + This writes an `account_owners` authorization, not a credential-indexed account entry. -1. It is possible to query the `sov-accounts` module using the `get_account` - method and get the legacy/custom mapped address corresponding to the given - credential id. +4. It is possible to query the `sov-accounts` module using the `get_account` + method and get the legacy/custom mapped address corresponding to the given credential id. ## Credential and Address Relations -The module has three credential/address relations. They answer different -questions and should not be treated as interchangeable. +The module has three credential/address relations. +They answer different questions and should not be treated as interchangeable. ### Stateless canonical address @@ -31,10 +29,9 @@ questions and should not be treated as interchangeable. credential_id -> credential_id.into::() ``` -This is the default address for a credential. It is deterministic and requires -no state write. If a credential has no explicit credential-indexed state -mapping, this canonical address is the natural fallback for credential-only -routing. +This is the default address for a credential. It is deterministic and requires no state write. +If a credential has no explicit credential-indexed state mapping, +this canonical address is the natural fallback for credential-only routing. ### Credential-to-account map @@ -42,16 +39,15 @@ routing. accounts[credential_id] = Account { addr } ``` -This legacy/custom state map records the primary address explicitly associated -with a credential. A credential has at most one primary address in this map, +This legacy/custom state map records the primary address explicitly associated with a credential. +A credential has at most one primary address in this map, while one address may be the primary address for many credentials. This map is used when callers only know a `CredentialId` and need an address: -`resolve_sender_address`, `get_account`, and legacy/custom account mappings all -depend on this credential-indexed lookup. New `InsertCredentialId` calls do not -write this map. `get_account` reports entries from this explicit map; it does -not mean that every possible stateless canonical address or explicit -authorization has a stored account entry. +`resolve_sender_address`, `get_account`, and legacy/custom account mappings all depend on this credential-indexed lookup. +New `InsertCredentialId` calls do not write this map. +`get_account` reports entries from this explicit map; +it does not mean that every possible stateless canonical address or explicit authorization has a stored account entry. ### Account-credential authorization map @@ -59,19 +55,17 @@ authorization has a stored account entry. account_owners[(address, credential_id)] = true ``` -This state map records authorization. A present entry means the credential is -authorized to sign transactions that execute as the given address. The key is -the exact `(address, credential_id)` pair, so this relation does not provide a -credential-only lookup by itself. +This state map records authorization. +A present entry means the credential is authorized to sign transactions that execute as the given address. +The key is the exact `(address, credential_id)` pair, so this relation does not provide a credential-only lookup by itself. -This relation answers "may this credential act as this address?" once the target -address is known. It is not a replacement for the credential-to-account map, +This relation answers "may this credential act as this address?" once the target address is known. +It is not a replacement for the credential-to-account map, because it is keyed by `(address, credential_id)` and cannot efficiently answer "which address is this credential mapped to?" with a single point lookup. New `InsertCredentialId` calls write this relation. In short: routing from only a credential uses the explicit credential-to-account -map or the stateless canonical fallback. Callers that need to verify whether a -known address may be used with a credential should use the combined -`is_authorized_for` check: legacy/custom mapping, stateless canonical address, -or `account_owners`. +map or the stateless canonical fallback. +Callers that need to verify whether a known address may be used with a credential should use the combined +`is_authorized_for` check: legacy/custom mapping, stateless canonical address, or `account_owners`. From 83d4df0b3da150220cbc9d7c8675aa2cadbc1ec4 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 15:18:05 +0200 Subject: [PATCH 14/21] More info in genesis bail --- .../module-implementations/sov-accounts/src/genesis.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index 5d18e27beb..e4f4d6e409 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -53,7 +53,11 @@ impl Accounts { for acc in &config.accounts { let key = AccountOwnerKey::new(acc.address, acc.credential_id); if self.account_owners.get(&key, state)?.is_some() { - bail!("Account already exists") + bail!( + "Authorization already exists for address {} and credential {}", + acc.address, + acc.credential_id + ) } self.authorize_credential(&acc.address, &acc.credential_id, state)?; } From 91d0e3e04c4af11d2b4cbf42c0325b878fa979e3 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Mon, 4 May 2026 15:43:12 +0200 Subject: [PATCH 15/21] Simplify part n --- .../tests/integration/registration.rs | 8 ++++---- .../sov-accounts/src/capabilities.rs | 9 +++++---- .../sov-accounts/src/lib.rs | 9 +++------ .../sov-accounts/tests/integration/main.rs | 18 +++++++++++------- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs index 0baa89fe7f..d155be963f 100644 --- a/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs +++ b/crates/module-system/module-implementations/extern/hyperlane-solana-register/tests/integration/registration.rs @@ -34,7 +34,7 @@ fn test_user_is_registered_correctly() { runner.query_state(|state| { assert!( !sov_accounts::Accounts::::default() - .is_authorized(&payer_addr, &credential, state) + .is_explicitly_authorized(&payer_addr, &credential, state) .unwrap_infallible(), "Embedded credential should not yet be authorized for the payer address" ); @@ -67,7 +67,7 @@ fn test_user_is_registered_correctly() { runner.query_state(|state| { assert!( sov_accounts::Accounts::::default() - .is_authorized(&payer_addr, &credential, state) + .is_explicitly_authorized(&payer_addr, &credential, state) .unwrap_infallible(), "Embedded credential should be authorized for the payer address after registration" ); @@ -131,13 +131,13 @@ fn test_two_payers_registering_same_embedded_both_succeed() { let accounts = sov_accounts::Accounts::::default(); assert!( accounts - .is_authorized(&payer_a_addr, &credential, state) + .is_explicitly_authorized(&payer_a_addr, &credential, state) .unwrap_infallible(), "First (payer_a, embedded) authorization should be recorded" ); assert!( accounts - .is_authorized(&payer_b_addr, &credential, state) + .is_explicitly_authorized(&payer_b_addr, &credential, state) .unwrap_infallible(), "Second (payer_b, embedded) authorization should also be recorded" ); diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 90e7a05e57..184553d684 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -46,9 +46,10 @@ impl Accounts { Ok(*default_address) } - /// Returns `true` if `credential_id` has an explicit `account_owners` - /// authorization for `address`. - pub fn is_authorized>( + /// Returns `true` only if `(address, credential_id)` has an explicit entry + /// in `account_owners`. For the full authorization check including + /// legacy and canonical fallback, use [`Self::is_authorized_for`]. + pub fn is_explicitly_authorized>( &self, address: &S::Address, credential_id: &CredentialId, @@ -82,6 +83,6 @@ impl Accounts { return Ok(true); } - self.is_authorized(address, credential_id, state) + self.is_explicitly_authorized(address, credential_id, state) } } diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 247ff3e718..459cd20f18 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -69,12 +69,9 @@ impl std::str::FromStr for AccountOwnerKey { .rsplit_once('/') .ok_or_else(|| anyhow::anyhow!("invalid AccountOwnerKey: missing '/' separator"))?; Ok(Self { - address: addr_str - .parse() - .map_err(|e| anyhow::anyhow!("invalid address in AccountOwnerKey: {e:?}"))?, - credential_id: cred_str - .parse() - .map_err(|e| anyhow::anyhow!("invalid credential_id in AccountOwnerKey: {e:?}"))?, + address: ::from_str(addr_str) + .map_err(|e| anyhow::Error::from_boxed(e.into()))?, + credential_id: cred_str.parse()?, }) } } diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index aaaa4e9124..326e0acc73 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -375,7 +375,7 @@ fn test_register_new_account() { Response::AccountEmpty ); assert!(!accounts - .is_authorized( + .is_explicitly_authorized( &non_registered_account.address(), &non_registered_account.credential_id(), state @@ -402,7 +402,7 @@ fn test_register_new_account() { let accounts = Accounts::::default(); assert!(accounts - .is_authorized(&non_registered_account.address(), &new_credential, state) + .is_explicitly_authorized(&non_registered_account.address(), &new_credential, state) .unwrap()); assert!(accounts .is_authorized_for(&non_registered_account.address(), &new_credential, state) @@ -413,7 +413,7 @@ fn test_register_new_account() { ); assert!(!accounts - .is_authorized( + .is_explicitly_authorized( &non_registered_account.address(), &non_registered_account.credential_id(), state @@ -459,7 +459,7 @@ fn test_resolve_sender_address_with_default_address_non_registered() { non_registered_account.address() ); assert!(!accounts - .is_authorized( + .is_explicitly_authorized( &non_registered_account.address(), &non_registered_account.credential_id(), state @@ -548,8 +548,12 @@ fn test_resolve_address_with_multi_credential_ownership() { addr ); - assert!(accounts.is_authorized(&addr, &credential_1, state).unwrap()); - assert!(accounts.is_authorized(&addr, &credential_2, state).unwrap()); + assert!(accounts + .is_explicitly_authorized(&addr, &credential_1, state) + .unwrap()); + assert!(accounts + .is_explicitly_authorized(&addr, &credential_2, state) + .unwrap()); assert!(accounts .is_authorized_for(&addr, &credential_1, state) .unwrap()); @@ -585,7 +589,7 @@ fn test_resolve_with_different_default_address() { account_1.address() ); assert!(!accounts - .is_authorized(&account_1.address(), &random_credential, state) + .is_explicitly_authorized(&account_1.address(), &random_credential, state) .unwrap()); assert_eq!( accounts.get_account(random_credential, state), From c87eba4782c755d5a405d13a147997d66751a8b9 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 5 May 2026 11:19:00 +0200 Subject: [PATCH 16/21] Initial migrations implementation --- .../sov-accounts/README.md | 87 ++++--- .../sov-accounts/src/call.rs | 10 +- .../sov-accounts/src/capabilities.rs | 43 ++- .../sov-accounts/src/lib.rs | 16 +- .../sov-accounts/src/migrations.rs | 224 ++++++++++++++++ .../sov-accounts/src/query.rs | 35 --- .../sov-accounts/src/tests.rs | 48 ---- .../sov-accounts/tests/integration/main.rs | 54 +--- .../module-schemas/schemas/sov-accounts.json | 2 +- examples/demo-rollup/Cargo.toml | 7 + .../src/migrations/legacy_accounts.rs | 244 ++++++++++++++++++ 11 files changed, 564 insertions(+), 206 deletions(-) create mode 100644 crates/module-system/module-implementations/sov-accounts/src/migrations.rs delete mode 100644 crates/module-system/module-implementations/sov-accounts/src/query.rs create mode 100644 examples/demo-rollup/src/migrations/legacy_accounts.rs diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index 6f54511a17..b7d5c6d6c7 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -5,23 +5,17 @@ addresses and records which credentials may act for which addresses. ### The `sov-accounts` module offers the following functionality -1. A credential has a deterministic canonical address, computed as `credential_id.into::()`. +1. A credential has a deterministic canonical address, computed as `credential_id.into::()`. This relation is stateless: using the canonical address does not require an account entry to be written. -2. Legacy state can contain an explicit `credential_id -> address` mapping. - This map is used for credential-only resolution and the `get_account` query. - -3. It is possible to authorize another credential for the caller's address using - the `CallMessage::InsertCredentialId(..)` message. - This writes an `account_owners` authorization, not a credential-indexed account entry. - -4. It is possible to query the `sov-accounts` module using the `get_account` - method and get the legacy/custom mapped address corresponding to the given credential id. +2. It is possible to authorize another credential for the caller's address using + the `CallMessage::InsertCredentialId(..)` message. + This writes an `account_owners` authorization. ## Credential and Address Relations -The module has three credential/address relations. -They answer different questions and should not be treated as interchangeable. +The module has two credential/address relations. They answer different +questions and should not be treated as interchangeable. ### Stateless canonical address @@ -30,24 +24,7 @@ credential_id -> credential_id.into::() ``` This is the default address for a credential. It is deterministic and requires no state write. -If a credential has no explicit credential-indexed state mapping, -this canonical address is the natural fallback for credential-only routing. - -### Credential-to-account map - -```text -accounts[credential_id] = Account { addr } -``` - -This legacy/custom state map records the primary address explicitly associated with a credential. -A credential has at most one primary address in this map, -while one address may be the primary address for many credentials. - -This map is used when callers only know a `CredentialId` and need an address: -`resolve_sender_address`, `get_account`, and legacy/custom account mappings all depend on this credential-indexed lookup. -New `InsertCredentialId` calls do not write this map. -`get_account` reports entries from this explicit map; -it does not mean that every possible stateless canonical address or explicit authorization has a stored account entry. +If a credential has no explicit authorization, this canonical address is the natural fallback. ### Account-credential authorization map @@ -55,17 +32,51 @@ it does not mean that every possible stateless canonical address or explicit aut account_owners[(address, credential_id)] = true ``` -This state map records authorization. +This state map records authorization. A present entry means the credential is authorized to sign transactions that execute as the given address. The key is the exact `(address, credential_id)` pair, so this relation does not provide a credential-only lookup by itself. This relation answers "may this credential act as this address?" once the target address is known. -It is not a replacement for the credential-to-account map, -because it is keyed by `(address, credential_id)` and cannot efficiently answer -"which address is this credential mapped to?" with a single point lookup. New `InsertCredentialId` calls write this relation. -In short: routing from only a credential uses the explicit credential-to-account -map or the stateless canonical fallback. -Callers that need to verify whether a known address may be used with a credential should use the combined -`is_authorized_for` check: legacy/custom mapping, stateless canonical address, or `account_owners`. +Callers that need to verify whether a known address may be used with a credential should use +`is_authorized_for`, which checks the stateless canonical address and `account_owners`. + +## Legacy `accounts` map + +The module struct retains a tombstoned `accounts: StateMap` field +purely to preserve the macro-derived `#[state]` discriminant ordering — it is the first +state field, so removing it would shift the discriminants of every following field and +corrupt their on-disk data. The field is `pub(crate)`, never read or written outside +[`migrations`](src/migrations.rs), and empty after the legacy-accounts migration runs. + +## Upgrade procedure for chains with legacy `accounts` entries + +Chains created before the layer-1 reads were dropped may have entries in `accounts`. +Those entries need to be moved to `account_owners` before deploying the new binary, +otherwise the credentials they encode will silently lose their authorization. + +The migration ships as a CLI binary in `examples/demo-rollup`: + +```sh +# Inspect what would change without committing. +cargo run --features migration-script \ + --bin legacy-accounts-migrate -- \ + --rollup-config-path /path/to/rollup_config.toml \ + --dry-run + +# Commit the migration in-place at the current head version. +cargo run --features migration-script \ + --bin legacy-accounts-migrate -- \ + --rollup-config-path /path/to/rollup_config.toml +``` + +The binary requires a stopped node (the storage manager opens the DB exclusively). +The reported `pre_state_root` and `post_state_root` are written in JSON; verify the +post-root matches what the rollup loads on restart. + +The migration requires NOMT prefix iteration; JMT-backed deployments are not supported. + +For non-demo rollups, copy `examples/demo-rollup/src/migrations/legacy_accounts.rs` +and swap in your own runtime/spec types — the migration logic itself lives in +`sov_accounts::migrations` and is reusable. diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 7af211009d..7a731cd5b5 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -12,8 +12,7 @@ use crate::{AccountOwnerKey, Accounts}; #[serde(rename_all = "snake_case")] pub enum CallMessage { /// Authorizes `credential_id` as a signer for the caller's address. - /// Fails if the credential has a legacy/custom account mapping or is - /// already authorized for the caller's address. + /// Fails if the credential is already authorized for the caller's address. InsertCredentialId( /// The credential id being authorized. CredentialId, @@ -45,13 +44,6 @@ impl Accounts { address: &S::Address, state: &mut impl StateReader, ) -> anyhow::Result<()> { - anyhow::ensure!( - self.accounts - .get(new_credential_id, state) - .context("Failed to read legacy account mapping")? - .is_none(), - "New CredentialId already exists" - ); anyhow::ensure!( self.account_owners .get(&AccountOwnerKey::new(*address, *new_credential_id), state) diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 184553d684..71ff90e0ac 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -1,4 +1,4 @@ -use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, StateWriter}; +use sov_modules_api::{CredentialId, Spec, StateReader, StateWriter}; use sov_state::User; use crate::{AccountOwnerKey, Accounts}; @@ -19,17 +19,17 @@ impl Accounts { } /// Resolve the sender's credential to an address. - /// If `credential_id` has a legacy/custom mapping in `accounts`, return it. - /// Otherwise, return the supplied `default_address` without writing account state. - pub fn resolve_sender_address( + /// + /// Returns `default_address` unconditionally. The legacy `accounts` map is + /// no longer consulted; operators must run the legacy-accounts migration + /// (see [`crate::migrations`]) before deploying a binary that includes + /// this code on a chain with pre-upgrade entries. + pub fn resolve_sender_address>( &mut self, default_address: &S::Address, - credential_id: &CredentialId, - state: &mut ST, - ) -> Result>::Error> { - if let Some(account) = self.accounts.get(credential_id, state)? { - return Ok(account.addr); - } + _credential_id: &CredentialId, + _state: &mut ST, + ) -> Result { Ok(*default_address) } @@ -37,18 +37,15 @@ impl Accounts { pub fn resolve_sender_address_read_only>( &self, default_address: &S::Address, - credential_id: &CredentialId, - state: &mut ST, + _credential_id: &CredentialId, + _state: &mut ST, ) -> Result { - if let Some(account) = self.accounts.get(credential_id, state)? { - return Ok(account.addr); - } Ok(*default_address) } /// Returns `true` only if `(address, credential_id)` has an explicit entry - /// in `account_owners`. For the full authorization check including - /// legacy and canonical fallback, use [`Self::is_authorized_for`]. + /// in `account_owners`. For the full authorization check including the + /// canonical fallback, use [`Self::is_authorized_for`]. pub fn is_explicitly_authorized>( &self, address: &S::Address, @@ -63,21 +60,15 @@ impl Accounts { /// Returns `true` if `credential_id` is authorized to act as `address`. /// - /// Precedence: if `credential_id` has a legacy mapping in `accounts`, that - /// mapping is authoritative and only the mapped address is considered - /// authorized. Otherwise, returns `true` if `address` is the canonical - /// address of `credential_id`, or if an explicit `account_owners` - /// authorization exists. + /// Returns `true` when `address` is the canonical address of + /// `credential_id` (i.e. `credential_id.into() == address`) or when an + /// explicit `account_owners` authorization exists. pub fn is_authorized_for>( &self, address: &S::Address, credential_id: &CredentialId, state: &mut ST, ) -> Result { - if let Some(account) = self.accounts.get(credential_id, state)? { - return Ok(account.addr == *address); - } - let canonical_address: S::Address = (*credential_id).into(); if canonical_address == *address { return Ok(true); diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 459cd20f18..51e1dc83fd 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -7,9 +7,7 @@ mod fuzz; mod genesis; pub use genesis::*; #[cfg(feature = "native")] -mod query; -#[cfg(feature = "native")] -pub use query::*; +pub mod migrations; #[cfg(test)] mod tests; pub use call::CallMessage; @@ -85,8 +83,16 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// Legacy/custom `credential_id -> address` routing index. New - /// authorization writes use [`Self::account_owners`] instead. + /// Tombstone for the legacy `credential_id -> address` routing index. + /// + /// **Do not read or write outside [`crate::migrations`].** The field is + /// retained only to preserve the `#[state]` field discriminant ordering + /// derived by the `ModuleInfo` macro (this is the first state field, so + /// removing it would shift the discriminants of every following field + /// and corrupt their on-disk data). Existing entries are migrated to + /// [`Self::account_owners`] by + /// [`crate::migrations::migrate_legacy_accounts_to_owners`] and the + /// source rows are deleted. #[state] pub(crate) accounts: StateMap>, diff --git a/crates/module-system/module-implementations/sov-accounts/src/migrations.rs b/crates/module-system/module-implementations/sov-accounts/src/migrations.rs new file mode 100644 index 0000000000..a16031347b --- /dev/null +++ b/crates/module-system/module-implementations/sov-accounts/src/migrations.rs @@ -0,0 +1,224 @@ +//! One-time data migration from the legacy +//! [`Accounts::accounts`](crate::Accounts::accounts) credential→account index +//! to the [`Accounts::account_owners`](crate::Accounts::account_owners) +//! authorization set. +//! +//! Offline only: requires backend prefix iteration via +//! [`NativeStorage::maybe_iter_user_values_with_prefix`], which is supported by +//! NOMT but not by JMT. Run before deploying a binary that has dropped the +//! layer-1 `accounts` reads. +//! +//! Two-phase API: +//! 1. [`collect_legacy_account_entries`] reads all entries from `storage` +//! while it is still borrowable. +//! 2. [`apply_legacy_account_migration`] writes the entries to +//! `account_owners` and deletes them from `accounts` via a +//! [`sov_modules_api::StateCheckpoint`] (which consumes storage on +//! construction, so collection has to happen first). + +use anyhow::Context; +use sov_modules_api::{CredentialId, Spec, StateWriter}; +use sov_state::namespaces::User; +use sov_state::NativeStorage; + +use crate::{Account, AccountOwnerKey, Accounts}; + +/// Summary of a single legacy-accounts migration run. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct MigrationReport { + /// Number of `accounts` entries copied to `account_owners` and deleted + /// from the source map. + pub entries_migrated: u64, +} + +/// Reads every `(credential_id, Account)` pair currently stored in the +/// legacy [`Accounts::accounts`](crate::Accounts::accounts) map. +/// +/// # Errors +/// +/// - The backend does not support prefix iteration (e.g. JMT). NOMT does. +/// - A stored value cannot be borsh-decoded as [`Account`]. +#[allow(deprecated)] +pub fn collect_legacy_account_entries( + accounts: &Accounts, + storage: &Storage, +) -> anyhow::Result)>> +where + S: Spec, + Storage: NativeStorage, +{ + let raw_entries: Vec<(CredentialId, Vec)> = accounts + .accounts + .iter_raw(storage) + .context("failed to start prefix iteration over legacy accounts map")? + .ok_or_else(|| { + anyhow::anyhow!( + "storage backend does not support prefix iteration; \ + legacy-accounts migration requires NOMT" + ) + })? + .collect::>>()?; + + raw_entries + .into_iter() + .map(|(credential_id, value_bytes)| { + let account: Account = borsh::from_slice(&value_bytes) + .with_context(|| format!("failed to decode legacy Account for {credential_id}"))?; + Ok((credential_id, account)) + }) + .collect() +} + +/// Writes every entry from `entries` to +/// [`Accounts::account_owners`](crate::Accounts::account_owners) as +/// `(addr, credential_id) -> true` and deletes the corresponding +/// [`Accounts::accounts`](crate::Accounts::accounts) row. +/// +/// Idempotent: passing an empty slice (or running a second time after the +/// first run cleared the source) returns `entries_migrated: 0` without error. +/// +/// # Errors +/// +/// - The `writer` returns an error from `set`/`delete`. +#[allow(deprecated)] +pub fn apply_legacy_account_migration( + accounts: &mut Accounts, + entries: &[(CredentialId, Account)], + writer: &mut Writer, +) -> anyhow::Result +where + S: Spec, + Writer: StateWriter, +{ + for (credential_id, account) in entries { + let owner_key = AccountOwnerKey::new(account.addr, *credential_id); + accounts.account_owners.set(&owner_key, &true, writer)?; + accounts.accounts.delete(credential_id, writer)?; + } + + Ok(MigrationReport { + entries_migrated: entries.len() as u64, + }) +} + +#[cfg(test)] +mod tests { + use sov_modules_api::Spec; + use sov_test_utils::runtime::genesis::optimistic::HighLevelOptimisticGenesisConfig; + use sov_test_utils::runtime::TestRunner; + use sov_test_utils::{generate_optimistic_runtime, TestSpec}; + + use super::*; + use crate::Accounts; + + type S = TestSpec; + generate_optimistic_runtime!(MigrationTestRuntime <=); + type RT = MigrationTestRuntime; + + fn setup_runner() -> TestRunner { + let genesis_config = HighLevelOptimisticGenesisConfig::generate(); + let genesis = GenesisConfig::from_minimal_config(genesis_config.into()); + TestRunner::new_with_genesis(genesis.into_genesis_params(), RT::default()) + } + + fn cred(byte: u8) -> CredentialId { + [byte; 32].into() + } + + fn addr(byte: u8) -> ::Address { + let mut bytes = [0u8; 28]; + bytes[0] = byte; + ::Address::from(bytes) + } + + /// Migrating a populated `accounts` map writes `account_owners` entries + /// for every pair and clears the source rows. + #[test] + #[allow(deprecated)] + fn apply_migration_moves_entries_to_owners() { + let mut runner = setup_runner(); + let cred_1 = cred(1); + let cred_2 = cred(2); + let addr_1 = addr(0xAA); + let addr_2 = addr(0xBB); + + runner.__apply_to_state(|state| { + let mut accounts = Accounts::::default(); + + accounts + .accounts + .set(&cred_1, &Account { addr: addr_1 }, state) + .unwrap(); + accounts + .accounts + .set(&cred_2, &Account { addr: addr_2 }, state) + .unwrap(); + + assert_eq!( + accounts.accounts.get(&cred_1, state).unwrap(), + Some(Account { addr: addr_1 }) + ); + assert_eq!( + accounts.accounts.get(&cred_2, state).unwrap(), + Some(Account { addr: addr_2 }) + ); + + let entries = vec![ + (cred_1, Account { addr: addr_1 }), + (cred_2, Account { addr: addr_2 }), + ]; + let report = apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); + assert_eq!(report.entries_migrated, 2); + + assert!(accounts.accounts.get(&cred_1, state).unwrap().is_none()); + assert!(accounts.accounts.get(&cred_2, state).unwrap().is_none()); + assert!(accounts + .is_explicitly_authorized(&addr_1, &cred_1, state) + .unwrap()); + assert!(accounts + .is_explicitly_authorized(&addr_2, &cred_2, state) + .unwrap()); + }); + } + + /// Empty input is a valid no-op. + #[test] + fn apply_migration_empty_input() { + let mut runner = setup_runner(); + runner.__apply_to_state(|state| { + let mut accounts = Accounts::::default(); + let report = apply_legacy_account_migration(&mut accounts, &[], state).unwrap(); + assert_eq!(report.entries_migrated, 0); + }); + } + + /// Re-applying the same entries after they've already been migrated + /// re-writes the same `account_owners` entries (still `true`) and + /// no-op-deletes the (already empty) source rows; the post-state is + /// indistinguishable from a single application. + #[test] + #[allow(deprecated)] + fn apply_migration_is_safe_to_repeat() { + let mut runner = setup_runner(); + let cred_1 = cred(7); + let addr_1 = addr(0xCC); + let entries = vec![(cred_1, Account { addr: addr_1 })]; + + runner.__apply_to_state(|state| { + let mut accounts = Accounts::::default(); + + accounts + .accounts + .set(&cred_1, &Account { addr: addr_1 }, state) + .unwrap(); + + apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); + apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); + + assert!(accounts.accounts.get(&cred_1, state).unwrap().is_none()); + assert!(accounts + .is_explicitly_authorized(&addr_1, &cred_1, state) + .unwrap()); + }); + } +} diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs deleted file mode 100644 index 27833ff3f0..0000000000 --- a/crates/module-system/module-implementations/sov-accounts/src/query.rs +++ /dev/null @@ -1,35 +0,0 @@ -//! Defines queries exposed by the accounts module, along with the relevant types -use sov_modules_api::prelude::UnwrapInfallible; -use sov_modules_api::{ApiStateAccessor, CredentialId, Spec}; - -use crate::{Account, Accounts}; - -/// This is the response returned from the accounts_getAccount endpoint. -#[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] -#[serde( - bound = "Addr: serde::Serialize + serde::de::DeserializeOwned", - rename_all = "snake_case" -)] -pub enum Response { - /// A legacy/custom mapping for the given credential id exists. - AccountExists { - /// The mapped address. - addr: Addr, - }, - /// No legacy/custom mapping for the credential id exists. - AccountEmpty, -} - -impl Accounts { - /// Get the legacy/custom account mapping corresponding to the given credential id. - pub fn get_account( - &self, - credential_id: CredentialId, - state: &mut ApiStateAccessor, - ) -> Response { - match self.accounts.get(&credential_id, state).unwrap_infallible() { - Some(Account { addr }) => Response::AccountExists { addr }, - None => Response::AccountEmpty, - } - } -} diff --git a/crates/module-system/module-implementations/sov-accounts/src/tests.rs b/crates/module-system/module-implementations/sov-accounts/src/tests.rs index 8db2d82b8f..a7c975cab4 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/tests.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/tests.rs @@ -1,56 +1,8 @@ use sov_modules_api::prelude::*; use sov_modules_api::sov_universal_wallet::schema::Schema; -use crate::query::Response; use crate::CallMessage; -type S = sov_test_utils::TestSpec; - -#[test] -fn test_response_serialization() { - let addr: Vec = (1..=28).collect(); - let mut addr_array = [0u8; 28]; - addr_array.copy_from_slice(&addr); - let response = Response::AccountExists::<::Address> { - addr: ::Address::from(addr_array), - }; - - let json = serde_json::to_string(&response).unwrap(); - assert_eq!( - json, - r#"{"account_exists":{"addr":"sov1qypqxpq9qcrsszg2pvxq6rs0zqg3yyc5z5tpwxqergd3crhxalf"}}"# - ); -} - -#[test] -fn test_response_deserialization() { - let json = - r#"{"account_exists":{"addr":"sov1qypqxpq9qcrsszg2pvxq6rs0zqg3yyc5z5tpwxqergd3crhxalf"}}"#; - let response: Response<::Address> = serde_json::from_str(json).unwrap(); - - let expected_addr: Vec = (1..=28).collect(); - let mut addr_array = [0u8; 28]; - addr_array.copy_from_slice(&expected_addr); - let expected_response = Response::AccountExists::<::Address> { - addr: ::Address::from(addr_array), - }; - - assert_eq!(response, expected_response); -} - -#[test] -fn test_response_deserialization_on_wrong_hrp() { - let json = r#"{"account_exists":{"addr":"hax1qypqx68ju0l"}}"#; - let response: Result::Address>, serde_json::Error> = - serde_json::from_str(json); - match response { - Ok(response) => panic!("Expected error, got {response:?}"), - Err(err) => { - assert_eq!(err.to_string(), "Wrong HRP: hax at line 1 column 44"); - } - } -} - #[test] fn test_display_accounts_call() { #[derive(Debug, Clone, PartialEq, borsh::BorshSerialize, UniversalWallet)] diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index 326e0acc73..c88d25e38a 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -1,4 +1,4 @@ -use sov_accounts::{Accounts, CallMessage, Response}; +use sov_accounts::{Accounts, CallMessage}; use sov_modules_api::transaction::{UnsignedTransactionV0, Version1}; use sov_modules_api::{ CryptoSpec, PrivateKey, PublicKey, RawTx, Runtime, SkippedTxContents, Spec, TxEffect, @@ -74,11 +74,13 @@ fn test_config_account() { runner, ) = setup(); - // The account is registered at genesis. + // The account is registered at genesis: its credential is authorized for + // the user's address via `account_owners`. runner.query_visible_state(|state| { let accounts = Accounts::::default(); - let response = accounts.get_account(user.credential_id(), state); - assert_eq!(response, Response::AccountEmpty); + assert!(accounts + .is_explicitly_authorized(&user.address(), &user.credential_id(), state) + .unwrap()); }); } @@ -103,17 +105,10 @@ fn test_update_account() { let accounts = Accounts::::default(); - // New credential has no `accounts` map entry under the new model. - assert_eq!( - accounts.get_account(new_credential, state), - Response::AccountEmpty - ); - // Sender's own credential also has no `accounts` map entry. - assert_eq!( - accounts.get_account(user.credential_id(), state), - Response::AccountEmpty - ); - + // The new credential is authorized for the user's address. + assert!(accounts + .is_explicitly_authorized(&user.address(), &new_credential, state) + .unwrap()); assert_ne!(new_credential, user.credential_id()); }), }); @@ -365,15 +360,10 @@ fn test_register_new_account() { mut runner, ) = setup(); - // The credential has no legacy/custom account-map entry at genesis. assert_eq!(non_registered_account.custom_credential_id, None); runner.query_visible_state(|state| { let accounts = Accounts::::default(); - assert_eq!( - accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountEmpty - ); assert!(!accounts .is_explicitly_authorized( &non_registered_account.address(), @@ -407,10 +397,6 @@ fn test_register_new_account() { assert!(accounts .is_authorized_for(&non_registered_account.address(), &new_credential, state) .unwrap()); - assert_eq!( - accounts.get_account(new_credential, state), - Response::AccountEmpty - ); assert!(!accounts .is_explicitly_authorized( @@ -426,10 +412,6 @@ fn test_register_new_account() { state ) .unwrap()); - assert_eq!( - accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountEmpty - ); assert_ne!(new_credential, non_registered_account.credential_id()); }), @@ -465,10 +447,6 @@ fn test_resolve_sender_address_with_default_address_non_registered() { state ) .unwrap()); - assert_eq!( - accounts.get_account(non_registered_account.credential_id(), state), - Response::AccountEmpty - ); }); } @@ -560,14 +538,6 @@ fn test_resolve_address_with_multi_credential_ownership() { assert!(accounts .is_authorized_for(&addr, &credential_2, state) .unwrap()); - assert_eq!( - accounts.get_account(credential_1, state), - Response::AccountEmpty - ); - assert_eq!( - accounts.get_account(credential_2, state), - Response::AccountEmpty - ); }); } @@ -591,10 +561,6 @@ fn test_resolve_with_different_default_address() { assert!(!accounts .is_explicitly_authorized(&account_1.address(), &random_credential, state) .unwrap()); - assert_eq!( - accounts.get_account(random_credential, state), - Response::AccountEmpty - ); }); } diff --git a/crates/module-system/module-schemas/schemas/sov-accounts.json b/crates/module-system/module-schemas/schemas/sov-accounts.json index a4d990f3cf..629b469441 100644 --- a/crates/module-system/module-schemas/schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/schemas/sov-accounts.json @@ -4,7 +4,7 @@ "description": "Represents the available call messages for interacting with the sov-accounts module.", "oneOf": [ { - "description": "Authorizes `credential_id` as a signer for the caller's address. Fails if the credential has a legacy/custom account mapping or is already authorized for the caller's address.", + "description": "Authorizes `credential_id` as a signer for the caller's address. Fails if the credential is already authorized for the caller's address.", "type": "object", "required": [ "insert_credential_id" diff --git a/examples/demo-rollup/Cargo.toml b/examples/demo-rollup/Cargo.toml index 3ca0126090..c2fb60e774 100644 --- a/examples/demo-rollup/Cargo.toml +++ b/examples/demo-rollup/Cargo.toml @@ -22,6 +22,7 @@ sov-stf-runner = { workspace = true } sov-metrics = { workspace = true, features = ["native"] } # Sovereign crates +sov-accounts = { workspace = true, features = ["native"], optional = true } sov-bank = { workspace = true, features = ["native"] } sov-blob-storage = { workspace = true, features = ["native"], optional = true } sov-chain-state = { workspace = true, features = ["native"], optional = true } @@ -144,6 +145,7 @@ default = [] # Used for using different encoding between host and guest bincode = ["risc0/bincode", "sov-risc0-adapter/bincode"] migration-script = [ + "dep:sov-accounts", "dep:sov-blob-storage", "dep:rockbound", "dep:bincode", @@ -171,6 +173,11 @@ name = "mockda-to-celestia-migrate" path = "src/migrations/mockda_to_celestia.rs" required-features = ["migration-script"] +[[bin]] +name = "legacy-accounts-migrate" +path = "src/migrations/legacy_accounts.rs" +required-features = ["migration-script"] + [[bin]] name = "sov-hive-genesis-adapter" path = "src/hive/genesis_adapter.rs" diff --git a/examples/demo-rollup/src/migrations/legacy_accounts.rs b/examples/demo-rollup/src/migrations/legacy_accounts.rs new file mode 100644 index 0000000000..61667a37c6 --- /dev/null +++ b/examples/demo-rollup/src/migrations/legacy_accounts.rs @@ -0,0 +1,244 @@ +//! Offline migration tool that copies every legacy +//! `sov_accounts::Accounts::accounts` entry into `Accounts::account_owners` +//! and deletes the source row. +//! +//! Run this against a stopped MockDA demo-rollup DB before deploying a binary +//! that has dropped the layer-1 `accounts` reads. The tool is structurally +//! identical for the Celestia rollup; swap `MockDemoRollup` for +//! `CelestiaDemoRollup` (and `MockDaSpec` for `CelestiaSpec`). + +use std::path::PathBuf; + +use anyhow::{bail, Context}; +use clap::Parser; +use demo_stf::runtime::Runtime; +use rockbound::SchemaBatch; +use serde::Serialize; +use sov_db::ledger_db::LedgerDb; +use sov_db::schema::tables::SlotByNumber; +use sov_db::storage_manager::NomtStorageManager; +use sov_full_node_configs::runner::from_toml_path; +use sov_mock_da::storable::StorableMockDaService; +use sov_mock_da::MockDaSpec; +use sov_modules_api::capabilities::HasKernel; +use sov_modules_api::execution_mode::Native; +use sov_modules_api::{Spec, StateCheckpoint}; +use sov_modules_rollup_blueprint::RollupBlueprint; +use sov_rollup_interface::common::SlotNumber; +use sov_state::{NativeStorage, StateUpdate}; +use sov_stf_runner::RollupConfig; + +use sov_demo_rollup::MockDemoRollup; + +#[derive(Parser, Debug)] +#[command( + author, + version, + about = "Offline migration tool for sov-accounts: copies legacy `accounts` map entries to \ + `account_owners` and deletes the source rows." +)] +struct Args { + /// Path to the rollup config file used by the running node. + #[arg(long)] + rollup_config_path: PathBuf, + + /// Override `storage.path` from the rollup config. + #[arg(long)] + db_path: Option, + + /// Compute the post-migration state root but do not commit changes. + #[arg(long, default_value_t = false)] + dry_run: bool, + + /// Optional path to write the JSON migration report. + #[arg(long)] + report_out: Option, +} + +type RollupSpec = as RollupBlueprint>::Spec; +type Hasher = <::CryptoSpec as sov_modules_api::CryptoSpec>::Hasher; +type DemoStorage = ::Storage; +type DemoStorageManager = NomtStorageManager; + +#[derive(Serialize)] +struct MigrationReport { + dry_run: bool, + db_path: String, + pre_state_root: String, + post_state_root: String, + head_rollup_slot: u64, + entries_migrated: u64, +} + +fn main() { + if let Err(err) = run() { + eprintln!("legacy-accounts migration failed: {err:#}"); + std::process::exit(1); + } +} + +fn run() -> anyhow::Result<()> { + let args = Args::parse(); + + let storage_config = load_storage_config(&args)?; + let db_path = storage_config.path.clone(); + + let mut storage_manager = DemoStorageManager::new(storage_config, false) + .with_context(|| format!("failed to open storage manager at {}", db_path.display()))?; + let (storage, ledger_reader) = storage_manager + .create_state_for_migration() + .context("failed to create migration storage view")?; + let ledger_db = + LedgerDb::with_reader(ledger_reader).context("failed to initialize ledger db")?; + + let head_slot_number = + assert_storage_latest_version_matches_ledger_head(&storage, &ledger_db, "pre-migration")?; + assert_ledger_head_state_root_matches_storage_root(&storage, &ledger_db, "pre-migration")?; + + let pre_state_root = storage + .get_root_hash(head_slot_number) + .context("failed to read pre-migration state root")?; + + let mut runtime = Runtime::::default(); + let entries = + sov_accounts::migrations::collect_legacy_account_entries(&runtime.accounts, &storage) + .context("failed to collect legacy accounts entries")?; + + let mut checkpoint = StateCheckpoint::new(storage, &runtime.kernel(), None); + let report = sov_accounts::migrations::apply_legacy_account_migration( + &mut runtime.accounts, + &entries, + &mut checkpoint, + ) + .context("failed to apply legacy-accounts migration to checkpoint")?; + + let (next_state_root, mut state_update, accessory_delta, _witness, storage_after) = + checkpoint.materialize_update(pre_state_root.clone()); + state_update.add_accessory_items(accessory_delta.freeze()); + let change_set = storage_after.materialize_changes_at_version(state_update, head_slot_number); + + let post_state_root = if args.dry_run { + next_state_root + } else { + let ledger_change_set = make_ledger_root_patch(&ledger_db, next_state_root.as_ref())?; + storage_manager + .commit_migration_change_set_at_head(head_slot_number, change_set, ledger_change_set) + .context("failed to commit migration changeset at head version")?; + + let (post_storage, post_ledger_reader) = storage_manager + .create_state_for_migration() + .context("failed to create post-migration storage view")?; + let post_ledger_db = LedgerDb::with_reader(post_ledger_reader) + .context("failed to initialize post-migration ledger db view")?; + let post_head_slot_number = assert_storage_latest_version_matches_ledger_head( + &post_storage, + &post_ledger_db, + "post-migration", + )?; + if post_head_slot_number != head_slot_number { + bail!( + "post-migration head slot changed unexpectedly: expected {}, found {}", + head_slot_number, + post_head_slot_number + ); + } + assert_ledger_head_state_root_matches_storage_root( + &post_storage, + &post_ledger_db, + "post-migration", + )?; + post_storage + .get_root_hash(post_head_slot_number) + .context("failed to read post-migration state root")? + }; + + let migration_report = MigrationReport { + dry_run: args.dry_run, + db_path: db_path.display().to_string(), + pre_state_root: pre_state_root.to_string(), + post_state_root: post_state_root.to_string(), + head_rollup_slot: head_slot_number.get(), + entries_migrated: report.entries_migrated, + }; + + let json = serde_json::to_string_pretty(&migration_report) + .context("failed to serialize migration report JSON")?; + + if let Some(path) = args.report_out { + std::fs::write(&path, &json) + .with_context(|| format!("failed to write migration report to {}", path.display()))?; + } + + println!("{json}"); + Ok(()) +} + +fn load_storage_config(args: &Args) -> anyhow::Result { + let rollup_config: RollupConfig<::Address, StorableMockDaService> = + from_toml_path(&args.rollup_config_path).with_context(|| { + format!( + "failed to read rollup config from {}", + args.rollup_config_path.display() + ) + })?; + + let mut storage = rollup_config.storage; + if let Some(db_path_override) = &args.db_path { + storage.path = db_path_override.clone(); + } + Ok(storage) +} + +fn assert_storage_latest_version_matches_ledger_head( + storage: &S, + ledger_db: &LedgerDb, + phase: &str, +) -> anyhow::Result { + let (head_slot_number, _head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; + let storage_latest_version = storage.latest_version(); + if storage_latest_version != head_slot_number { + bail!( + "{phase} invariant failed: storage.latest_version ({}) != ledger head slot ({})", + storage_latest_version, + head_slot_number + ); + } + Ok(head_slot_number) +} + +fn assert_ledger_head_state_root_matches_storage_root( + storage: &S, + ledger_db: &LedgerDb, + phase: &str, +) -> anyhow::Result<()> { + let (head_slot_number, head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; + let storage_root = storage + .get_root_hash(head_slot_number) + .context("failed to read storage root at ledger head slot")?; + if head_slot.state_root.as_ref() != storage_root.as_ref() { + bail!( + "{phase} invariant failed: ledger head state_root does not match storage root at slot {}", + head_slot_number + ); + } + Ok(()) +} + +fn make_ledger_root_patch( + ledger_db: &LedgerDb, + new_state_root: &[u8], +) -> anyhow::Result { + let (head_slot_number, mut head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot patch state root"))?; + + head_slot.state_root = new_state_root.to_vec().into(); + + let mut batch = SchemaBatch::new(); + batch.put::(&head_slot_number, &head_slot)?; + Ok(batch) +} From acbef4a27f926e200f88771b405ef33fc133bf08 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 5 May 2026 11:54:31 +0200 Subject: [PATCH 17/21] Clean up --- .../sov-accounts/src/capabilities.rs | 25 ---- .../sov-accounts/src/lib.rs | 10 +- .../sov-accounts/src/migrations.rs | 20 ++-- .../sov-accounts/tests/integration/main.rs | 112 +----------------- .../module-system/sov-capabilities/src/lib.rs | 21 +--- .../sov-test-utils/src/runtime/macros.rs | 6 +- examples/demo-rollup/stf/src/runtime.rs | 8 +- 7 files changed, 34 insertions(+), 168 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 71ff90e0ac..7128610e4b 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -18,31 +18,6 @@ impl Accounts { ) } - /// Resolve the sender's credential to an address. - /// - /// Returns `default_address` unconditionally. The legacy `accounts` map is - /// no longer consulted; operators must run the legacy-accounts migration - /// (see [`crate::migrations`]) before deploying a binary that includes - /// this code on a chain with pre-upgrade entries. - pub fn resolve_sender_address>( - &mut self, - default_address: &S::Address, - _credential_id: &CredentialId, - _state: &mut ST, - ) -> Result { - Ok(*default_address) - } - - /// Read-only variant of [`Self::resolve_sender_address`]. - pub fn resolve_sender_address_read_only>( - &self, - default_address: &S::Address, - _credential_id: &CredentialId, - _state: &mut ST, - ) -> Result { - Ok(*default_address) - } - /// Returns `true` only if `(address, credential_id)` has an explicit entry /// in `account_owners`. For the full authorization check including the /// canonical fallback, use [`Self::is_authorized_for`]. diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 51e1dc83fd..da8b2c1283 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -1,4 +1,7 @@ #![deny(missing_docs)] +// Tombstone field `_accounts` makes the `ModuleInfo`-derived +// `_prefix__accounts` accessor double-underscored. +#![allow(non_snake_case)] #![doc = include_str!("../README.md")] mod call; mod capabilities; @@ -91,10 +94,11 @@ pub struct Accounts { /// removing it would shift the discriminants of every following field /// and corrupt their on-disk data). Existing entries are migrated to /// [`Self::account_owners`] by - /// [`crate::migrations::migrate_legacy_accounts_to_owners`] and the - /// source rows are deleted. + /// [`crate::migrations::apply_legacy_account_migration`] and the source + /// rows are deleted. The leading underscore signals to readers that this + /// field is intentionally unused. #[state] - pub(crate) accounts: StateMap>, + pub(crate) _accounts: StateMap>, /// If this field is false, configured genesis authorizations and /// `CallMessage::InsertCredentialId` messages will be rejected. diff --git a/crates/module-system/module-implementations/sov-accounts/src/migrations.rs b/crates/module-system/module-implementations/sov-accounts/src/migrations.rs index a16031347b..a7fb97900c 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/migrations.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/migrations.rs @@ -48,7 +48,7 @@ where Storage: NativeStorage, { let raw_entries: Vec<(CredentialId, Vec)> = accounts - .accounts + ._accounts .iter_raw(storage) .context("failed to start prefix iteration over legacy accounts map")? .ok_or_else(|| { @@ -93,7 +93,7 @@ where for (credential_id, account) in entries { let owner_key = AccountOwnerKey::new(account.addr, *credential_id); accounts.account_owners.set(&owner_key, &true, writer)?; - accounts.accounts.delete(credential_id, writer)?; + accounts._accounts.delete(credential_id, writer)?; } Ok(MigrationReport { @@ -146,20 +146,20 @@ mod tests { let mut accounts = Accounts::::default(); accounts - .accounts + ._accounts .set(&cred_1, &Account { addr: addr_1 }, state) .unwrap(); accounts - .accounts + ._accounts .set(&cred_2, &Account { addr: addr_2 }, state) .unwrap(); assert_eq!( - accounts.accounts.get(&cred_1, state).unwrap(), + accounts._accounts.get(&cred_1, state).unwrap(), Some(Account { addr: addr_1 }) ); assert_eq!( - accounts.accounts.get(&cred_2, state).unwrap(), + accounts._accounts.get(&cred_2, state).unwrap(), Some(Account { addr: addr_2 }) ); @@ -170,8 +170,8 @@ mod tests { let report = apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); assert_eq!(report.entries_migrated, 2); - assert!(accounts.accounts.get(&cred_1, state).unwrap().is_none()); - assert!(accounts.accounts.get(&cred_2, state).unwrap().is_none()); + assert!(accounts._accounts.get(&cred_1, state).unwrap().is_none()); + assert!(accounts._accounts.get(&cred_2, state).unwrap().is_none()); assert!(accounts .is_explicitly_authorized(&addr_1, &cred_1, state) .unwrap()); @@ -208,14 +208,14 @@ mod tests { let mut accounts = Accounts::::default(); accounts - .accounts + ._accounts .set(&cred_1, &Account { addr: addr_1 }, state) .unwrap(); apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); apply_legacy_account_migration(&mut accounts, &entries, state).unwrap(); - assert!(accounts.accounts.get(&cred_1, state).unwrap().is_none()); + assert!(accounts._accounts.get(&cred_1, state).unwrap().is_none()); assert!(accounts .is_explicitly_authorized(&addr_1, &cred_1, state) .unwrap()); diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index c88d25e38a..de534a4773 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -21,6 +21,9 @@ type RT = TestAccountsRuntime; struct TestData { account_1: TestUser, + // `account_2` is intentionally kept in the fixture for the upcoming + // `target_address` PR. Until then it has no readers — silence the lint. + #[allow(dead_code)] account_2: TestUser, non_registered_account: TestUser, } @@ -418,77 +421,10 @@ fn test_register_new_account() { }); } -#[test] -fn test_resolve_sender_address_with_default_address_non_registered() { - let ( - TestData { - non_registered_account, - .. - }, - runner, - ) = setup(); - - runner.query_visible_state(|state| { - let mut accounts = Accounts::::default(); - assert_eq!( - accounts - .resolve_sender_address( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state - ) - .unwrap(), - non_registered_account.address() - ); - assert!(!accounts - .is_explicitly_authorized( - &non_registered_account.address(), - &non_registered_account.credential_id(), - state - ) - .unwrap()); - }); -} - -/// Genesis-authorized credentials have no `accounts` entry, so credential-only -/// resolution falls back to the supplied address. -#[test] -fn test_resolve_sender_address_registered() { - let ( - TestData { - account_1, - account_2, - .. - }, - runner, - ) = setup(); - - runner.query_visible_state(|state| { - let mut accounts = Accounts::::default(); - - assert_eq!( - accounts - .resolve_sender_address(&account_1.address(), &account_1.credential_id(), state) - .unwrap(), - account_1.address() - ); - assert_eq!( - accounts - .resolve_sender_address(&account_2.address(), &account_1.credential_id(), state) - .unwrap(), - account_2.address() - ); - assert!(accounts - .is_authorized_for(&account_1.address(), &account_1.credential_id(), state) - .unwrap()); - }); -} - /// After `InsertCredentialId` from a user, each inserted credential is -/// authorized under that user's address without getting a credential-indexed -/// account entry. +/// authorized under that user's address. #[test] -fn test_resolve_address_with_multi_credential_ownership() { +fn test_authorize_multiple_credentials_for_same_address() { let ( TestData { non_registered_account, @@ -510,22 +446,9 @@ fn test_resolve_address_with_multi_credential_ownership() { ); runner.query_visible_state(|state| { - let mut accounts = Accounts::::default(); + let accounts = Accounts::::default(); let addr = non_registered_account.address(); - assert_eq!( - accounts - .resolve_sender_address(&addr, &credential_1, state) - .unwrap(), - addr - ); - assert_eq!( - accounts - .resolve_sender_address(&addr, &credential_2, state) - .unwrap(), - addr - ); - assert!(accounts .is_explicitly_authorized(&addr, &credential_1, state) .unwrap()); @@ -541,29 +464,6 @@ fn test_resolve_address_with_multi_credential_ownership() { }); } -/// Resolving a credential with no legacy/custom mapping returns the supplied -/// fallback without writing account state. -#[test] -fn test_resolve_with_different_default_address() { - let (TestData { account_1, .. }, runner) = setup(); - - let random_credential = TestPrivateKey::generate().pub_key().credential_id(); - - runner.query_visible_state(|state| { - let mut accounts = Accounts::::default(); - - assert_eq!( - accounts - .resolve_sender_address(&account_1.address(), &random_credential, state) - .unwrap(), - account_1.address() - ); - assert!(!accounts - .is_explicitly_authorized(&account_1.address(), &random_credential, state) - .unwrap()); - }); -} - #[test] fn test_disable_custom_account_mappings() { let (user, mut runner) = setup_with_disable_custom_account_mappings(); diff --git a/crates/module-system/sov-capabilities/src/lib.rs b/crates/module-system/sov-capabilities/src/lib.rs index b462f840d3..c0e1274958 100644 --- a/crates/module-system/sov-capabilities/src/lib.rs +++ b/crates/module-system/sov-capabilities/src/lib.rs @@ -311,19 +311,13 @@ impl TransactionAuthorizer for StandardProvenRollupCapabilities<' auth_data: &AuthorizationData, sequencer: &::Address, sequencer_rollup_address: S::Address, - state: &mut impl StateAccessor, + _state: &mut impl StateAccessor, sequencing_data: Option, execution_context: ExecutionContext, sequencer_type: SequencerType, ) -> anyhow::Result> { - // This should be resolved by the sequencer registry during blob selection - let sender = self.accounts.resolve_sender_address( - &auth_data.default_address, - &auth_data.credential_id, - state, - )?; Ok(Context::new( - sender, + auth_data.default_address, auth_data.credentials.clone(), sequencer_rollup_address, *sequencer, @@ -337,19 +331,14 @@ impl TransactionAuthorizer for StandardProvenRollupCapabilities<' &mut self, auth_data: &AuthorizationData, sequencer: &<::Da as DaSpec>::Address, - state: &mut impl StateAccessor, + _state: &mut impl StateAccessor, execution_context: ExecutionContext, ) -> anyhow::Result> { - let sender = self.accounts.resolve_sender_address( - &auth_data.default_address, - &auth_data.credential_id, - state, - )?; // The tx sender & sequencer are the same entity Ok(Context::new( - sender, + auth_data.default_address, auth_data.credentials.clone(), - sender, + auth_data.default_address, *sequencer, None, execution_context, diff --git a/crates/utils/sov-test-utils/src/runtime/macros.rs b/crates/utils/sov-test-utils/src/runtime/macros.rs index 68e7761e2a..7fb2818e1f 100644 --- a/crates/utils/sov-test-utils/src/runtime/macros.rs +++ b/crates/utils/sov-test-utils/src/runtime/macros.rs @@ -172,10 +172,10 @@ macro_rules! generate_runtime_without_capabilities { fn resolve_address>( &self, default_address: &S::Address, - credential_id: &::sov_modules_api::CredentialId, - state: &mut ST, + _credential_id: &::sov_modules_api::CredentialId, + _state: &mut ST, ) -> ::std::result::Result{ - self.accounts.resolve_sender_address_read_only(default_address, credential_id, state) + ::std::result::Result::Ok(*default_address) } fn genesis_config(_input: &Self::GenesisInput) -> ::sov_modules_api::prelude::anyhow::Result { diff --git a/examples/demo-rollup/stf/src/runtime.rs b/examples/demo-rollup/stf/src/runtime.rs index 6352567300..b5c0937599 100644 --- a/examples/demo-rollup/stf/src/runtime.rs +++ b/examples/demo-rollup/stf/src/runtime.rs @@ -122,12 +122,10 @@ where fn resolve_address>( &self, default_address: &S::Address, - credential_id: &sov_modules_api::CredentialId, - state: &mut ST, + _credential_id: &sov_modules_api::CredentialId, + _state: &mut ST, ) -> Result { - self.0 - .accounts - .resolve_sender_address_read_only(default_address, credential_id, state) + Ok(*default_address) } #[cfg(feature = "native")] From d1c2bc90261885af1c05591dc2a2be3577b2116e Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 5 May 2026 12:55:16 +0200 Subject: [PATCH 18/21] Fix migrations and add custom REST API for accounts module --- .../sov-accounts/src/lib.rs | 4 + .../sov-accounts/src/query.rs | 95 +++++++++++++++++++ .../src/migrations/legacy_accounts.rs | 28 +++++- 3 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 crates/module-system/module-implementations/sov-accounts/src/query.rs diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index da8b2c1283..5a9c452091 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -11,6 +11,10 @@ mod genesis; pub use genesis::*; #[cfg(feature = "native")] pub mod migrations; +#[cfg(feature = "native")] +mod query; +#[cfg(feature = "native")] +pub use query::*; #[cfg(test)] mod tests; pub use call::CallMessage; diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs new file mode 100644 index 0000000000..39ee1ea46e --- /dev/null +++ b/crates/module-system/module-implementations/sov-accounts/src/query.rs @@ -0,0 +1,95 @@ +//! Read-only REST endpoints for inspecting `sov-accounts` state. +//! +//! Intended for migration verification and debugging. The legacy +//! `_accounts` lookup exists so operators can confirm the source map is +//! drained after running [`crate::migrations::apply_legacy_account_migration`]. + +use std::str::FromStr; + +use axum::routing::get; +use sov_modules_api::prelude::{axum, UnwrapInfallible}; +use sov_modules_api::rest::utils::{errors, ApiResult, Path}; +use sov_modules_api::rest::{ApiState, HasCustomRestApi}; +use sov_modules_api::{ApiStateAccessor, CredentialId, Spec}; + +use crate::{AccountOwnerKey, Accounts}; + +/// Response of `GET /authorizations/{address}/{credential_id}`. +#[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] +pub struct AuthorizationResponse { + /// `true` iff `(address, credential_id)` has an explicit `account_owners` + /// entry. Does not include the canonical-address fallback. + pub authorized: bool, +} + +/// Response of `GET /legacy/{credential_id}`. +#[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] +#[serde(bound = "Addr: serde::Serialize + serde::de::DeserializeOwned")] +pub struct LegacyAccountResponse { + /// The address from the legacy `_accounts` map for this credential, if any. + /// Expected to be `null` for every credential after the legacy-accounts + /// migration has run. + pub legacy_address: Option, +} + +impl Accounts { + async fn route_is_authorized( + state: ApiState, + mut accessor: ApiStateAccessor, + Path((address_str, credential_id_str)): Path<(String, String)>, + ) -> ApiResult { + let address = ::from_str(&address_str).map_err(|_| { + errors::bad_request_400( + &format!("invalid address `{address_str}`"), + "address parse failed", + ) + })?; + let credential_id = CredentialId::from_str(&credential_id_str).map_err(|e| { + errors::bad_request_400( + &format!("invalid credential_id `{credential_id_str}`"), + e.to_string(), + ) + })?; + + let authorized = state + .account_owners + .get(&AccountOwnerKey::new(address, credential_id), &mut accessor) + .unwrap_infallible() + .unwrap_or(false); + Ok(AuthorizationResponse { authorized }.into()) + } + + async fn route_legacy_account( + state: ApiState, + mut accessor: ApiStateAccessor, + Path(credential_id_str): Path, + ) -> ApiResult> { + let credential_id = CredentialId::from_str(&credential_id_str).map_err(|e| { + errors::bad_request_400( + &format!("invalid credential_id `{credential_id_str}`"), + e.to_string(), + ) + })?; + + let legacy_address = state + ._accounts + .get(&credential_id, &mut accessor) + .unwrap_infallible() + .map(|a| a.addr); + Ok(LegacyAccountResponse { legacy_address }.into()) + } +} + +impl HasCustomRestApi for Accounts { + type Spec = S; + + fn custom_rest_api(&self, state: ApiState) -> axum::Router<()> { + axum::Router::new() + .route( + "/authorizations/{address}/{credential_id}", + get(Self::route_is_authorized), + ) + .route("/legacy/{credential_id}", get(Self::route_legacy_account)) + .with_state(state.with(self.clone())) + } +} diff --git a/examples/demo-rollup/src/migrations/legacy_accounts.rs b/examples/demo-rollup/src/migrations/legacy_accounts.rs index 61667a37c6..eb39131e98 100644 --- a/examples/demo-rollup/src/migrations/legacy_accounts.rs +++ b/examples/demo-rollup/src/migrations/legacy_accounts.rs @@ -22,10 +22,10 @@ use sov_mock_da::storable::StorableMockDaService; use sov_mock_da::MockDaSpec; use sov_modules_api::capabilities::HasKernel; use sov_modules_api::execution_mode::Native; -use sov_modules_api::{Spec, StateCheckpoint}; +use sov_modules_api::{ModuleInfo, Spec, StateCheckpoint, StateWriter}; use sov_modules_rollup_blueprint::RollupBlueprint; use sov_rollup_interface::common::SlotNumber; -use sov_state::{NativeStorage, StateUpdate}; +use sov_state::{Kernel, NativeStorage, Prefix, SlotKey, StateUpdate}; use sov_stf_runner::RollupConfig; use sov_demo_rollup::MockDemoRollup; @@ -104,6 +104,28 @@ fn run() -> anyhow::Result<()> { sov_accounts::migrations::collect_legacy_account_entries(&runtime.accounts, &storage) .context("failed to collect legacy accounts entries")?; + // The historical-state DB enforces that any version touching the User + // namespace also touches the Kernel namespace (see + // `sov-db/src/historical_state.rs` "User namespace got updated without + // kernel namespace"). Our migration only writes User entries, so we + // round-trip the kernel `chain_state.true_slot_number` value to register + // a state-root-neutral kernel write. NOMT roots are pure functions of + // (key, value) pairs, so writing the same bytes back to the same key + // leaves the post-migration root identical to what the user-only diff + // would produce. + let true_slot_key = SlotKey::singleton(&Prefix::new( + runtime.chain_state.discriminant(), + sov_chain_state::ChainState::::TRUE_SLOT_NUMBER_ITEM_DISCRIMINANT, + )); + let true_slot_value = storage + .get_unbound::(true_slot_key.clone()) + .ok_or_else(|| { + anyhow::anyhow!( + "kernel `chain_state.true_slot_number` is unset; \ + cannot perform no-op kernel write" + ) + })?; + let mut checkpoint = StateCheckpoint::new(storage, &runtime.kernel(), None); let report = sov_accounts::migrations::apply_legacy_account_migration( &mut runtime.accounts, @@ -111,6 +133,8 @@ fn run() -> anyhow::Result<()> { &mut checkpoint, ) .context("failed to apply legacy-accounts migration to checkpoint")?; + StateWriter::::set(&mut checkpoint, &true_slot_key, true_slot_value) + .context("failed to round-trip kernel value to satisfy historical-state invariant")?; let (next_state_root, mut state_update, accessory_delta, _witness, storage_after) = checkpoint.materialize_update(pre_state_root.clone()); From a274156e68b56d9717d50615eb37742d70ccb076 Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 5 May 2026 14:09:28 +0200 Subject: [PATCH 19/21] Remove legacy account map reading from custom REST API. Extract common migration logic --- .../sov-accounts/src/query.rs | 35 -------- examples/demo-rollup/src/migrations/common.rs | 66 +++++++++++++++ .../src/migrations/legacy_accounts.rs | 80 +++---------------- .../src/migrations/mockda_to_celestia.rs | 65 ++------------- 4 files changed, 87 insertions(+), 159 deletions(-) create mode 100644 examples/demo-rollup/src/migrations/common.rs diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs index 39ee1ea46e..9a4d10a548 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/query.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/query.rs @@ -1,8 +1,4 @@ //! Read-only REST endpoints for inspecting `sov-accounts` state. -//! -//! Intended for migration verification and debugging. The legacy -//! `_accounts` lookup exists so operators can confirm the source map is -//! drained after running [`crate::migrations::apply_legacy_account_migration`]. use std::str::FromStr; @@ -22,16 +18,6 @@ pub struct AuthorizationResponse { pub authorized: bool, } -/// Response of `GET /legacy/{credential_id}`. -#[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] -#[serde(bound = "Addr: serde::Serialize + serde::de::DeserializeOwned")] -pub struct LegacyAccountResponse { - /// The address from the legacy `_accounts` map for this credential, if any. - /// Expected to be `null` for every credential after the legacy-accounts - /// migration has run. - pub legacy_address: Option, -} - impl Accounts { async fn route_is_authorized( state: ApiState, @@ -58,26 +44,6 @@ impl Accounts { .unwrap_or(false); Ok(AuthorizationResponse { authorized }.into()) } - - async fn route_legacy_account( - state: ApiState, - mut accessor: ApiStateAccessor, - Path(credential_id_str): Path, - ) -> ApiResult> { - let credential_id = CredentialId::from_str(&credential_id_str).map_err(|e| { - errors::bad_request_400( - &format!("invalid credential_id `{credential_id_str}`"), - e.to_string(), - ) - })?; - - let legacy_address = state - ._accounts - .get(&credential_id, &mut accessor) - .unwrap_infallible() - .map(|a| a.addr); - Ok(LegacyAccountResponse { legacy_address }.into()) - } } impl HasCustomRestApi for Accounts { @@ -89,7 +55,6 @@ impl HasCustomRestApi for Accounts { "/authorizations/{address}/{credential_id}", get(Self::route_is_authorized), ) - .route("/legacy/{credential_id}", get(Self::route_legacy_account)) .with_state(state.with(self.clone())) } } diff --git a/examples/demo-rollup/src/migrations/common.rs b/examples/demo-rollup/src/migrations/common.rs new file mode 100644 index 0000000000..88a90889cb --- /dev/null +++ b/examples/demo-rollup/src/migrations/common.rs @@ -0,0 +1,66 @@ +//! Shared helpers for offline migration binaries. +//! +//! Each binary in `examples/demo-rollup/src/migrations` includes this file via +//! `#[path = "common.rs"] mod common;` since binaries don't share a crate +//! root with the package library. + +use anyhow::{bail, Context}; +use rockbound::SchemaBatch; +use sov_db::ledger_db::LedgerDb; +use sov_db::schema::tables::SlotByNumber; +use sov_rollup_interface::common::SlotNumber; +use sov_state::NativeStorage; + +pub fn assert_storage_latest_version_matches_ledger_head( + storage: &S, + ledger_db: &LedgerDb, + phase: &str, +) -> anyhow::Result { + let (head_slot_number, _head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; + let storage_latest_version = storage.latest_version(); + if storage_latest_version != head_slot_number { + bail!( + "{phase} invariant failed: storage.latest_version ({}) != ledger head slot ({})", + storage_latest_version, + head_slot_number + ); + } + Ok(head_slot_number) +} + +pub fn assert_ledger_head_state_root_matches_storage_root( + storage: &S, + ledger_db: &LedgerDb, + phase: &str, +) -> anyhow::Result<()> { + let (head_slot_number, head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; + let storage_root = storage + .get_root_hash(head_slot_number) + .context("failed to read storage root at ledger head slot")?; + if head_slot.state_root.as_ref() != storage_root.as_ref() { + bail!( + "{phase} invariant failed: ledger head state_root does not match storage root at slot {}", + head_slot_number + ); + } + Ok(()) +} + +pub fn make_ledger_root_patch( + ledger_db: &LedgerDb, + new_state_root: &[u8], +) -> anyhow::Result { + let (head_slot_number, mut head_slot) = ledger_db + .get_head_slot()? + .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot patch state root"))?; + + head_slot.state_root = new_state_root.to_vec().into(); + + let mut batch = SchemaBatch::new(); + batch.put::(&head_slot_number, &head_slot)?; + Ok(batch) +} diff --git a/examples/demo-rollup/src/migrations/legacy_accounts.rs b/examples/demo-rollup/src/migrations/legacy_accounts.rs index eb39131e98..e51de6677d 100644 --- a/examples/demo-rollup/src/migrations/legacy_accounts.rs +++ b/examples/demo-rollup/src/migrations/legacy_accounts.rs @@ -12,10 +12,8 @@ use std::path::PathBuf; use anyhow::{bail, Context}; use clap::Parser; use demo_stf::runtime::Runtime; -use rockbound::SchemaBatch; use serde::Serialize; use sov_db::ledger_db::LedgerDb; -use sov_db::schema::tables::SlotByNumber; use sov_db::storage_manager::NomtStorageManager; use sov_full_node_configs::runner::from_toml_path; use sov_mock_da::storable::StorableMockDaService; @@ -24,12 +22,18 @@ use sov_modules_api::capabilities::HasKernel; use sov_modules_api::execution_mode::Native; use sov_modules_api::{ModuleInfo, Spec, StateCheckpoint, StateWriter}; use sov_modules_rollup_blueprint::RollupBlueprint; -use sov_rollup_interface::common::SlotNumber; use sov_state::{Kernel, NativeStorage, Prefix, SlotKey, StateUpdate}; use sov_stf_runner::RollupConfig; use sov_demo_rollup::MockDemoRollup; +#[path = "common.rs"] +mod common; +use common::{ + assert_ledger_head_state_root_matches_storage_root, + assert_storage_latest_version_matches_ledger_head, make_ledger_root_patch, +}; + #[derive(Parser, Debug)] #[command( author, @@ -104,15 +108,11 @@ fn run() -> anyhow::Result<()> { sov_accounts::migrations::collect_legacy_account_entries(&runtime.accounts, &storage) .context("failed to collect legacy accounts entries")?; - // The historical-state DB enforces that any version touching the User - // namespace also touches the Kernel namespace (see - // `sov-db/src/historical_state.rs` "User namespace got updated without - // kernel namespace"). Our migration only writes User entries, so we - // round-trip the kernel `chain_state.true_slot_number` value to register - // a state-root-neutral kernel write. NOMT roots are pure functions of - // (key, value) pairs, so writing the same bytes back to the same key - // leaves the post-migration root identical to what the user-only diff - // would produce. + // The historical-state DB rejects User-namespace writes that don't also touch + // Kernel (sov-db/src/historical_state.rs). Round-trip + // `chain_state.true_slot_number` to satisfy the invariant — NOMT roots are + // pure functions of (key, value) pairs, so re-writing the same bytes is + // state-root-neutral. let true_slot_key = SlotKey::singleton(&Prefix::new( runtime.chain_state.discriminant(), sov_chain_state::ChainState::::TRUE_SLOT_NUMBER_ITEM_DISCRIMINANT, @@ -137,7 +137,7 @@ fn run() -> anyhow::Result<()> { .context("failed to round-trip kernel value to satisfy historical-state invariant")?; let (next_state_root, mut state_update, accessory_delta, _witness, storage_after) = - checkpoint.materialize_update(pre_state_root.clone()); + checkpoint.materialize_update(pre_state_root); state_update.add_accessory_items(accessory_delta.freeze()); let change_set = storage_after.materialize_changes_at_version(state_update, head_slot_number); @@ -212,57 +212,3 @@ fn load_storage_config(args: &Args) -> anyhow::Result( - storage: &S, - ledger_db: &LedgerDb, - phase: &str, -) -> anyhow::Result { - let (head_slot_number, _head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; - let storage_latest_version = storage.latest_version(); - if storage_latest_version != head_slot_number { - bail!( - "{phase} invariant failed: storage.latest_version ({}) != ledger head slot ({})", - storage_latest_version, - head_slot_number - ); - } - Ok(head_slot_number) -} - -fn assert_ledger_head_state_root_matches_storage_root( - storage: &S, - ledger_db: &LedgerDb, - phase: &str, -) -> anyhow::Result<()> { - let (head_slot_number, head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; - let storage_root = storage - .get_root_hash(head_slot_number) - .context("failed to read storage root at ledger head slot")?; - if head_slot.state_root.as_ref() != storage_root.as_ref() { - bail!( - "{phase} invariant failed: ledger head state_root does not match storage root at slot {}", - head_slot_number - ); - } - Ok(()) -} - -fn make_ledger_root_patch( - ledger_db: &LedgerDb, - new_state_root: &[u8], -) -> anyhow::Result { - let (head_slot_number, mut head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot patch state root"))?; - - head_slot.state_root = new_state_root.to_vec().into(); - - let mut batch = SchemaBatch::new(); - batch.put::(&head_slot_number, &head_slot)?; - Ok(batch) -} diff --git a/examples/demo-rollup/src/migrations/mockda_to_celestia.rs b/examples/demo-rollup/src/migrations/mockda_to_celestia.rs index f696c90540..85683dbcdd 100644 --- a/examples/demo-rollup/src/migrations/mockda_to_celestia.rs +++ b/examples/demo-rollup/src/migrations/mockda_to_celestia.rs @@ -14,7 +14,7 @@ use sov_celestia_adapter::types::TmHash; use sov_celestia_adapter::verifier::address::CelestiaAddress; use sov_db::config::RollupDbConfig; use sov_db::ledger_db::LedgerDb; -use sov_db::schema::tables::{BatchByNumber, SlotByNumber}; +use sov_db::schema::tables::BatchByNumber; use sov_db::schema::types::{BatchNumber, DbBytes, StoredBatch}; use sov_db::storage_manager::NomtStorageManager; use sov_full_node_configs::runner::from_toml_path; @@ -35,6 +35,13 @@ use sov_stf_runner::RollupConfig; use sov_demo_rollup::{CelestiaDemoRollup, MockDemoRollup}; +#[path = "common.rs"] +mod common; +use common::{ + assert_ledger_head_state_root_matches_storage_root, + assert_storage_latest_version_matches_ledger_head, make_ledger_root_patch, +}; + #[derive(Parser, Debug)] #[command( author, @@ -1220,21 +1227,6 @@ fn select_source_sequencer( Ok(only.clone()) } -fn make_ledger_root_patch( - ledger_db: &LedgerDb, - new_state_root: &[u8], -) -> anyhow::Result { - let (head_slot_number, mut head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot patch state root"))?; - - head_slot.state_root = new_state_root.to_vec().into(); - - let mut batch = SchemaBatch::new(); - batch.put::(&head_slot_number, &head_slot)?; - Ok(batch) -} - fn make_batch_receipt_patch( ledger_db: &LedgerDb, sequencer_plan: &ResolvedSequencerPlan, @@ -1355,44 +1347,3 @@ fn migrate_stored_batch_receipt( ); Ok(BatchReceiptMigrationOutcome::NeedsRewrite(batch)) } - -fn assert_storage_latest_version_matches_ledger_head( - storage: &S, - ledger_db: &LedgerDb, - phase: &str, -) -> anyhow::Result { - let (head_slot_number, _head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; - let storage_latest_version = storage.latest_version(); - if storage_latest_version != head_slot_number { - bail!( - "{phase} invariant failed: storage.latest_version ({}) != ledger head slot ({})", - storage_latest_version, - head_slot_number - ); - } - - Ok(head_slot_number) -} - -fn assert_ledger_head_state_root_matches_storage_root( - storage: &S, - ledger_db: &LedgerDb, - phase: &str, -) -> anyhow::Result<()> { - let (head_slot_number, head_slot) = ledger_db - .get_head_slot()? - .ok_or_else(|| anyhow::anyhow!("ledger has no head slot; cannot migrate an empty DB"))?; - let storage_root = storage - .get_root_hash(head_slot_number) - .context("failed to read storage root at ledger head slot")?; - if head_slot.state_root.as_ref() != storage_root.as_ref() { - bail!( - "{phase} invariant failed: ledger head state_root does not match storage root at slot {}", - head_slot_number - ); - } - - Ok(()) -} From 8377e6df2078a5b78fcec00bb48c98ef14df66fa Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Tue, 5 May 2026 15:08:39 +0200 Subject: [PATCH 20/21] Fixes --- .../sov-accounts/README.md | 16 ++++++ .../sov-accounts/src/query.rs | 55 ++++++++++++++++--- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index b7d5c6d6c7..5bc6bbcccb 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -75,6 +75,22 @@ The binary requires a stopped node (the storage manager opens the DB exclusively The reported `pre_state_root` and `post_state_root` are written in JSON; verify the post-root matches what the rollup loads on restart. +### Behavior change: canonical-address authorization is no longer suppressed + +A pre-migration `accounts[C] = A` row was authoritative: `is_authorized_for(X, C)` returned strictly +`A == X` and bypassed the canonical-address fallback. The refactored `is_authorized_for` returns +`true` whenever `X == credential_id.into::()` or `account_owners[(X, C)] = true`, with +no suppression. + +The migration converts each `accounts[C] = A` row into `account_owners[(A, C)] = true`. For any +legacy row where `A != canonical(credential_id)`, the credential gains authorization to act as +`canonical(credential_id)` after the migration, in addition to keeping authorization for `A`. The +canonical fallback is computed, not stored, so it cannot be revoked through `account_owners`. + +Operators should treat each migrated entry as also implicitly authorizing +`credential_id.into::()`. If that address holds assets or permissions whose security +relied on the legacy exclusive semantic, retire the credential before deploying the new binary. + The migration requires NOMT prefix iteration; JMT-backed deployments are not supported. For non-demo rollups, copy `examples/demo-rollup/src/migrations/legacy_accounts.rs` diff --git a/crates/module-system/module-implementations/sov-accounts/src/query.rs b/crates/module-system/module-implementations/sov-accounts/src/query.rs index 9a4d10a548..80e8807900 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/query.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/query.rs @@ -8,13 +8,13 @@ use sov_modules_api::rest::utils::{errors, ApiResult, Path}; use sov_modules_api::rest::{ApiState, HasCustomRestApi}; use sov_modules_api::{ApiStateAccessor, CredentialId, Spec}; -use crate::{AccountOwnerKey, Accounts}; +use crate::Accounts; /// Response of `GET /authorizations/{address}/{credential_id}`. #[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] pub struct AuthorizationResponse { - /// `true` iff `(address, credential_id)` has an explicit `account_owners` - /// entry. Does not include the canonical-address fallback. + /// `true` iff `credential_id` is authorized to act as `address`, + /// including the canonical-address fallback. pub authorized: bool, } @@ -38,10 +38,8 @@ impl Accounts { })?; let authorized = state - .account_owners - .get(&AccountOwnerKey::new(address, credential_id), &mut accessor) - .unwrap_infallible() - .unwrap_or(false); + .is_authorized_for(&address, &credential_id, &mut accessor) + .unwrap_infallible(); Ok(AuthorizationResponse { authorized }.into()) } } @@ -58,3 +56,46 @@ impl HasCustomRestApi for Accounts { .with_state(state.with(self.clone())) } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use sov_modules_api::capabilities::mocks::MockKernel; + use sov_modules_api::rest::utils::Path; + use sov_modules_api::{ConcurrentStateCheckpoint, StateCheckpoint}; + use sov_test_utils::storage::SimpleStorageManager; + use sov_test_utils::TestSpec; + + use super::*; + + type S = TestSpec; + + #[test] + fn route_includes_canonical_fallback() { + let kernel = Arc::new(MockKernel::::default()); + let storage = SimpleStorageManager::new().create_storage(); + let checkpoint = Arc::new(ConcurrentStateCheckpoint::from_state_checkpoint( + StateCheckpoint::::new(storage, kernel.as_ref(), None), + )); + let (_sender, receiver) = sov_modules_api::prelude::tokio::sync::watch::channel(checkpoint); + + let accounts = Accounts::::default(); + let state = ApiState::build(Arc::new(()), receiver, kernel, None).with(accounts); + let accessor = state.default_api_state_accessor(); + + let credential_id = CredentialId::from([7u8; 32]); + let address = ::Address::from(credential_id); + + let response = sov_modules_api::prelude::tokio::runtime::Runtime::new() + .unwrap() + .block_on(Accounts::::route_is_authorized( + state, + accessor, + Path((address.to_string(), credential_id.to_string())), + )) + .unwrap(); + + assert!(response.0.authorized); + } +} From 00beac9db12a93376eb8c8583385493f7c0d90af Mon Sep 17 00:00:00 2001 From: Nikolai Golub Date: Wed, 6 May 2026 10:56:17 +0200 Subject: [PATCH 21/21] Address README.md comments --- .../sov-accounts/README.md | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/crates/module-system/module-implementations/sov-accounts/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index 5bc6bbcccb..f3cf64be8a 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -14,9 +14,6 @@ addresses and records which credentials may act for which addresses. ## Credential and Address Relations -The module has two credential/address relations. They answer different -questions and should not be treated as interchangeable. - ### Stateless canonical address ```text @@ -42,19 +39,11 @@ New `InsertCredentialId` calls write this relation. Callers that need to verify whether a known address may be used with a credential should use `is_authorized_for`, which checks the stateless canonical address and `account_owners`. -## Legacy `accounts` map - -The module struct retains a tombstoned `accounts: StateMap` field -purely to preserve the macro-derived `#[state]` discriminant ordering — it is the first -state field, so removing it would shift the discriminants of every following field and -corrupt their on-disk data. The field is `pub(crate)`, never read or written outside -[`migrations`](src/migrations.rs), and empty after the legacy-accounts migration runs. - ## Upgrade procedure for chains with legacy `accounts` entries -Chains created before the layer-1 reads were dropped may have entries in `accounts`. -Those entries need to be moved to `account_owners` before deploying the new binary, -otherwise the credentials they encode will silently lose their authorization. +The module used to have a separate `accounts` mapping with different semantics, now deprecated and unused. +Chains whose genesis was before the `accounts` deprecation need to have a migration run at the upgrade height +(including whenever resyncing from genesis). The migration ships as a CLI binary in `examples/demo-rollup`: