-
Notifications
You must be signed in to change notification settings - Fork 5
feat: introduce owner rotation for Safe account #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
08dadd0
69a923e
72f84d4
a9472ba
aebdff9
9aff5fc
08c403d
eaafadc
82ba7ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,7 @@ use alloy::{ | |
|
||
use crate::primitives::PrimitiveError; | ||
use crate::smart_account::{ | ||
ISafe4337Module, InstructionFlag, Is4337Encodable, NonceKeyV1, SafeOperation, | ||
TransactionTypeId, UserOperation, | ||
InstructionFlag, Is4337Encodable, NonceKeyV1, TransactionTypeId, UserOperation, | ||
}; | ||
|
||
sol! { | ||
|
@@ -87,16 +86,12 @@ pub struct MetadataArg { | |
impl Is4337Encodable for Erc20 { | ||
type MetadataArg = MetadataArg; | ||
|
||
fn as_execute_user_op_call_data(&self) -> Bytes { | ||
ISafe4337Module::executeUserOpCall { | ||
// The token address | ||
to: self.token_address, | ||
value: U256::ZERO, | ||
data: self.call_data.clone().into(), | ||
operation: SafeOperation::Call as u8, | ||
} | ||
.abi_encode() | ||
.into() | ||
fn target_address(&self) -> Address { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to define these fns on the Is4337Encodable instead of duplicating inside each impl? |
||
self.token_address | ||
} | ||
|
||
fn call_data(&self) -> Bytes { | ||
self.call_data.clone().into() | ||
} | ||
|
||
fn as_preflight_user_operation( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,3 +2,5 @@ | |
//! that power the common transactions for the crypto wallet. | ||
|
||
pub mod erc20; | ||
|
||
pub mod safe_owner; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
//! This module introduces the contract interface for the Safe contract. | ||
//! | ||
//! Explicitly this only allows management of the Safe Smart Account. Executing transactions with the Safe Smart Account | ||
//! is done via the `SafeSmartAccount` module. | ||
use alloy::{ | ||
primitives::{address, Address, Bytes}, | ||
sol, | ||
sol_types::SolCall, | ||
}; | ||
|
||
use crate::{ | ||
primitives::PrimitiveError, | ||
smart_account::{ | ||
InstructionFlag, Is4337Encodable, NonceKeyV1, TransactionTypeId, UserOperation, | ||
}, | ||
}; | ||
|
||
const SENTINEL_ADDRESS: Address = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a doc here on what this magic address is |
||
address!("0x0000000000000000000000000000000000000001"); | ||
|
||
sol! { | ||
///Owner Manager Interface for the Safe | ||
/// | ||
/// Reference: <https://github.com/safe-global/safe-smart-account/blob/v1.4.1/contracts/base/OwnerManager.sol> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming there is no difference here between 1.3.0 and 1.4.1 OwnerManager? Just wanted to ask |
||
#[derive(serde::Serialize)] | ||
#[sol(rename_all = "camelcase")] | ||
interface IOwnerManager { | ||
function swapOwner(address prev_owner, address old_owner, address new_owner) public; | ||
} | ||
} | ||
|
||
/// Represents a Safe owner swap transaction for key rotation. | ||
pub struct SafeOwner { | ||
/// The inner call data for the ERC-20 `transferCall` function. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment is wrong |
||
call_data: Vec<u8>, | ||
/// The address of the Safe Smart Account. | ||
wallet_address: Address, | ||
} | ||
|
||
impl SafeOwner { | ||
/// Creates a new `SafeOwner` transaction for swapping Safe owners. | ||
/// | ||
/// # Arguments | ||
/// - `wallet_address`: The address of the Safe Smart Account | ||
/// - `old_owner`: The current owner to be replaced | ||
/// - `new_owner`: The new owner to replace the old owner | ||
#[must_use] | ||
pub fn new( | ||
wallet_address: Address, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets use |
||
old_owner: Address, | ||
new_owner: Address, | ||
) -> Self { | ||
Self { | ||
call_data: IOwnerManager::swapOwnerCall { | ||
prev_owner: SENTINEL_ADDRESS, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
old_owner, | ||
new_owner, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Incorrect
|
||
.abi_encode(), | ||
wallet_address, | ||
} | ||
} | ||
} | ||
|
||
impl Is4337Encodable for SafeOwner { | ||
type MetadataArg = (); | ||
|
||
fn target_address(&self) -> Address { | ||
self.wallet_address | ||
} | ||
|
||
fn call_data(&self) -> Bytes { | ||
self.call_data.clone().into() | ||
} | ||
|
||
fn as_preflight_user_operation( | ||
&self, | ||
wallet_address: Address, | ||
_metadata: Option<Self::MetadataArg>, | ||
) -> Result<UserOperation, PrimitiveError> { | ||
let call_data = self.as_execute_user_op_call_data(); | ||
|
||
let key = NonceKeyV1::new( | ||
TransactionTypeId::SwapOwner, | ||
InstructionFlag::Default, | ||
[0u8; 10], | ||
); | ||
let nonce = key.encode_with_sequence(0); | ||
|
||
Ok(UserOperation::new_with_defaults( | ||
wallet_address, | ||
nonce, | ||
call_data, | ||
)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ mod contracts; | |
pub mod foreign; | ||
pub mod rpc; | ||
|
||
pub use contracts::safe_owner::SafeOwner; | ||
pub use rpc::{RpcClient, RpcError, RpcProviderName, SponsorUserOperationResponse}; | ||
|
||
/// Errors that can occur when interacting with transaction operations. | ||
|
@@ -45,7 +46,7 @@ impl SafeSmartAccount { | |
/// # let safe_account = SafeSmartAccount::new("test_key".to_string(), "0x1234567890123456789012345678901234567890").unwrap(); | ||
/// | ||
/// // Transfer USDC on World Chain | ||
/// let tx_hash = safe_account.transaction_transfer( | ||
/// let tx_hash = safe_account.tx_transfer( | ||
/// Network::WorldChain, | ||
/// "0x79A02482A880BCE3F13E09Da970dC34DB4cD24d1", // USDC on World Chain | ||
/// "0x1234567890123456789012345678901234567890", | ||
|
@@ -62,7 +63,7 @@ impl SafeSmartAccount { | |
/// - Will throw a parsing error if any of the provided attributes are invalid. | ||
/// - 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: lets remove |
||
&self, | ||
network: Network, | ||
token_address: &str, | ||
|
@@ -81,7 +82,51 @@ impl SafeSmartAccount { | |
.sign_and_execute(network, self, None, None, provider) | ||
.await | ||
.map_err(|e| TransactionError::Generic { | ||
message: format!("Failed to execute transaction: {e}"), | ||
message: format!("Failed to execute ERC-20 transfer: {e}"), | ||
})?; | ||
|
||
Ok(HexEncodedData::new(&user_op_hash.to_string())?) | ||
} | ||
|
||
/// Allows swapping the owner of a Safe Smart Account. | ||
/// | ||
/// 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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can fetch this from on-chain instead of having to take in as input There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need anything to enforce that this only done for world app safes? Ie check that threshold/signers length is 1 |
||
/// - `new_owner`: The EOA of the new owner (address). | ||
/// | ||
/// # 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
&self, | ||
old_owner: &str, | ||
new_owner: &str, | ||
) -> Result<HexEncodedData, TransactionError> { | ||
let old_owner = Address::parse_from_ffi(old_owner, "old_owner")?; | ||
let new_owner = Address::parse_from_ffi(new_owner, "new_owner")?; | ||
|
||
// TODO: Check if we derive new_owner through key derivation directly in Bedrock. | ||
// TODO: Check if rotation on Optimism is also necessary. | ||
|
||
let transaction = crate::transaction::SafeOwner::new( | ||
self.wallet_address, | ||
old_owner, | ||
new_owner, | ||
); | ||
|
||
let user_op_hash = transaction | ||
.sign_and_execute( | ||
Network::WorldChain, | ||
self, | ||
None, | ||
None, | ||
RpcProviderName::Alchemy, | ||
) | ||
.await | ||
.map_err(|e| TransactionError::Generic { | ||
message: format!("Failed to execute swapOwner: {e}"), | ||
paolodamico marked this conversation as resolved.
Show resolved
Hide resolved
|
||
})?; | ||
|
||
Ok(HexEncodedData::new(&user_op_hash.to_string())?) | ||
|
Uh oh!
There was an error while loading. Please reload this page.