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: skip new contracts on check_storage_layout #974

Open
wants to merge 34 commits into
base: development
Choose a base branch
from

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Oct 4, 2024

Resolves #972

@zugdev
Copy link
Contributor Author

zugdev commented Oct 4, 2024

@rndquu Here are the QAs, new contract is working great now, but I can't reproduce a collision at my local fork for some reason.

  1. No storage updates, CI passing

here

  1. Storage update, no collision, CI passing

here

  1. Storage update, collision, CI failing

here for some reason check_storage_layout is passing but it's not something I did since I haven't touched this job, only the provide_contracts one

  1. New contract added, CI passing

here

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 4, 2024

@zugdev
Copy link
Contributor Author

zugdev commented Oct 5, 2024

If you need a quick way to see workflow runs per commit you can use GitHub's CLI with a version younger than a year, since this change was included in this PR.

gh run list -c 6f1f50f394a06bf8081f96659814d7dadfb4e035

@zugdev zugdev marked this pull request as ready for review October 5, 2024 00:52
@zugdev zugdev requested a review from rndquu October 6, 2024 05:49
@rndquu rndquu requested review from rndquu and removed request for rndquu October 16, 2024 06:56
@rndquu
Copy link
Member

rndquu commented Oct 16, 2024

@zugdev I've missed this in the original issue description, could you apply the same changes to the diamond-storage-check.yml workflow which checks that storage was not modified for Diamond facet libraries? I've increased a time estimate in the original issue to <1 Day.

Pls mark this PR as "ready for review" when you're finished.

@rndquu rndquu marked this pull request as draft October 16, 2024 08:34
@zugdev
Copy link
Contributor Author

zugdev commented Oct 17, 2024

I can't QA properly because all artifacts from development have expired. Error: Error: No workflow run found with an artifact named "development.packages_contracts_src_dollar_libraries_LibAppStorage.sol-LibAppStorage.json". We could either:

  1. Run core-contracts-storage-check.yml and diamond-storage-check.yml with all contracts as parameters periodically in development. Writing a new action that can be manually triggered to do that.

  2. Fork the storage check repo to change how it handles not finding artifacts.

@0x4007
Copy link
Member

0x4007 commented Oct 17, 2024

Adding workflow dispatch seems like a simple and good idea. Its just a line of code.

@zugdev
Copy link
Contributor Author

zugdev commented Oct 17, 2024

Adding workflow dispatch seems like a simple and good idea. Its just a line of code.

Won't really work since core-contracts-storage-check.yml and diamond-storage-check.yml filter by changed files only. I've added generate-storage-artifacts.yml which will generate artifacts for core and diamond. Please check a run of it in my own repo here. Then can can one of you please add it to development branch and run it manually?

@0x4007 @rndquu

@0x4007
Copy link
Member

0x4007 commented Oct 19, 2024

Adding workflow dispatch seems like a simple and good idea. Its just a line of code.

Won't really work since core-contracts-storage-check.yml and diamond-storage-check.yml filter by changed files only. I've added generate-storage-artifacts.yml which will generate artifacts for core and diamond. Please check a run of it in my own repo here. Then can can one of you please add it to development branch and run it manually?

@0x4007 @rndquu

Is it easier to just merge and then run? You could always add a workflow_dispatch so we can manually execute from the Actions UI. Example.

@rndquu rndquu requested review from rndquu and removed request for rndquu October 19, 2024 11:12
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 looks good, pls address the comments, once it's done I'll properly QA both for core and diamond lib contracts.

.github/workflows/core-contracts-storage-check.yml Outdated Show resolved Hide resolved
.github/workflows/core-contracts-storage-check.yml Outdated Show resolved Hide resolved
.github/workflows/diamond-storage-check.yml Outdated Show resolved Hide resolved
packages/contracts/src/dollar/core/ERC20Ubiquity.sol Outdated Show resolved Hide resolved
@zugdev zugdev marked this pull request as ready for review October 19, 2024 17:42
Copy link

ubiquity-os bot commented Nov 4, 2024

@zugdev, this task has been idle for a while. Please provide an update.

@zugdev
Copy link
Contributor Author

zugdev commented Nov 4, 2024

waiting on PR review

Copy link

ubiquity-os bot commented Nov 8, 2024

@zugdev, this task has been idle for a while. Please provide an update.

@0x4007
Copy link
Member

0x4007 commented Nov 18, 2024

@rndquu

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.

CI: fix check_storage_layout for new contracts
3 participants