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

check that diamond storage is untouched after each pr #636

Closed
zgorizzo69 opened this issue May 12, 2023 · 104 comments · Fixed by #788 or ubiquity/foundry-storage-check#1
Closed

check that diamond storage is untouched after each pr #636

zgorizzo69 opened this issue May 12, 2023 · 104 comments · Fixed by #788 or ubiquity/foundry-storage-check#1

Comments

@zgorizzo69
Copy link
Contributor

best would be to add a gh action to make sure that we haven't messed with the diamond storage

@PhantomCracker
Copy link
Contributor

PhantomCracker commented May 28, 2023

/start

@ubiquibot
Copy link

ubiquibot bot commented May 28, 2023

Skipping /start since the issue is already assigned

@PhantomCracker
Copy link
Contributor

Hello @zgorizzo69 ,
Can you help with something? I have 2 questions:

  1. where can i find the diamond storage?
  2. is there a backup for diamond storage?

I am thinking of checking the diamond storage before creating a PR and after

@ubiquibot
Copy link

ubiquibot bot commented Jun 3, 2023

Do you have any updates @PhantomCracker? If you would like to release the bounty back to the DevPool, please comment /unassign

@PhantomCracker
Copy link
Contributor

This is work in progress

@0x4007
Copy link
Member

0x4007 commented Jun 4, 2023

Hello @zgorizzo69 , Can you help with something? I have 2 questions:

  1. where can i find the diamond storage?
  2. is there a backup for diamond storage?

I am thinking of checking the diamond storage before creating a PR and after

Perhaps @rndquu might be able to help with this question.

@PhantomCracker
Copy link
Contributor

Hello @zgorizzo69 , Can you help with something? I have 2 questions:

  1. where can i find the diamond storage?
  2. is there a backup for diamond storage?

I am thinking of checking the diamond storage before creating a PR and after

Perhaps @rndquu might be able to help with this question.

I did it in another way so we are good now, thank you!

@0x4007
Copy link
Member

0x4007 commented Jun 8, 2023

I just realized that a week to create a single .yml file seems very steep.

I will need to lower the time estimate to a day if the current assignee is unable to complete.

@PhantomCracker
Copy link
Contributor

PhantomCracker commented Jun 8, 2023

I will try to finish as fast as possible.

@PhantomCracker
Copy link
Contributor

Need to test a few more things here and I will come back with updated PR

@Arjuna2022
Copy link

name: Check Diamond Storage
on:
pull_request:
branches:
- main
jobs:
check_diamond_storage:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Check Diamond Storage Hash
run: |
echo "Checking diamond storage hash..."
before_hash=$(sha256sum diamond_storage)
git merge --no-ff main
after_hash=$(sha256sum diamond_storage)
if [ "$before_hash" != "$after_hash" ]; then
echo "Diamond storage has been tampered with!"
else
echo "Diamond storage has not been tampered with."
fi
Hello everyone, i was wondering if this code helps i got here from build box. very inexpericeind so any advice would be kind helpfull.

@ubiquibot
Copy link

ubiquibot bot commented Jun 15, 2023

Available commands

- /assign: Assign the origin sender to the issue automatically.
- /unassign: Unassign the origin sender from the issue automatically.
- /help: List all available commands.
- /payout: Disable automatic payment for the issue.
- /multiplier: Set bounty multiplier (for treasury)
- /allow: Set access control. (Admin Only)
- /wallet: <WALLET_ADDRESS | ENS_NAME>: Register the hunter's wallet address. 
  ex1: /wallet 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266 
  ex2: /wallet vitalik.eth

@Arjuna2022

@rndquu
Copy link
Member

rndquu commented Jun 15, 2023

name: Check Diamond Storage on: pull_request: branches: - main jobs: check_diamond_storage: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - name: Check Diamond Storage Hash run: | echo "Checking diamond storage hash..." before_hash=$(sha256sum diamond_storage) git merge --no-ff main after_hash=$(sha256sum diamond_storage) if [ "$before_hash" != "$after_hash" ]; then echo "Diamond storage has been tampered with!" else echo "Diamond storage has not been tampered with." fi Hello everyone, i was wondering if this code helps i got here from build box. very inexpericeind so any advice would be kind helpfull.

this issue is already taken by the other bounty hunter, pls find an issue with no assignee

@0x4007
Copy link
Member

0x4007 commented Jun 15, 2023

name: Check Diamond Storage
on:
pull_request:
branches:

  • main
    jobs:
    check_diamond_storage:
    runs-on: ubuntu-latest
    steps:
  • uses: actions/checkout@v2
  • name: Check Diamond Storage Hash
    run: |
    echo "Checking diamond storage hash..."
    before_hash=$(sha256sum diamond_storage)
    git merge --no-ff main
    after_hash=$(sha256sum diamond_storage)
    if [ "$before_hash" != "$after_hash" ]; then
    echo "Diamond storage has been tampered with!"
    else
    echo "Diamond storage has not been tampered with."
    fi
    Hello everyone, i was wondering if this code helps i got here from build box. very inexpericeind so any advice would be kind helpfull.

I think that a fully GitHub Action implementation makes a lot more sense actually vs what we have going on in the associated pull request. I'm just unsure if it makes sense to throw an error on CI if changes happen. I don't think the pull request reviewers should be expected to manually check the CI logs every time before merging.

@PhantomCracker
Copy link
Contributor

Hei @pavlovcik,
I can throw an error if changes happen if this is required.
Regarding github action or script, it will be a long action if we write everything there and it will be harder to read.
Like this, we use best practices for this type of issue/feature.

@0x4007
Copy link
Member

0x4007 commented Jun 18, 2023

