Allow Initializable versions greater than 256 #4460
Conversation
🦋 Changeset detectedLatest commit: 5935090 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Bumping pragma to 0.8.20 on Initializable to support NatSpec in structs. Also, keeping it as a draft at the moment because I suspect we may require a test for storage collisions on storage slot 0. |
| @@ -89,17 +102,20 @@ abstract contract Initializable { | |||
| * Emits an {Initialized} event. | |||
| */ | |||
| modifier initializer() { | |||
| bool isTopLevelCall = !_initializing; | |||
| if (!(isTopLevelCall && _initialized < 1) && !(address(this).code.length == 0 && _initialized == 1)) { | |||
| // solhint-disable-next-line var-name-mixedcase | |||
There was a problem hiding this comment.
There are several of these for the $ storage variable. We either keep this rule or wait on protofire/solhint#453 to be merged if that's the case.
|
I like this but before reviewing I'll let @Amxx comment whether he thinks this is a good idea. |
| // solhint-disable-next-line var-name-mixedcase | ||
| InitializableStorage storage $ = _getInitializableStorage(); | ||
|
|
||
| if ($._initializing) { | ||
| revert AlreadyInitialized(); |
There was a problem hiding this comment.
That is not changed by this PR, but I'm wondering if that is the right error to trigger.
There was a problem hiding this comment.
I agree with you but we may decide on this once we decide to tackle #3950 as well
|
The storage layout checks are failing as expected |
|
Closed accidentally. |
|
| @@ -213,7 +213,7 @@ contract('Initializable', function () { | |||
| it('old and new patterns in good sequence', async function () { | |||
| const ok = await DisableOk.new(); | |||
| await expectEvent.inConstruction(ok, 'Initialized', { version: '1' }); | |||
| await expectEvent.inConstruction(ok, 'Initialized', { version: '255' }); | |||
| await expectEvent.inConstruction(ok, 'Initialized', { version: (2n ** 64n - 1n).toString() }); // MAX_UINT64 | |||
There was a problem hiding this comment.
we should consider generalizing this syntax if possible. The issue is that web3.utils.toBN does not support native bigints.
please, ethers.js + chai matchers.
There was a problem hiding this comment.
I used web3.utils.toBN(1).shln(64).subn(1).toString() instead because I think we've previously agreed on a similar one in the ERC2771Forwarder (but for uint48), does this work?
|
LGTM. Do we wait on the linter rule or do we merge like this and come back to it later? |
I'd rather not wait on them, I'm sure we'll revisit after starting with the transpiler work |
Co-authored-by: Francisco <fg@frang.io>
Fixes LIB-828
Fixes #3924
A greater initialized was proposed for #3924, which is implemented within this PR. Also, the EIP-7201 for Namespaced storage is used here to reduce the risk of storage collisions in upgradeable contracts that are also initializable. In this way, the Initializable variables are placed at a storage location that's guaranteed not to clash.
PR Checklist
npx changeset add)