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

Add storage gaps into upgradeable contracts #103

Merged
merged 6 commits into from
Dec 16, 2024
Merged

Add storage gaps into upgradeable contracts #103

merged 6 commits into from
Dec 16, 2024

Conversation

Mike-CZ
Copy link
Collaborator

@Mike-CZ Mike-CZ commented Nov 26, 2024

This PR adds storage gaps into upgradeable contracts to avoid post-deployment upgradeability issues.

@Mike-CZ Mike-CZ requested review from thaarok and jmpike November 26, 2024 11:46
@thaarok
Copy link
Collaborator

thaarok commented Nov 26, 2024

What is the purpose of this?
From what I know, gaps are being added into parents/abstract contracts, to make some space to add more variables into the parent contract - to create gap before variables of the child:

000 variable of parent
001 variable of parent
002 gap in parent (new variable can be added into the parent here)
003 gap in parent
004 variable of child
005 gap in child (why???)

I don't see how is this going to help in the top-level contract...?

Copy link
Collaborator

@thaarok thaarok left a comment

Choose a reason for hiding this comment

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

Throughout the codebase, there are several contracts that are inherited by upgradeable contracts that do not include a storage gap

There are not. The mentioned contracts are top-level, they are never inherited by other contracts in production, only in few unit-test scenarios like UnitTestSFC/etc.
There is no reason for these gaps.

Copy link
Collaborator

@thaarok thaarok left a comment

Choose a reason for hiding this comment

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

As confirmed by auditors - since these contracts are NOT parents of other contracts, gaps or namespaced storage in these contracts will not bring any advantage.

thaarok
thaarok previously approved these changes Dec 16, 2024
Copy link
Collaborator

@thaarok thaarok left a comment

Choose a reason for hiding this comment

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

Already in prod, whatever...

@Mike-CZ Mike-CZ changed the base branch from mike/retreive_collected_fees to main December 16, 2024 15:34
@Mike-CZ Mike-CZ dismissed thaarok’s stale review December 16, 2024 15:34

The base branch was changed.

@Mike-CZ Mike-CZ requested a review from thaarok December 16, 2024 15:34
@Mike-CZ Mike-CZ merged commit 2cdaaa4 into main Dec 16, 2024
2 checks passed
@Mike-CZ Mike-CZ deleted the mike/storage_gap branch December 16, 2024 17:26
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.

2 participants