-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: master
Are you sure you want to change the base?
Conversation
solves #10632 please have a look @zerosnacks @grandizzy @mattsse |
nice! lgtm |
any comments? |
return Ok(None); | ||
} | ||
|
||
let maybe_sender = self.wallets.private_keys()?.filter(|pks| pks.len() == 1).map(|pks| { |
There was a problem hiding this comment.
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
?
foundry/crates/script/src/lib.rs
Lines 352 to 359 in 643c7fc
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) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont fully understand this just yet, because this is currently using privat_keys not the --account arg as @grandizzy pointed out.
we also want a new test for this because all of this isn't trivial
// If no sender derived yet, try to derive from account | ||
if evm_opts.sender == Address::ZERO { |
There was a problem hiding this comment.
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
#[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"))); | ||
} |
There was a problem hiding this comment.
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?
closes #10632
Primary Enhancement
Make
--sender
optional when--account
is specified. Foundry should automatically derive the sender address from the account's private key.Desired Usage
Implementation Logic