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: script simulation with default sender #9042

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Oct 6, 2024

Motivation

Closes #8960

Right now, when simulating a script without providing a --sender arg, we will use the default sender both for script contract deployment and setUp/run. This will result in sender nonce being higher than an actual on-chain nonce. For certain scripts, this might result in mismatched addresses and failure during on-chain simulation, if incorrect addresses end up in calldata.

Solution

Fix contains 2 parts:

  1. Follow-up for fix: nonce correction logic #7907 which enables nonce correction even when sender is default foundry sender. Unsure why this case was excluded earlier
  2. Hack during script execution — if we're simulating with default sender, we'd set it's nonce to a hardcoded high value for script contract CREATE transaction. This allows us to reset nonce back to on-chain value when simulating the script, and ensure matching contract addresses during on-chain sumlation

@grandizzy
Copy link
Collaborator

Possibly fixing #2002 too

@klkvr klkvr changed the title Klkvr/script sender bug fix: scipt simulation with default sender Oct 6, 2024
@klkvr
Copy link
Member Author

klkvr commented Oct 6, 2024

Possibly fixing #2002 too

I don't think so. This sounds like an issue with gas estimation on optimism? looks like people are getting around it by using --with-gas-multiplier

it could be addressed by enforcing --slow for optimism by default, like we do for arbitrum:

pub fn has_different_gas_calc(chain_id: u64) -> bool {

though this might affect UX

cc @mds1, assume you're running scripts on OP networks quite often and know about hacks that are required

@zerosnacks zerosnacks changed the title fix: scipt simulation with default sender fix: script simulation with default sender Oct 7, 2024
@mds1
Copy link
Collaborator

mds1 commented Oct 10, 2024

cc @mds1, assume you're running scripts on OP networks quite often and know about hacks that are required

Most scripts I run these days are actually on L1, so I don't have much insight unfortunately. I will see if someone else has more insight than I do here

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

makes sense, lgtm!

@grandizzy
Copy link
Collaborator

@zerosnacks @yash-atreya mind to check before merging? thank you!

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Fix LGTM

Initially thought that setting the nonce to a constant u64::MAX / 2 might result in duplicate nonces / replacement transactions but considering the state is committed directly and synchronously this should not be an issue

@grandizzy grandizzy merged commit 09824ad into master Oct 21, 2024
21 checks passed
@grandizzy grandizzy deleted the klkvr/script-sender-bug branch October 21, 2024 08:49
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* add test

* fix: ensure correct sender nonce when dry-running script in fork

* fix test

* Fix test

---------

Co-authored-by: grandizzy <[email protected]>
@grandizzy grandizzy added T-bug Type: bug C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-bug Type: bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug(forge script): reading from other contracts that are deployed as part of the script fails
4 participants