feat: introduce owner rotation for Safe account #90

paolodamico · 2025-08-27T00:33:52Z

Changes

Enables key rotation by calling swapOwner on the Safe contract to rotate the owner of the Safe.
Refactors as_execute_user_op_call_data in the Is4337Encodable as for types of transactions this code will be exactly the same.
Introduces an on-chain integration test to swap the Safe owner and a generic mock RPC HTTP client for other integration tests.

⚠️ Breaking Changes

Renames transaction_transfer to tx_transfer. Less verbosity, especially as we introduce other types of transactions.

🔜 Changes coming soon

Rotating owners in Optimism too

Notes

Tests assisted by AI (manually reviewed)

Copilot

Pull Request Overview

This PR introduces owner rotation functionality for Safe smart accounts by implementing a new tx_swap_safe_owner method that enables key rotation through the Safe contract's swapOwner function. The implementation includes comprehensive integration tests and refactors existing code for better reusability.

Adds Safe owner swap functionality with SafeOwner transaction type and tx_swap_safe_owner method
Refactors Is4337Encodable trait to provide shared as_execute_user_op_call_data implementation
Renames transaction_transfer to tx_transfer for consistency and brevity

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
bedrock/src/transaction/mod.rs	Exports SafeOwner and adds tx_swap_safe_owner method to SafeSmartAccount
bedrock/src/transaction/contracts/safe_owner.rs	Implements SafeOwner struct and Is4337Encodable trait for owner swap functionality
bedrock/src/transaction/contracts/mod.rs	Adds safe_owner module export
bedrock/src/transaction/contracts/erc20.rs	Refactors to use new Is4337Encodable trait methods
bedrock/src/smart_account/transaction_4337.rs	Refactors Is4337Encodable trait with shared implementation
bedrock/src/smart_account/nonce.rs	Adds SwapOwner transaction type ID
bedrock/tests/test_smart_account_safe_owner_swap.rs	Comprehensive integration test for Safe owner swap functionality
bedrock/tests/common.rs	Adds AnvilBackedHttpClient mock for testing 4337 operations
bedrock/tests/test_smart_account_transfer.rs	Updates method name from transaction_transfer to tx_transfer

bedrock/src/smart_account/nonce.rs

karankurbur · 2025-08-29T00:56:09Z

bedrock/src/transaction/contracts/safe_owner.rs

