Skip to content
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

fix: set script evm sender from all wallet signers #7538

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tchardin
Copy link

@tchardin tchardin commented Apr 1, 2024

Motivation

forge script sets the evm sender by loading the address from the private key. In the case where users are connecting other type of signers the sender is not set which breaks execution of some scripts. (Tested against OP stack deploy script)

Solution

This solution checks that we've loaded a single address across ALL the signers and sets it in the script preprocess.

@klkvr
Copy link
Member

klkvr commented Apr 1, 2024

This is something I was thinking of adding as well, however, this approach has a downside: with this change, users will be required to unlock wallets/keystores right after starting the script which might be annoying, especially for users who were using only vm.startBroadcast(address) cheatcode which requires being explicit about the sender, and thus allows us to avoid touching signers until the broadcast moment, making it possible to simulate a script without ever unlocking wallets at all.

wdyt @mattsse @mds1

@tchardin
Copy link
Author

tchardin commented Apr 2, 2024

Ah yes thanks for the comment! What about adding an unlocked_signers method on MultiWallet so that maybe_load_sender does not try to unlock the pending ones?

@klkvr
Copy link
Member

klkvr commented Apr 2, 2024

@tchardin yeah, that could work, but would probably only change UX for users of hw-wallets

@klkvr
Copy link
Member

klkvr commented Apr 2, 2024

tbh I don't really like the current state of script "sender". It's not really safe to use as for different script parameters it can have different values

When no or multiple wallets are added it's set to default foundry sender.
When only --private-key is provided it's set to the private key wallet

That way, script behavior might be dependent on the exact signer type used which is not great.

What if we changed its semantics and resolved on the level of CALLER opcode invocation instead:

  1. If --sender is provided, CALLER returns provided address
  2. If exactly one signer is provided, CALLER is resolved in the same way we are currently resolving broadcast sender by optionally unlocking interactive wallets.
  3. If multiple signers are provided and no --sender is provided, we throw an error
  4. If no signers and no --sender is provided, we return the default foundry sender

That way, we don't change UX for scripts which weren't using msg.sender, but get rid of ambiguity and complete this PR's goal

@tchardin
Copy link
Author

tchardin commented Apr 2, 2024

Sounds good @klkvr, I adjusted the PR to reflect this logic which does feel nicer. The signers may be unlocked only when the runner is created as it must have the sender address by the time CALLER is resolved.

@mattsse
Copy link
Member

mattsse commented Apr 2, 2024

making it possible to simulate a script without ever unlocking

that sounds good!

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

Successfully merging this pull request may close these issues.

None yet

3 participants