Accounts Refactor PR 3: Addinng more call messages to accounts module

CHANGELOG.md

@@ -21,6 +21,7 @@
 
 # MULTISIG UPGRADE
 Temporary section for maintaining breaking changes from individual PRs, which will be consolidated into a single changelog entry when the feature branch is merged into `dev`.
+- **Breaking change (code, wire, consensus)**: `sov_accounts::CallMessage` gains three variants — `AddCredentialToAddress { address, credential }`, `RemoveCredentialFromAddress { address, credential }`, and `RotateCredentialOnAddress { address, old_credential, new_credential }` — and is now generic in `S: Spec`. The enum's wire format changes, which changes the chain hash. All three new variants require `context.sender() == address` (callers can only modify credentials on the address they are currently signing as); `InsertCredentialId` semantics are unchanged. `Rotate` is functionally equivalent to a `Remove` + `Add` collapsed into a single call for key rotation. There is no orphan guard on remove.
 - #2688 **Breaking change(code, state, consensus)**: The transaction formats have changed in this version. This is a consensus breaking change and requires coordinating an upgrade using `--stop-at-rollup-height`, and using sov-rollup-manager for full resyncs from this point onwards for existing rollups. This fixes Credential ID malleability that allowed the same set of signed bytes to be replayed for different credentials, across V0 and V1 (multisig) transactions and with different V1 multisig parameters.
   * The on-chain transaction data has changed, and multisig transactions now explicitly include the multisig id as part of the signed bytes.
   * Previous signatures are no longer valid. Clients will need to upgrade to our latest SDKs to be able to sign transactions in the new format.

crates/fuzz/fuzz_targets/accounts_call_random.rs

@@ -9,9 +9,10 @@ use sov_test_utils::storage::SimpleStorageManager;
 use sov_test_utils::TestStorageSpec;
 
 type S = sov_test_utils::TestSpec;
+type FuzzInput<'a> = (&'a [u8], Vec<(Context<S>, CallMessage<S>)>);
 
 // Check arbitrary, random calls
