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..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]; - let embedded = [2u8; 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 account definently doesnt already exist + // 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)); @@ -60,15 +64,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 +83,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 +96,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/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/README.md b/crates/module-system/module-implementations/sov-accounts/README.md index 08b73c9c37..c5e4f66632 100644 --- a/crates/module-system/module-implementations/sov-accounts/README.md +++ b/crates/module-system/module-implementations/sov-accounts/README.md @@ -1,15 +1,77 @@ # `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. 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 add new credential to a given address using the `CallMessage::InsertCredentialId(..)` message. +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 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 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. + +### 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 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. + +### 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. 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`. 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..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,20 +1,21 @@ -use anyhow::bail; -use anyhow::{anyhow, 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}; 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 `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 new credential id. + /// The credential id being authorized. CredentialId, ), } @@ -25,36 +26,39 @@ 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)?; + self.exit_if_credential_exists(&new_credential_id, context.sender(), 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::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) + .context("Failed to read account owner")? + .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 8d84fc04d4..30de1f7103 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,89 @@ 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. + /// 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.account_owners.set( + &AccountOwnerKey::new(*address, *credential_id), + &true, + 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, 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) - } + if let Some(account) = self.accounts.get(credential_id, state)? { + return Ok(account.addr); } + Ok(*default_address) } - /// Resolve the sender's public key to an address. + /// Read-only variant of [`Self::resolve_sender_address`]: returns + /// `default_address` when the credential has no legacy/custom mapping, + /// 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 let Some(account) = self.accounts.get(credential_id, state)? { + return Ok(account.addr); } + Ok(*default_address) + } + + /// Returns `true` if `credential_id` has an explicit `account_owners` + /// authorization for `address`. + 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()) + } + + /// 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, + 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/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 5bf6eefaec..5d18e27beb 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -4,17 +4,17 @@ 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. +/// 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 `CredentailId` => `Account` mapping. + /// Enable configured credential authorizations and `InsertCredentialId`. #[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.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 6bfe8bbda0..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,11 +30,57 @@ use sov_modules_api::{ Clone, )] pub struct Account { - /// The address of the account. + /// The mapped address. pub addr: S::Address, } -/// A module responsible for managing accounts on the rollup. +/// 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 { + address: S::Address, + credential_id: CredentialId, +} + +impl AccountOwnerKey { + pub(crate) fn new(address: S::Address, credential_id: CredentialId) -> Self { + Self { + address, + credential_id, + } + } +} + +// `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) + } +} + +impl std::str::FromStr for AccountOwnerKey { + type Err = anyhow::Error; + fn from_str(s: &str) -> Result { + let (addr_str, cred_str) = s + .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:?}"))?, + }) + } +} + +/// 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 { @@ -42,13 +88,20 @@ pub struct Accounts { #[id] pub id: ModuleId, - /// Mapping from a credential to its corresponding account. + /// Legacy/custom `credential_id -> address` routing index. New + /// authorization writes use [`Self::account_owners`] instead. #[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, + + /// Authorization set: a present entry means `credential_id` may sign as + /// `address`. + #[state] + pub(crate) account_owners: StateMap, bool>, } impl Module for Accounts { 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..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,17 +11,17 @@ 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, } 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 b4b0d42138..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,15 +74,19 @@ fn test_config_account() { runner, ) = setup(); - // The account is registered at genesis. + // 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(); - let response = accounts.get_account(user.credential_id(), 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!( - response, - Response::AccountExists { - addr: user.address() - } + accounts.get_account(user.credential_id(), state), + Response::AccountEmpty ); }); } @@ -107,19 +112,28 @@ fn test_update_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. + // 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()); assert_eq!( accounts.get_account(new_credential, state), - Response::AccountExists { - addr: user.address() - } + Response::AccountEmpty ); - // Account corresponding to the old credential still exists. + // 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()); @@ -127,45 +141,53 @@ fn test_update_account() { }); } +/// A credential already authorized for an address cannot be inserted twice. #[test] -fn test_update_account_fails() { +fn test_insert_existing_credential_fails() { let ( TestData { - account_1, - account_2, + non_registered_account: sender, .. }, mut runner, ) = setup(); + let new_credential = TestPrivateKey::generate().pub_key().credential_id(); + runner.execute_transaction(TransactionTestCase { - input: account_1.create_plain_message::>(CallMessage::InsertCredentialId( - account_2.credential_id(), + input: sender.create_plain_message::>(CallMessage::InsertCredentialId( + new_credential, )), assert: Box::new(move |result, _state| { - if let TxEffect::Reverted(contents) = result.tx_receipt { + assert!(result.tx_receipt.is_successful()); + }), + }); + + runner.execute_transaction(TransactionTestCase { + input: sender.create_plain_message::>(CallMessage::InsertCredentialId( + 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"), }), }); } /// Tests the multisig functionality of the Accounts module. +/// +/// Seeds genesis with a `TestUser` whose custom `credential_id` matches the +/// 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; - let ( - TestData { - non_registered_account: user, - .. - }, - mut runner, - ) = setup(); - // First, create and register a multisig let multisig_keys = [ TestPrivateKey::generate(), TestPrivateKey::generate(), @@ -174,33 +196,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 canonical address. + 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 @@ -383,13 +387,29 @@ 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| { let accounts = Accounts::::default(); - let response = accounts.get_account(non_registered_account.credential_id(), state); - assert_eq!(response, Response::AccountEmpty); + assert_eq!( + 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()); + 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(); @@ -403,20 +423,37 @@ fn test_register_new_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. + // The new credential is authorized for the sender's address. + 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 default credential of the account exists + // 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()); @@ -446,9 +483,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-authorized credentials have no `accounts` entry, so credential-only +/// resolution falls back to the supplied address. #[test] fn test_resolve_sender_address_registered() { let ( @@ -463,19 +513,29 @@ 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 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() ); + 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()); }); } -/// Tests what happens if one tries to resolve an address when there is more than one credential available. +/// 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_if_more_than_one_credential() { +fn test_resolve_address_with_multi_credential_ownership() { let ( TestData { non_registered_account, @@ -484,19 +544,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)), @@ -504,27 +558,42 @@ fn test_resolve_address_if_more_than_one_credential() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); + let addr = non_registered_account.address(); assert_eq!( accounts - .resolve_sender_address(&default_address_1, &credential_1, state) + .resolve_sender_address(&addr, &credential_1, state) .unwrap(), - non_registered_account.address() + addr ); - assert_eq!( accounts - .resolve_sender_address(&default_address_2, &credential_2, state) + .resolve_sender_address(&addr, &credential_2, state) .unwrap(), - non_registered_account.address() + addr + ); + + 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::AccountEmpty + ); + assert_eq!( + accounts.get_account(credential_2, state), + Response::AccountEmpty ); }); } -/// 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 -/// query_visible_state doesn't mutate the state so it simply verifies that the fallback address is -/// returned correctly +/// 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(); @@ -540,6 +609,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 + ); }); } 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..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 `CredentailId` => `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 f2405b3954..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": "Inserts a new credential id for the corresponding Account.", + "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"