-
Notifications
You must be signed in to change notification settings - Fork 809
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
add non static call constraint for sstore #467
add non static call constraint for sstore #467
Conversation
the CI error looks weird to me (https://github.com/appliedzkp/zkevm-circuits/runs/6073405191?check_suite_focus=true), Does anybody know that ? @CPerezz @ChihChengLiang @miha-stopar ? |
@DreamWuGit Created an issue #470 and I'm investigating. For reviewers, Feel free to ignore that error and do the review. |
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.
Minor suggestions from my side, looks good otherwise :)
@DreamWuGit this happens because github.secrets are not shared with workflows triggered from PRs on forks.Therefore the invoked action is not permitted to access the https://api.github.com/repos/appliedzkp/zlevm-circuits/pull/467/requested_reviewers endpoint. We are working on alternatives with @ntampakas |
We are trying to avoid GH Actions limitations on forked repos, by implementing two different workflows:
This is the cleanest way to trigger workflows from PRs coming from forks, that bypasses the necessity to access the GH secret(s). |
Moved to #446 |
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.
overall LGTM. Just need to address the comments
@ntampakas I am curious why other PR not hit this issue , even my other PR (i.e. #466 is OK) :) |
after push new stuff, CI can pass now . |
@AronisAt79 @ntampakas could you move this thread to the issue/PR where it corresponds to? |
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.
LGTM.
Can we update and merge this??
@lispc I can't rebase myself in order to merge it. |
i don't fully understand "rebase myself in order to merge it". I clicked the "update branch" button. And i think there is a "squash and merge" button, which is very helpful for things like this? |
Oh @lispc I thought you couldn't merge the PR. Ye, just squash and merge please! :) |
nonono, I cannot merge this. I snapshot from another repo just to give a demo.. |
OOOOH! No, what I meant was that when your branch is out-of-date you can Thanks!! Merging! |
sstore op code will modify state, so if it is executed in static call , exception will occur , here constrain only in non static call , spec privacy-scaling-explorations/zkevm-specs#188