Skip to content

feat: eliminate sender flag if account #10647

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions crates/script/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,16 @@ impl ScriptArgs {
evm_opts.sender = sender;
}

// If no sender derived yet, try to derive from account
if evm_opts.sender == Address::ZERO {
Comment on lines +236 to +237
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this works, I'd prefer if we check if there is a sender value provided in the ScriptArgs instead

or since this is already done tin the maybe_derive_sender_from_account, we can skip the zero check here and use an else if

if let Some(sender) = self.maybe_derive_sender_from_account()? {
evm_opts.sender = sender;
}
}

// Validate that sender matches account if both are provided
self.validate_sender_account_match(&evm_opts)?;

let script_config = ScriptConfig::new(config, evm_opts).await?;

Ok(PreprocessedState { args: self, script_config, script_wallets })
Expand Down Expand Up @@ -348,6 +358,36 @@ impl ScriptArgs {
Ok(maybe_sender)
}

/// Attempts to derive the sender address from the configured account
fn maybe_derive_sender_from_account(&self) -> Result<Option<Address>> {
if self.evm.sender.is_some() {
return Ok(None);
}

let maybe_sender = self.wallets.private_keys()?.filter(|pks| pks.len() == 1).map(|pks| {
Copy link
Contributor

@grandizzy grandizzy Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, don't really understand how this is different from existing maybe_load_private_key ?

fn maybe_load_private_key(&self) -> Result<Option<Address>> {
let maybe_sender = self
.wallets
.private_keys()?
.filter(|pks| pks.len() == 1)
.map(|pks| pks.first().unwrap().address());
Ok(maybe_sender)
}

let address = pks.first().unwrap().address();
trace!(target: "script", "Derived sender {} from private key", address);
address
});
Ok(maybe_sender)
}

/// Validates that the sender address matches the account when both are provided
fn validate_sender_account_match(&self, evm_opts: &EvmOpts) -> Result<()> {
if self.evm.sender.is_some() && evm_opts.sender != Address::ZERO {
if let Some(private_keys) = self.wallets.private_keys()? {
let sender_matches = private_keys.iter().any(|pk| pk.address() == evm_opts.sender);
if !sender_matches {
eyre::bail!(
"Sender address {} does not match any of the provided accounts",
evm_opts.sender
);
}
}
}
Ok(())
}

/// Returns the Function and calldata based on the signature
///
/// If the `sig` is a valid human-readable function we find the corresponding function in the
Expand Down Expand Up @@ -918,4 +958,39 @@ mod tests {
]);
assert!(args.with_gas_price.unwrap().is_zero());
}

// <https://github.com/foundry-rs/foundry/issues/10632>
#[test]
fn test_automatic_sender_derivation_from_account() {
use alloy_primitives::address;

let private_key = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";

let args =
ScriptArgs::parse_from(["foundry-cli", "Contract.sol", "--private-key", private_key]);

assert_eq!(args.evm.sender, None);

let derived_sender = args.maybe_derive_sender_from_account().unwrap();
assert_eq!(derived_sender, Some(address!("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266")));
}
Comment on lines +963 to +976
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoulnd't those use the --account arg as described in the issue and pr?


// <https://github.com/foundry-rs/foundry/issues/10632>
#[test]
fn test_no_sender_derivation_when_sender_already_provided() {
let private_key = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";
let provided_sender = "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266";

let args = ScriptArgs::parse_from([
"foundry-cli",
"Contract.sol",
"--private-key",
private_key,
"--sender",
provided_sender,
]);

let derived_sender = args.maybe_derive_sender_from_account().unwrap();
assert_eq!(derived_sender, None);
}
}