+sol! {
+    ///Owner Manager Interface for the Safe
+    ///
+    /// Reference: <https://github.com/safe-global/safe-smart-account/blob/v1.4.1/contracts/base/OwnerManager.sol>


I'm assuming there is no difference here between 1.3.0 and 1.4.1 OwnerManager? Just wanted to ask

karankurbur · 2025-08-29T00:56:43Z

bedrock/src/transaction/contracts/safe_owner.rs

+
+/// Represents a Safe owner swap transaction for key rotation.
+pub struct SafeOwner {
+    /// The inner call data for the ERC-20 `transferCall` function.


comment is wrong

karankurbur · 2025-08-29T00:57:20Z

bedrock/src/transaction/contracts/safe_owner.rs

+    /// - `new_owner`: The new owner to replace the old owner
+    #[must_use]
+    pub fn new(
+        wallet_address: Address,


lets use safe_address, wallet_address is unclear

karankurbur · 2025-08-29T00:58:10Z

bedrock/src/transaction/contracts/safe_owner.rs

+    },
+};
+
+const SENTINEL_ADDRESS: Address =


Can you add a doc here on what this magic address is

karankurbur · 2025-08-29T01:00:08Z

bedrock/src/transaction/contracts/safe_owner.rs

+    ) -> Self {
+        Self {
+            call_data: IOwnerManager::swapOwnerCall {
+                prev_owner: SENTINEL_ADDRESS,


is this always the case? I'm not familiar with this logic in the contract

What if a user rotates for the 2nd/3rd time

karankurbur · 2025-08-29T01:01:25Z

bedrock/src/transaction/contracts/erc20.rs

-        }
-        .abi_encode()
-        .into()
+    fn target_address(&self) -> Address {


Better to define these fns on the Is4337Encodable instead of duplicating inside each impl?

karankurbur · 2025-08-29T01:02:47Z

bedrock/src/transaction/mod.rs

    /// - Will throw an RPC error if the transaction submission fails.
    /// - Will throw an error if the global HTTP client has not been initialized.
-    pub async fn transaction_transfer(
+    pub async fn tx_transfer(


nit: lets remove tx_ prefix? Its already on the safe_account object

karankurbur · 2025-08-29T01:03:07Z

bedrock/src/transaction/mod.rs

+    /// # Errors
+    /// - Will throw a parsing error if any of the provided attributes are invalid.
+    /// - Will throw an RPC error if the transaction submission fails.
+    pub async fn tx_swap_safe_owner(


nit: swap_owner?

karankurbur · 2025-08-29T01:03:40Z

bedrock/src/transaction/mod.rs

+    /// This is used to allow key rotation. The EOA signer that can act on behalf of the Safe is rotated.
+    ///
+    /// # Arguments
+    /// - `old_owner`: The EOA of the old owner (address).


we can fetch this from on-chain instead of having to take in as input

do we need anything to enforce that this only done for world app safes? Ie check that threshold/signers length is 1

bedrock/tests/test_smart_account_safe_owner_swap.rs

karankurbur · 2025-08-29T01:09:42Z

bedrock/tests/test_smart_account_safe_owner_swap.rs

+
+    // Create SafeSmartAccount instance
+    let safe_account =
+        SafeSmartAccount::new(initial_owner_key_hex, &safe_address.to_string())


Can we make the parameter be Address instead of hex?

* refactors * improvements * clarify for tests

cursor · 2025-08-29T01:25:36Z

bedrock/src/transaction/contracts/safe_owner.rs

+                prev_owner: SENTINEL_ADDRESS,
+                old_owner,
+                new_owner,
+            }


Bug: Incorrect prev_owner Handling in SafeOwner::new

The SafeOwner::new function hardcodes prev_owner to SENTINEL_ADDRESS (0x1) for the swapOwner call. This is only correct for single-owner Safes or the first owner swap. For multi-owner Safes or subsequent rotations, prev_owner must be the actual preceding owner, causing transactions to fail on-chain.

paolodamico added 5 commits August 26, 2025 16:47

first I

08dadd0

nice refactor

69a923e

tests stash

72f84d4

e2e tests

a9472ba

Merge branch 'main' into pd/swap-owner

aebdff9

paolodamico requested review from bergusman and karankurbur August 28, 2025 21:06

paolodamico added 2 commits August 28, 2025 14:15

fix doctests

9aff5fc

Update mod.rs

08c403d

paolodamico marked this pull request as ready for review August 28, 2025 21:34

paolodamico requested review from marioradonic, tomislavhoman, lukejmann, andy-t-wang, aurel-fr and murph as code owners August 28, 2025 21:34

This comment was marked as outdated.

Sign in to view

oops

eaafadc

This comment was marked as outdated.

Sign in to view

paolodamico requested a review from Copilot August 28, 2025 21:42

Copilot AI reviewed Aug 28, 2025

View reviewed changes

bedrock/src/smart_account/nonce.rs Show resolved Hide resolved

karankurbur reviewed Aug 29, 2025

View reviewed changes

bedrock/tests/test_smart_account_safe_owner_swap.rs Show resolved Hide resolved

karankurbur reviewed Aug 29, 2025

View reviewed changes

chore: refactor entrypoint & related contracts (#92)

82ba7ab

* refactors * improvements * clarify for tests

cursor bot reviewed Aug 29, 2025

View reviewed changes

feat: introduce owner rotation for Safe account #90

Are you sure you want to change the base?

feat: introduce owner rotation for Safe account #90

Uh oh!

Conversation

paolodamico commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

⚠️ Breaking Changes

🔜 Changes coming soon

Notes

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karankurbur Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Aug 29, 2025

Choose a reason for hiding this comment

Bug: Incorrect prev_owner Handling in SafeOwner::new

Uh oh!

Uh oh!

paolodamico commented Aug 27, 2025 •

edited

Loading

karankurbur Aug 29, 2025 •

edited

Loading

Bug: Incorrect `prev_owner` Handling in `SafeOwner::new`