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(forge): add contract ignore list for gas reports #2528

Merged
merged 2 commits into from Aug 10, 2022

Conversation

danielramirezch
Copy link
Contributor

@danielramirezch danielramirezch commented Jul 31, 2022

Motivation

See #2464

Solution

Create a new Config field called gas_reports_ignore and a new field in GasReport called ignore. (Default value for gas_reports_ignore is an empty Vec<String>.)

Pass gas_reports_ignore as an argument to function GasReport::new so GasReports are built with an ignore list.

The initialization of the variable report_contract decides if the contract has to be reported, using this logic:
image

Were:

  • a: self.report_for.contains(&contract_name)
  • b: self.ignore.contains(&contract_name)
  • c: self.report_for.is_empty()
  • d: self.report_for.contains(&"*".to_string())

In simpler terms: Report the contract every time it's listed on report_for. Report all contracts if report_for is either empty or contains "*". If you're reporting all contracts but one of them is listed on ignore, don't report that one.

If the user listed the contract in 'gas_reports' (the foundry.toml field) a report for the contract is generated even if it's listed in the ignore list. This is addressed this way because getting a report you don't expect is preferable than not getting one you expect. A warning is printed to stderr indicating the "double listing".

@onbjerg onbjerg added the T-feature Type: feature label Aug 3, 2022
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Some minor nits, otherwise good to me so far :)

forge/src/gas_report.rs Outdated Show resolved Hide resolved
forge/src/gas_report.rs Outdated Show resolved Hide resolved
forge/src/gas_report.rs Outdated Show resolved Hide resolved
@onbjerg
Copy link
Member

onbjerg commented Aug 4, 2022

Let me know if something is blocking you :)

@danielramirezch
Copy link
Contributor Author

As I mentioned here, I need help testing it because I don't know how to write tests for it. So I'd really appreciate if you let me know how can I learn to test new features :)

@onbjerg
Copy link
Member

onbjerg commented Aug 4, 2022

@derch28 I think the best way currently would be to add a new forgetest! in cli/tests/it/cmd.rs and checking that the output of the command contains or does not contain what you expect - be careful not to match on the entirey of the output (use String::contains instead) since some of the output may change between each run.

A good starting point is reading some of the tests in there, figuring out what is closest to what you want to do (e.g. setting foundry.toml, adding a simple contract, running forge test --gas-report)

@danielramirezch
Copy link
Contributor Author

danielramirezch commented Aug 7, 2022

@onbjerg Followed what you said and managed to write some tests :)

While testing, I realized that there was a simpler way to solve the issue and that allowed me to delete a function (analyze_trace) and a variable (report_for_all) that were no longer needed.

So, given that all the previous commits don't represent any logical change (because the PR-ready code was all made in the last one) I'm squashing them and force-pushing the result.

Should I edit the "Solution" Section of my first comment and explain how the new solution works there?

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

looks good :) two minor nits, nice one on the clean up

re: updating the original post in the PR, that would be appreciated, mostly because I am going to link to it in the book repo as context for documenting the feature 😄

feel free to take this PR out of draft if you are ready for a final review (mine below)

forge/src/gas_report.rs Outdated Show resolved Hide resolved
forge/src/gas_report.rs Outdated Show resolved Hide resolved
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.

thx for this,

lgtm

cli/tests/it/cmd.rs Outdated Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Comment on lines +1247 to +1258
prj.write_config(Config {
gas_reports: (vec![
"ContractOne".to_string(),
"ContractTwo".to_string(),
"ContractThree".to_string(),
]),
gas_reports_ignore: (vec!["ContractThree".to_string()]),
..Default::default()
});
cmd.forge_fuse();
let third_out = cmd.arg("test").arg("--gas-report").stdout();
assert!(third_out.contains("foo") && third_out.contains("bar") && third_out.contains("baz"));
Copy link
Member

Choose a reason for hiding this comment

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

ContractThree::baz doesn't look ignored here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, "ContractThree" was added to the report list and the ignore list in the same run.

I explained how I decided to handle this behavior in forge/src/gas_report.rs line 53:

"If the user listed the contract in 'gas_reports' (the foundry.toml field) a report for the contract is generated even if it's listed in the ignore list. This is addressed this way because getting a report you don't expect is preferable than not getting one you expect. A warning is printed to stderr indicating the "double listing"."

@gakonst gakonst merged commit 6cd6618 into foundry-rs:master Aug 10, 2022
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* feat(forge): add contract ignore list for gas reports

* Update cli/tests/it/cmd.rs

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants