-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Bump NPM and Forge dependencies #14093
Conversation
# Conflicts: # contracts/gas-snapshots/shared.gas-snapshot
I see you updated files related to
|
exclude ExactGas tests from code coverage for shared + exclude test files from Slither
e529ef1
# Conflicts: # .github/CODEOWNERS # .github/workflows/solidity-foundry.yml
e529ef1
to
6830a85
Compare
@@ -246,199 +246,3 @@ contract ArbitrumSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFuncti | |||
assertEq(answer, 0); | |||
} | |||
} | |||
|
|||
contract ArbitrumSequencerUptimeFeed_GasCosts is ArbitrumSequencerUptimeFeedTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional removal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove them? Are they not needed / used anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are "bad" tests that only exist in Foundry because they existed in Hardhat. Since Foundry has gas snapshots, these tests have no purpose. This type of test is generally bad as you need to update them for every tiny contract change. In this case, they broke CI because Foundry coverage has different gas usage compared to foundry test
Quality Gate passedIssues Measures |
@@ -246,199 +246,3 @@ contract ArbitrumSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFuncti | |||
assertEq(answer, 0); | |||
} | |||
} | |||
|
|||
contract ArbitrumSequencerUptimeFeed_GasCosts is ArbitrumSequencerUptimeFeedTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove them? Are they not needed / used anymore?
@@ -43,7 +43,7 @@ mockery: $(mockery) ## Install mockery. | |||
|
|||
.PHONY: foundry | |||
foundry: ## Install foundry. | |||
foundryup --version nightly-de33b6af53005037b463318d2628b5cfcaf39916 | |||
foundryup --version nightly-515a4cc8aba1627a717a1057ff4f09c8cd3bf51f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: does the CI action now use this version automatically? Or do we still need to update something in the CI file? Previously we had an env
for FOUNDRY_VERSION
in solidity-foundry.yaml
, but looks like we don't have it anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's magic now! It actually reads this file now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
* bump npm * more npm * bump foundry dep * make snapshot * rm gas tests * fix change in gas cost * fix blockhash, this breaks the proof * Regenerated VRF proofs and fixed payment outputs in Foundry tests * Updated gas cost for LINK payments in unit tests * try with no coverage, exclude test files from Slither * add support for extra code coverage params * fix warnings and snapshot * changeset * fix codeowners * fix tests and bump foundry * add missing changes quantifier --------- Co-authored-by: Iva Brajer <[email protected]> Co-authored-by: Bartek Tofel <[email protected]>
No description provided.