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

chore: check core contracts storage changes on pr #789

Conversation

gitcoindev
Copy link
Contributor

Resolves: #775

This GitHub action checks pull requests for all core contracts storage changes.

@rndquu rndquu self-requested a review September 20, 2023 09:31
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Overall works fine for the following checklist:

  • success: new variable added to the end of a contract
  • fail: new state variable introduced in the beginning or in the middle of a contract
  • fail: order of state variables changed
  • fail: type of state variable changed
  • fail: existing state variable removed from the middle of a contract
  • fail: existing state variable removed from the end of a contract
  • fail: inheritance changed, ex: contract MyContract is A,B != contract MyContract is B,A
  • fail: new state variable added to the base contract

The https://github.com/Rubilmax/foundry-storage-check workflow throws a false positive when a new state variable is added to a base contract and the gap size is reduced but we can put up with it.

@ubiquibot
Copy link

ubiquibot bot commented Sep 22, 2023

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

There are some failing "Check For Core Contracts Storage Changes" CI runs but I guess we need to merge this PR first to make it run without errors. Anyway the "Check For Core Contracts Storage Changes" workflow works fine in my forked ubiquity-dollar repo.

@rndquu rndquu requested a review from molecula451 September 23, 2023 07:32
Copy link
Member

@molecula451 molecula451 left a comment

Choose a reason for hiding this comment

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

I have merged the whole thing to my forked branch, and I can replicate.

Working good

Yes. there is NO issue merging this in it's current stage, as it should pass everything when it's merged

My QA:

action running with no triggers
https://github.com/molecula451/ubiquity-dollar/actions/runs/6288841260
action running with trigger
https://github.com/molecula451/ubiquity-dollar/actions/runs/6288943288/job/17074938660

One thing: I will favor the deprecation warning by github about the "set-output" and fix it

[provide_contracts](https://github.com/molecula451/ubiquity-dollar/actions/runs/6288841260/job/17074756015#step:4:8)
The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

@molecula451
Copy link
Member

@gitcoindev korba please rebase branch and update it

@0x4007
Copy link
Member

0x4007 commented Sep 24, 2023

@gitcoindev korba please rebase branch and update it

GitHub UI allows me to directly merge the pull request. If you want I can do it for you

@molecula451 molecula451 merged commit 154081f into ubiquity:development Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add workflow to check that core contracts storage is not corrupted
4 participants