Use namespaced storage for variables when upgradeable #704
Conversation
…ity, using _nextTokenId in namespaced storage for safe upgradeable minting.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR adds struct support to the Solidity contract builder and introduces UUPS-upgradeable ERC721 minting with auto-incremented token IDs using namespaced storage slots. New utility functions generate storage slot hashes via keccak-256, and test cases verify the incremental minting pattern alongside existing deployments. Changes
Sequence DiagramsequenceDiagram
participant BuildERC721 as buildERC721()
participant AddMintable as addMintable()
participant StorageGen as Storage Utilities
participant Contract as Contract Builder
BuildERC721->>AddMintable: addMintable(..., uups: true)
alt UUPS Path (upgradeable)
AddMintable->>StorageGen: getStorageStruct('Storage')
StorageGen-->>AddMintable: ContractStruct with _nextTokenId
AddMintable->>Contract: addStructVariable(Storage, ...)
Contract->>Contract: structMap.set('Storage', struct)
AddMintable->>StorageGen: generateNamespacesStorageSlot(namespacedName)
StorageGen->>StorageGen: keccak(keccak(input)) + masking
StorageGen-->>AddMintable: storage slot (0x...)
AddMintable->>AddMintable: Create _getStorage() function
AddMintable->>AddMintable: Add assembly code<br/>to set storage slot
AddMintable->>Contract: addFunction(_getStorage)
AddMintable->>Contract: addFunction(safeMint with<br/>_nextTokenId tracking)
else Non-UUPS Path
AddMintable->>Contract: Use standard minting logic
end
AddMintable-->>BuildERC721: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce struct support infrastructure alongside UUPS-aware ERC721 minting with novel storage slot generation logic. While individual pieces are relatively straightforward, the integration across multiple files and introduction of cryptographic operations (dual keccak-256 hashing with BigInt masking) requires careful validation of correctness and interaction points between the contract builder, storage utilities, and ERC721 implementation. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/core/solidity/src/utils/namespaced-storage-generator.ts (1)
3-5: Consider making the namespace prefix configurable.The
getNamespacedStorageNamefunction hardcodes"myProject"as the namespace prefix. While this works for the current use case, consider making it configurable to support different project naming conventions.Example refactor:
-export function getNamespacedStorageName(name: string) { - return `myProject.${name}`; +export function getNamespacedStorageName(name: string, namespace: string = 'myProject') { + return `${namespace}.${name}`; }packages/core/solidity/src/erc721.ts (3)
103-103: Use strict equality for the UUPS check.Avoid implicit coercion here; use
===for clarity and type safety.- addMintable(c, access, allOpts.incremental, allOpts.uriStorage, allOpts.upgradeable == 'uups'); + addMintable(c, access, allOpts.incremental, allOpts.uriStorage, allOpts.upgradeable === 'uups');
178-209: UUPS+incremental flow looks good overall; minor nits.
- The
$identifier is allowed in Solidity but is unconventional; keep if you prefer collision‑resistance in generated code.- Returning
uint256on incremental path and adding_setTokenURIwhenuriStorageis on are correct.
282-294: Adopt unique struct names for forward-compatible ERC-7201 adoption.Currently, only ERC721 with UUPS generates
struct Storage(line 190); no collision exists. However, if ERC20 or ERC1155 later adopt ERC-7201, they will generate conflictingstruct Storagedefinitions. Consider refactoring now to use unique names (e.g.,${ContractName}Storage) to prevent future conflicts and improve code clarity.Minimal changes:
- Update
getStorageFunction(structName: string)to template the return type- Update
getStorageStruct(name: string)to use${name}Storage(already usesc.name, so only the struct field name needs adjustment to${name}Storage)- Update line 187–198 call sites in
addMintableto passstorageStruct.nameand reference it in code generation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/solidity/src/erc721.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
.changeset/rude-nails-mate.md(1 hunks)packages/core/solidity/package.json(1 hunks)packages/core/solidity/src/contract.ts(6 hunks)packages/core/solidity/src/erc721.test.ts(1 hunks)packages/core/solidity/src/erc721.test.ts.md(1 hunks)packages/core/solidity/src/erc721.ts(5 hunks)packages/core/solidity/src/print.ts(4 hunks)packages/core/solidity/src/utils/namespaced-storage-generator.test.ts(1 hunks)packages/core/solidity/src/utils/namespaced-storage-generator.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T22:49:25.653Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: .changeset/sour-hats-grow.md:2-6
Timestamp: 2025-08-15T22:49:25.653Z
Learning: In OpenZeppelin contracts-wizard, breaking changes that have concrete migration paths (like dependency migrations from Community Contracts to OpenZeppelin Contracts) can be handled as minor version bumps instead of major bumps, per maintainer ernestognw's versioning policy.
Applied to files:
.changeset/rude-nails-mate.md
🧬 Code graph analysis (3)
packages/core/solidity/src/print.ts (1)
packages/core/solidity/src/contract.ts (2)
structs(124-126)ContractStruct(56-60)
packages/core/solidity/src/utils/namespaced-storage-generator.test.ts (1)
packages/core/solidity/src/utils/namespaced-storage-generator.ts (1)
generateNamespacesStorageSlot(7-19)
packages/core/solidity/src/erc721.ts (2)
packages/core/solidity/src/contract.ts (3)
ContractBuilder(82-257)BaseFunction(39-45)ContractStruct(56-60)packages/core/solidity/src/utils/namespaced-storage-generator.ts (2)
generateNamespacesStorageSlot(7-19)getNamespacedStorageName(3-5)
🪛 Gitleaks (8.28.0)
packages/core/solidity/src/erc721.test.ts.md
[high] 805-805: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (14)
.changeset/rude-nails-mate.md (1)
1-5: LGTM!The changeset appropriately documents the new feature as a minor version bump, which aligns with the versioning policy for new functionality.
packages/core/solidity/src/print.ts (2)
9-9: LGTM!The ContractStruct import addition enables struct printing support.
262-274: LGTM!The
printStructfunction follows the same pattern asprintFunction2and properly handles both empty and non-empty structs with appropriate bracing.packages/core/solidity/src/erc721.test.ts.md (2)
805-805: Dismiss static analysis false positive.The Gitleaks tool flagged line 805 as a potential API key, but this is a false positive.
MYTOKEN_STORAGE_LOCATIONis a deterministic storage slot constant computed via the ERC-7201 namespaced storage pattern (keccak256-based), not a secret or API key.
800-840: LGTM! ERC-7201 namespaced storage pattern correctly implemented.The generated contract properly implements:
- Storage struct with
@custom:storage-locationannotation- Deterministic storage slot constant
- Internal
_getStoragefunction using inline assembly- Auto-incrementing tokenId in
safeMintThis follows the ERC-7201 namespaced storage pattern for upgrade-safe state management.
packages/core/solidity/src/utils/namespaced-storage-generator.ts (2)
7-19: LGTM! ERC-7201 implementation looks correct.The storage slot generation implements the ERC-7201 pattern:
- Hash the namespace string with keccak256
- Subtract 1 to get innerNum
- Encode to 32 bytes
- Hash again with keccak256
- Mask the lowest byte
This provides collision-resistant namespaced storage slots for upgradeable contracts.
21-26: LGTM! Good overflow protection.The
padTo32Byteshelper correctly validates that values fit within uint256 (64 hex characters = 32 bytes) before encoding.packages/core/solidity/src/contract.ts (5)
56-60: LGTM! Clean struct interface.The
ContractStructinterface follows the same pattern as other contract elements with name, comments, and content (variables in this case).
96-96: LGTM!The
structMapfollows the established pattern of using Maps for contract elements (parentMap, functionMap).
124-126: LGTM!The structs getter provides a clean public API for accessing structs, consistent with existing getters.
187-198: LGTM!The
addStructmethod follows the same deduplication pattern asaddFunction, preventing duplicate struct definitions.
248-256: LGTM! Safe struct variable management.The
addStructVariablemethod properly handles both creating new structs and adding variables to existing ones, with duplicate detection to prevent redundant additions.packages/core/solidity/src/utils/namespaced-storage-generator.test.ts (1)
4-23: ERC-7201 compliance verified. The implementation correctly follows the specification: it computeskeccak256(keccak256(id) - 1) & ~0xff, with the intermediate value properly encoded to 32 bytes (matchingabi.encode(uint256(...))). The test cases are well-structured with appropriate input variations.packages/core/solidity/src/erc721.ts (1)
1-1: LGTM on imports.Imports look correct and scoped. No issues.
Also applies to: 15-15
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
Thank you for the PR! This looks like the right approach.
I changed the keccak library to use ethereum-cryptography instead, as that is more recent and is maintained under the ethereum GitHub org.
Left some additional comments about how the namespace-related code is generated. Let us know if you'd like to take those on, otherwise we can pick up any remaining items. Thanks!
|
Thanks @typicalHuman. Would you mind signing the CLA according to #704 (comment)? We'll plan to update the UI to allow adding a namespace prefix. |
|
I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement |
ericglau
changed the title
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
We'll do another review internally before proceeding, but this LGTM, thanks!
|
@SocketSecurity ignore-all |
Co-authored-by: Ernesto García <[email protected]>
This reverts commit a14b9cc.
…NamespacedStorageSlot"" This reverts commit ddf8d21.
3 participants
Use namespaced storage for variables when upgradeability is enabled:
tokenBridgewhen cross-chain bridging is set to'custom'and upgradeability is enabled._nextTokenIdwhen mintable, auto increment IDs, and upgradeability are enabled.#383