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

feat(script): add --dry-run broadcast argument #9655

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

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Jan 9, 2025

Motivation

implement #9648

Solution

It looks like below:

$ forge script ... --broadcast --dry-run 

=== Transactions that will be broadcast ===


Chain 100010


### Transaction 1 ###

to: GnosisSafeProxyFactory(0x2B67ab75808f0Cf68C80592903427776AFe91A03)
data (raw): 0xc888c9ce9e098d5864...
value: 0 wei [0 ETH]
gasLimit: 1017001


### Transaction 2 ###

to: GnosisSafe(0x803d311111D7b8D4b375D1f11799Ec80e48896b5)
data (raw): 0xc888c9ce9e098d5864d3ded...
value: 0 wei [0 ETH]
gasLimit: 4934023


### Transaction 3 ###

to: GnosisSafeProxyFactory(0x2B67ab75808f0Cf68C80592903427776AFe91A03)
data (decoded): createProxyWithNonce(address,bytes,uint256)(
  0x803d311111D7b8D4b375D1f11799Ec80e48896b5,
  0xb63e800d000000000...00000000000,
  22331162845917118228534381901783985735304293879250085291444737830817647061134
)
data (raw): 0x1688f0b9000000000000000000000000803d311...
value: 0 wei [0 ETH]
gasLimit: 301239

jsvisa added 2 commits January 9, 2025 17:11
Signed-off-by: jsvisa <[email protected]>
@grandizzy
Copy link
Collaborator

@jsvisa thank you! would you mind adding a test case for such?

Comment on lines 354 to 355
// Print all transactions if --dry-run is set
if self.args.dry_run {
Copy link
Member

@zerosnacks zerosnacks Jan 9, 2025

Choose a reason for hiding this comment

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

I think dry run and broadcast should still be mutually exclusive as that is what their meaning is. To run a dry run means to not broadcast.

In favor of the proposed feature though I think it should be exposed either with the global verbosity flag or just by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think --dry-run should only be available when --broadcast or --resume is given, instead of the broadcasting, just print the tx body.

Copy link
Member

@zerosnacks zerosnacks Jan 9, 2025

Choose a reason for hiding this comment

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

To give some more context, basically what we currently refer to as dry run is when a user does not pass a --broadcast flag. I would like to stick to that as we use that for other cases as well (such as forge create, see #9420)

Instead the proposed feature should use the global verbosity flag (e.g. -vvvvv)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zerosnacks PTAL, now I set the dryrun as the default behavior.

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.

3 participants