It might be best to post a comment in the pull request so that we don't need to dig through the CI logs, similar to what we do already for the deployments.

@rndquu
Copy link
Member

rndquu commented Jul 4, 2023

This issue is really tricky

We have a diamond contract which includes all the facets.

Each facet has a struct (example) which is used as a storage in a unique slot (example).

So on each PR we should check that:

  • state variables in storage structs are not modified
  • now new state variables are added in the beginning or middle of storage structs (we can add new state variables only to the end of a storage struct in order not to allow a storage collision)
  • check that the same rules are also applied to the AppStorage (which occupies slot 0 in all of the facets)
  • make sure that newly added facets are also checked accordingly

So there should be a script that fetches storage layouts from all of the facets (perhaps via forge inspect) and compares the structure in the PR with the structure in the development branch

@ubiquibot
Copy link

ubiquibot bot commented Jul 13, 2023

Releasing the bounty back to dev pool because the allocated duration already ended!
Last activity time: Fri Jun 16 2023 06:57:46 GMT+0000 (Coordinated Universal Time)

@PhantomCracker
Copy link
Contributor

PhantomCracker commented Jul 13, 2023

/start

molecula451 pushed a commit that referenced this issue Sep 28, 2023
molecula451 pushed a commit that referenced this issue Sep 28, 2023
gitcoindev/foundry-storage-check fork

Use foundry-storage-check fork with diamond storage support.

Resolves: #636
molecula451 pushed a commit that referenced this issue Sep 28, 2023
molecula451 pushed a commit that referenced this issue Sep 28, 2023
…hanges

As requested during pr review.

Resolves: #636
molecula451 pushed a commit that referenced this issue Sep 28, 2023
molecula451 pushed a commit that referenced this issue Sep 28, 2023
…rary search

Treat each input as a NAME, use basename -a option instead of -s.
Testing detected that using -s option lost LibAccessControl library.

Resolves: #636
molecula451 pushed a commit that referenced this issue Sep 28, 2023
molecula451 pushed a commit that referenced this issue Sep 28, 2023
Use the same solution that was implemented for core contracts.

Resolves: #636
@gitcoindev
Copy link
Contributor

Thanks @rndquu @molecula451 @pavlovcik this was a significant effort not only for me but also for you, checking, testing and commenting took a lot of time.

Now it seems that the bot was overwhelmed by all of this, seeing this number of comments and did not trigger any comment nor triggered any incentives . I hope we can get this fixed and make the bot generate all incentives correctly.

@0x4007
Copy link
Member

0x4007 commented Sep 28, 2023

We are texting in the group chat about it now t.me/ubiquitydao

Please know that this is a priority for us to address this

We are scrambling for a fix now

Thanks for your contributions and your patience.

@rndquu rndquu reopened this Sep 28, 2023
@rndquu rndquu closed this as completed Sep 28, 2023
@ubiquibot
Copy link

ubiquibot bot commented Sep 28, 2023

Permit generation skipped because wallet private key is not set

If you enjoy the DevPool experience, please follow Ubiquity on GitHub and star this repo to show your support. It helps a lot!

@rndquu rndquu reopened this Sep 28, 2023
@rndquu rndquu closed this as completed Sep 28, 2023
@ubiquibot ubiquibot bot added the Permitted label Sep 28, 2023
@ubiquity ubiquity deleted a comment from ubiquibot bot Sep 28, 2023
@ubiquity ubiquity deleted a comment from ubiquibot bot Sep 28, 2023
@rndquu rndquu reopened this Sep 28, 2023
@rndquu rndquu closed this as completed Sep 28, 2023
@ubiquibot
Copy link

ubiquibot bot commented Sep 28, 2023

Task Assignee Reward

[ CLAIM 800 WXDAI ]

0x7e92476D...A5566653a

If you've enjoyed your experience in the DevPool, we'd appreciate your support. Follow Ubiquity on GitHub and star this repo. Your endorsement means the world to us and helps us grow!
We are excited to announce that the DevPool and UbiquiBot are now available to partners! Our ideal collaborators are globally distributed crypto-native organizations, who actively work on open source on GitHub, and excel in research & development. If you can introduce us to the repository maintainers in these types of companies, we have a special bonus in store for you!

@ubiquibot
Copy link

ubiquibot bot commented Sep 28, 2023

Task Creator Reward

zgorizzo69: [ CLAIM 5.7 WXDAI ]

@gitcoindev
Copy link
Contributor

Thanks @rndquu ! I guess the same approach can be used with #775

@molecula451
Copy link
Member

molecula451 commented Sep 28, 2023

also looks like it missed lot of the conversational rewars, looks like couldn't handle many commenting?. Would be nice see it re-triggered @rndquu

@PhantomCracker
Copy link
Contributor

Congratulations @gitcoindev for finishing this issue! Pretty hard one :)

@rndquu
Copy link
Member

rndquu commented Sep 29, 2023

also looks like it missed lot of the conversational rewars, looks like couldn't handle many commenting?. Would be nice see it re-triggered @rndquu

We will re-trigger when the bot is switched back to the main prod branch. Right now the bot's prod deployment is on a commit without proper conversation rewards.

@molecula451
Copy link
Member

@rndquu i think we should try to re-trigger conversational rewards here, i think the bot is reacting to those on your calls, there were helpful comment here to reach the goal (which after a couple of days) the PR was nice

@rndquu
Copy link
Member

rndquu commented Oct 5, 2023

@rndquu i think we should try to re-trigger conversational rewards here, i think the bot is reacting to those on your calls, there were helpful comment here to reach the goal (which after a couple of days) the PR was nice

I don't really want to re-trigger anything because rewards generation is kind of unpredicted right now. Let's wait for a stable bot's release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment