Skip to content

fix(forge): fix the --debug flag in forge debug, default to off #4793

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.

Sign up for GitHub

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

Closed
gbrew wants to merge 1 commit into from
Closed

fix(forge): fix the --debug flag in forge debug, default to off #4793

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/src/cmd/forge/debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl DebugArgs {
gas_estimate_multiplier: 130,
opts: BuildArgs { args: self.opts, ..Default::default() },
evm_opts: self.evm_opts,
debug: true,
debug: self.debug,
Copy link
Member

@mattsse mattsse Apr 22, 2023

Choose a reason for hiding this comment

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

hmm, so in order to launch the debugger you'd need to run forge debug --debug?

this does not sound right to me

  --debug
     Open the script in the debugger
     Revert string configuration. Possible values are "default", "strip" (remove), "debug"

I think the only reason --debug is even available is because we reused a common type that has this.

wdyt @iFrostizz ?

Copy link
Contributor

@iFrostizz iFrostizz Apr 22, 2023

Choose a reason for hiding this comment

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

The change makes sense, but not sure about forge debug utility, why don't we just use forge script --debug for the same purpose ?

Copy link
Author

@gbrew gbrew Apr 22, 2023

Choose a reason for hiding this comment

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

I agree that forge debug --debug is a bit odd, but since that's exactly what the documentation (forge help) says and it took a trivial change to make it so, I rolled with it. I'd be happy to change the docs and/or behavior though if there's consensus on something more sensible.

All I was looking for was the ability to run my contract function and get a trace. I used forge debug because the docs made that sound correct, and I didn't try forge script because it sounded like that was more about building transactions to run on chain, but having just tried it, forge script worked fine for me too.

Any thoughts on the following (mutually exclusive) options? :

  • remove the --debug flag from forge debug and documentation, and potentially add a --trace flag instead which turns off the debugger and just prints a trace. This would leave the default behavior unchanged.
  • remove forge debug altogether and make it clear that forge script is the right thing to use in this case. This seems a bit clearer, but may break people's existing infrastructure.

Copy link
Contributor

@iFrostizz iFrostizz Apr 22, 2023

Choose a reason for hiding this comment

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

forge script should be enough because it's kind of a superset of forge debug so I would be on the side of nuking it, good suggestions

Copy link
Author

@gbrew gbrew Apr 24, 2023

Choose a reason for hiding this comment

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

Given that, I'd be in favor of nuking forge debug as well, just to simplify the interface. I'll defer to you folks who know the project better, but If you'd like me to make either of these changes happen, let me know and I'll see what I can do.

retry: RETRY_VERIFY_ON_CREATE,
..Default::default()
};
Expand Down