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.

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

Jump to bottom

feat: forge-inspect script for contracts-core #344

Closed
wants to merge 3 commits into from
Closed

feat: forge-inspect script for contracts-core #344

wants to merge 3 commits into from

Conversation

odyslam
Copy link
Contributor

@odyslam odyslam commented Jun 14, 2022 •
edited
Loading

It's nifty!

@odyslam odyslam requested review from a team as code owners June 14, 2022 08:46
@odyslam odyslam changed the title feat: forge-inspect all contracts feat: forge-inspect script for contracts-core Jun 14, 2022
@odyslam
Copy link
Contributor Author

odyslam commented Jun 27, 2022

@prestwich addressed your comments

prestwich
prestwich reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

is it easy to adapt this to work for Bridge as well? Those are the other contracts that we need to upgrade periodically

prestwich
prestwich requested changes Jul 1, 2022
View reviewed changes
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

add a CI step that generates these and fails CI if it generates a different file

packages/contracts-core/package.json
@@ -31,7 +31,8 @@
"link:node_modules": "ln -s ../../node_modules",
"snapshot": "forge clean && FOUNDRY_PROFILE=core forge snapshot",
"snapshot:check": "forge clean && FOUNDRY_PROFILE=core forge snapshot --check",
"gen-proof": "../../scripts/accumulator-cli"
"gen-proof": "../../scripts/accumulator-cli",
"gen-sl": "scripts/storage-inspect.sh"
Copy link
Member

Choose a reason for hiding this comment

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

this should be added to the build script

@odyslam
Copy link
Contributor Author

odyslam commented Jul 1, 2022

Yeah. Let me add the bridge contracts as well and finally make an addition to the GitHub actions?

We could have something like what was mentioned above. The test will fail if the storage has changed so the engineer has to explicitly acknowledge the change, create and commit the new storage layout so that the test doesn't fail, and then merge. What do you say?

@anna-carroll anna-carroll added the solidity Solidity protocol or xapp change label Jul 4, 2022
@odyslam
Copy link
Contributor Author

odyslam commented Jul 25, 2022

Closing in favour of #403

@odyslam odyslam closed this Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devtooligan devtooligan devtooligan left review comments

@prestwich prestwich prestwich requested changes

@anna-carroll anna-carroll Awaiting requested review from anna-carroll

Assignees

@odyslam odyslam

Labels
solidity Solidity protocol or xapp change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odyslam @prestwich @devtooligan @anna-carroll