-fuzz_target!(|input: (&[u8], Vec<(Context<S>, CallMessage)>)| {
+fuzz_target!(|input: FuzzInput| {
     let storage_manager = SimpleStorageManager::<TestStorageSpec>::new();
     let storage = storage_manager.create_storage();
     let mut state = StateCheckpoint::new(storage, &MockKernel::<S>::default(), None);

crates/fuzz/fuzz_targets/accounts_parse_call_message.rs

@@ -3,8 +3,10 @@
 use libfuzzer_sys::fuzz_target;
 use sov_accounts::CallMessage;
 
-fuzz_target!(|input: CallMessage| {
+type S = sov_test_utils::TestSpec;
+
+fuzz_target!(|input: CallMessage<S>| {
     let json = serde_json::to_vec(&input).unwrap();
-    let msg = serde_json::from_slice::<CallMessage>(&json).unwrap();
+    let msg = serde_json::from_slice::<CallMessage<S>>(&json).unwrap();
     assert_eq!(input, msg);
 });

crates/fuzz/fuzz_targets/accounts_parse_call_message_random.rs

@@ -3,6 +3,8 @@
 use libfuzzer_sys::fuzz_target;
 use sov_accounts::CallMessage;
 
+type S = sov_test_utils::TestSpec;
+
 fuzz_target!(|input: &[u8]| {
-    serde_json::from_slice::<CallMessage>(input).ok();
+    serde_json::from_slice::<CallMessage<S>>(input).ok();
 });

crates/module-system/module-implementations/sov-accounts/README.md

@@ -16,6 +16,19 @@ addresses and records which credentials may act for which addresses.
    the `CallMessage::InsertCredentialId(..)` message. This writes an
    `account_owners` authorization, not a credential-indexed account entry.
 
+1. It is possible to explicitly authorize a credential for the caller's own
+   address with `CallMessage::AddCredentialToAddress { address, credential }`,
+   revoke such an authorization with
+   `CallMessage::RemoveCredentialFromAddress { address, credential }`, and
+   atomically swap one credential for another with
+   `CallMessage::RotateCredentialOnAddress { address, old_credential, new_credential }`.
+   All three calls require `message.address == context.sender()`: callers can
+   only modify credentials on the address they are currently signing as. The
+   V1 signing path's `target_address` field lets a caller signing with a
+   credential authorized for multiple addresses select which one to act as.
+   There is no orphan guard on remove — revoking the last credential leaves
+   the address unspendable via `account_owners`.
+
 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.
@@ -56,13 +69,14 @@ authorization has a stored account entry.
 ### Account-credential authorization map
 
 ```text
-account_owners[(address, credential_id)] = true
+account_owners[(address, credential_id)] = true | false
 ```
 
-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 overrides. `true` means the credential is
+authorized to sign transactions that execute as the given address. `false`
+explicitly revokes fallback authorization for that pair, including stateless
+canonical fallback. 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,

crates/module-system/module-implementations/sov-accounts/src/call.rs

@@ -1,4 +1,4 @@
-use anyhow::{bail, Context as _};
+use anyhow::{anyhow, bail, Context as _};
 use schemars::JsonSchema;
 use sov_modules_api::macros::{serialize, UniversalWallet};
 use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState};
@@ -9,15 +9,55 @@ 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)]
+#[schemars(bound = "S::Address: ::schemars::JsonSchema", rename = "CallMessage")]
 #[serde(rename_all = "snake_case")]
-pub enum CallMessage {
+pub enum CallMessage<S: Spec> {
     /// 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 authorized.
         CredentialId,
     ),
+
+    /// Authorizes `credential` to sign transactions that execute as `address`.
+    /// The caller must currently be signing as `address`. Fails if the tuple
+    /// is already authorized.
+    AddCredentialToAddress {
+        /// The address whose credential set is being extended. Must equal
+        /// `context.sender()`.
+        address: S::Address,
+        /// The credential being authorized for `address`.
+        credential: CredentialId,
+    },
+
+    /// Revokes `credential` from `address`. The caller must currently be
+    /// signing as `address`. No orphan guard: revoking the last credential
+    /// succeeds and leaves `address` unspendable via this map.
+    RemoveCredentialFromAddress {
+        /// The address whose credential set is being reduced. Must equal
+        /// `context.sender()`.
+        address: S::Address,
+        /// The credential being revoked from `address`.
+        credential: CredentialId,
+    },
+
+    /// Atomically swaps `old_credential` for `new_credential` on `address`.
+    /// Functionally equivalent to a `RemoveCredentialFromAddress` followed by
+    /// an `AddCredentialToAddress`, collapsed into a single call so the
+    /// caller does not have to authorize two transactions during a rotation.
+    /// The caller must currently be signing as `address`.
+    RotateCredentialOnAddress {
+        /// The address whose credential set is being rotated. Must equal
+        /// `context.sender()`.
+        address: S::Address,
+        /// The credential being revoked from `address`. Must be currently
+        /// authorized.
+        old_credential: CredentialId,
+        /// The credential being authorized for `address`. Must not already
+        /// be authorized.
+        new_credential: CredentialId,
+    },
 }
 
 impl<S: Spec> Accounts<S> {
@@ -27,18 +67,124 @@ impl<S: Spec> Accounts<S> {
         context: &Context<S>,
         state: &mut impl TxState<S>,
     ) -> 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.ensure_custom_account_mappings_enabled(state)?;
 
         self.exit_if_credential_exists(&new_credential_id, context.sender(), state)?;
 
         self.authorize_credential(context.sender(), &new_credential_id, state)?;
         Ok(())
     }
 
+    pub(crate) fn add_credential_to_address(
+        &mut self,
+        address: S::Address,
+        credential: CredentialId,
+        context: &Context<S>,
+        state: &mut impl TxState<S>,
+    ) -> anyhow::Result<()> {
+        self.ensure_custom_account_mappings_enabled(state)?;
+        self.ensure_caller_owns(&address, context)?;
+        self.ensure_credential_not_authorized(&address, &credential, state)?;
+
+        self.authorize_credential(&address, &credential, state)?;
+        Ok(())
+    }
+
+    pub(crate) fn remove_credential_from_address(
+        &mut self,
+        address: S::Address,
+        credential: CredentialId,
+        context: &Context<S>,
+        state: &mut impl TxState<S>,
+    ) -> anyhow::Result<()> {
+        self.ensure_custom_account_mappings_enabled(state)?;
+        self.ensure_caller_owns(&address, context)?;
+
+        anyhow::ensure!(
+            self.is_authorized_for(&address, &credential, state)
+                .map_err(|err| anyhow!("Error raised while checking authorization: {err:?}"))?,
+            "CredentialId is not authorized for this address"
+        );
+
+        self.revoke_credential(&address, &credential, state)?;
+        Ok(())
+    }
+
+    pub(crate) fn rotate_credential_on_address(
+        &mut self,
+        address: S::Address,
+        old_credential: CredentialId,
+        new_credential: CredentialId,
+        context: &Context<S>,
+        state: &mut impl TxState<S>,
+    ) -> anyhow::Result<()> {
+        self.ensure_custom_account_mappings_enabled(state)?;
+        self.ensure_caller_owns(&address, context)?;
+
+        anyhow::ensure!(
+            self.is_authorized_for(&address, &old_credential, state)
+                .map_err(|err| anyhow!("Error raised while checking authorization: {err:?}"))?,
+            "CredentialId is not authorized for this address"
+        );
+        self.ensure_credential_not_authorized(&address, &new_credential, state)?;
+
+        self.revoke_credential(&address, &old_credential, state)?;
+        self.authorize_credential(&address, &new_credential, state)?;
+        Ok(())
+    }
+
+    fn revoke_credential(
+        &mut self,
+        address: &S::Address,
+        credential: &CredentialId,
+        state: &mut impl TxState<S>,
+    ) -> anyhow::Result<()> {
+        let key = AccountOwnerKey::new(*address, *credential);
+        // Write `false` explicitly so a subsequent fallback (legacy
+        // `accounts` mapping or canonical address) cannot re-authorize the
+        // tuple.
+        self.account_owners.set(&key, &false, state)?;
+
+        if self
+            .accounts
+            .get(credential, state)
+            .map_err(|err| anyhow!("Error raised while getting account: {err:?}"))?
+            .is_some_and(|account| account.addr == *address)
+        {
+            self.accounts.delete(credential, state)?;
+        }
+
+        Ok(())
+    }
+
+    fn ensure_custom_account_mappings_enabled(
+        &self,
+        state: &mut impl StateReader<User>,
+    ) -> anyhow::Result<()> {
+        if !self
+            .enable_custom_account_mappings
+            .get(state)
+            .map_err(|err| anyhow!("Error reading enable_custom_account_mappings: {err:?}"))?
+            .expect(
+                "`enable_custom_account_mappings` should not be None; it must be set at genesis.",
+            )
+        {
+            bail!("Custom account mappings are disabled");
+        }
+        Ok(())
+    }
+
+    /// Enforces that the caller is signing as `address`. The upstream
+    /// authorization path has already verified the caller controls a
+    /// credential authorized for `context.sender()`.
+    fn ensure_caller_owns(&self, address: &S::Address, context: &Context<S>) -> anyhow::Result<()> {
+        anyhow::ensure!(
+            context.sender() == address,
+            "Caller is not authorized to modify credentials for this address"
+        );
+        Ok(())
+    }
+
     fn exit_if_credential_exists(
         &self,
         new_credential_id: &CredentialId,
@@ -52,11 +198,22 @@ impl<S: Spec> Accounts<S> {
                 .is_none(),
             "New CredentialId already exists"
         );
+        self.ensure_credential_not_authorized(address, new_credential_id, state)?;
+        Ok(())
+    }
+
+    fn ensure_credential_not_authorized(
+        &self,
+        address: &S::Address,
+        credential: &CredentialId,
+        state: &mut impl StateReader<User>,
+    ) -> anyhow::Result<()> {
         anyhow::ensure!(
-            self.account_owners
-                .get(&AccountOwnerKey::new(*address, *new_credential_id), state)
+            !self
+                .account_owners
+                .get(&AccountOwnerKey::new(*address, *credential), state)
                 .context("Failed to read account owner")?
-                .is_none(),
+                .unwrap_or(false),
             "CredentialId already authorized for this address"
         );
         Ok(())

crates/module-system/module-implementations/sov-accounts/src/capabilities.rs

@@ -49,7 +49,7 @@ impl<S: Spec> Accounts<S> {
     }
 
     /// Returns `true` if `credential_id` has an explicit `account_owners`
-    /// authorization for `address`.
+    /// authorization for `address` via explicit `account_owners` state.
     pub fn is_authorized<ST: StateReader<User>>(
         &self,
         address: &S::Address,
@@ -59,7 +59,7 @@ impl<S: Spec> Accounts<S> {
         Ok(self
             .account_owners
             .get(&AccountOwnerKey::new(*address, *credential_id), state)?
-            .is_some())
+            .unwrap_or(false))
     }
 
     /// Returns `true` if `credential_id` is authorized to act as `address`.
@@ -75,15 +75,17 @@ impl<S: Spec> Accounts<S> {
         credential_id: &CredentialId,
         state: &mut ST,
     ) -> Result<bool, ST::Error> {
-        if let Some(account) = self.accounts.get(credential_id, state)? {
-            return Ok(account.addr == *address);
+        if let Some(is_authorized) = self
+            .account_owners
+            .get(&AccountOwnerKey::new(*address, *credential_id), state)?
+        {
+            return Ok(is_authorized);
         }
 
-        let canonical_address: S::Address = (*credential_id).into();
-        if canonical_address == *address {
-            return Ok(true);
+        if let Some(account) = self.accounts.get(credential_id, state)? {
+            return Ok(account.addr == *address);
         }
 
-        self.is_authorized(address, credential_id, state)
+        Ok(S::Address::from(*credential_id) == *address)
     }
 }

crates/module-system/module-implementations/sov-accounts/src/fuzz.rs

@@ -3,9 +3,28 @@ use sov_modules_api::{CryptoSpec, DaSpec, Module, Spec, StateCheckpoint};
 
 use crate::{Account, AccountConfig, AccountData, Accounts, CallMessage};
 
-impl<'a> Arbitrary<'a> for CallMessage {
+impl<'a, S> Arbitrary<'a> for CallMessage<S>
+where
+    S: Spec,
+    S::Address: Arbitrary<'a>,
+{
     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
-        Ok(Self::InsertCredentialId(u.arbitrary()?))
+        match u.int_in_range(0..=3)? {
+            0 => Ok(Self::InsertCredentialId(u.arbitrary()?)),
+            1 => Ok(Self::AddCredentialToAddress {
+                address: u.arbitrary()?,
+                credential: u.arbitrary()?,
+            }),
+            2 => Ok(Self::RemoveCredentialFromAddress {
+                address: u.arbitrary()?,
+                credential: u.arbitrary()?,
+            }),
+            _ => Ok(Self::RotateCredentialOnAddress {
+                address: u.arbitrary()?,
+                old_credential: u.arbitrary()?,
+                new_credential: u.arbitrary()?,
+            }),
+        }
     }
 }