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

Fix ExtraVariablesProxy upgradeability check #1504

Merged
merged 2 commits into from
Dec 20, 2022
Merged

Conversation

webthethird
Copy link
Contributor

... and by inheritance, also fix ExtraVariablesNewContract.
This check was reporting too many variables, because it was starting at idx = len(order2) - len(order1).
So, if the new contract (or proxy) had 12 variables, and the original contract had 10, it was reporting every variable starting at idx = 2, whereas it should have only been reporting those starting at idx = len(order1).

and by inheritance, also fix `ExtraVariablesNewContract`.
This check was reporting too many variables, because it was starting at `idx = len(order2) - len(order1)`.
So, if the new contract (or proxy) had 12 variables, and the original contract had 10, it was reporting every variable starting at `idx = 2`, whereas it should have only been reporting those starting at `idx = len(order1)`.
@webthethird
Copy link
Contributor Author

Any idea why the Dapp test check failed here? This PR is a very simple fix, limited to just the one line, so I can't imagine the failed test has anything to do with the small change I made.

@0xalpharush
Copy link
Member

@webthethird There's a fix in dev if you'll merge it into your branch. It is unrelated to your changes

@elopez elopez changed the base branch from master to dev December 16, 2022 18:03
@webthethird
Copy link
Contributor Author

@webthethird There's a fix in dev if you'll merge it into your branch. It is unrelated to your changes

Done! And thanks @elopez for switching the base branch, makes sense to me.

@montyly
Copy link
Member

montyly commented Dec 20, 2022

great catch. Thanks @webthethird

@montyly montyly merged commit ed8e585 into crytic:dev Dec 20, 2022
@webthethird webthethird deleted the patch-2 branch December 20, 2022 15:37
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.

None yet

3 participants