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

Conversation

zjesko
Copy link
Contributor

@zjesko zjesko commented May 27, 2025

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

# Simple case - sender automatically derived from account
forge script script/Deploy.s.sol --rpc-url unichain --account deployer --broadcast

Implementation Logic

  1. When --account is provided without --sender:
  • Load the specified account from the keystore
  • Derive the address from the private key
  • Use that address as the sender for transactions
  • Set msg.sender appropriately in the script context
  1. When both --account and --sender are provided:
  • Validate that the sender address matches the account's derived address
  • Throw a clear error if there's a mismatch
  • This maintains backward compatibility for existing scripts
  1. When only --sender is provided (current behavior):
  • Continue to work as before for raw private key workflows
  • Maintain full backward compatibility

@zjesko
Copy link
Contributor Author

zjesko commented May 27, 2025

solves #10632

please have a look @zerosnacks @grandizzy @mattsse

@mshakeg
Copy link

mshakeg commented May 27, 2025

nice! lgtm

@zjesko
Copy link
Contributor Author

zjesko commented May 31, 2025

any comments?
@zerosnacks @grandizzy @mattsse @DaniPopes

return Ok(None);
}

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

@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)
}

Copy link
Member

@mattsse mattsse left a 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

Comment on lines +236 to +237
// If no sender derived yet, try to derive from account
if evm_opts.sender == Address::ZERO {
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

Comment on lines +963 to +976
#[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")));
}
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

feat(forge script): eliminate redundant --sender flag when using --account
4 participants