Refactor ERC404NullOwnerCappedUpgradeable contract by removing unused… #150
Conversation
… interfaces and dependencies - Deleted the `IERC404` interface and the `DoubleEndedQueue` library, simplifying the contract structure. - Updated function signatures to remove references to the deleted interface, ensuring compliance with ERC20 and ERC721 standards. - Streamlined the contract by eliminating unnecessary state variables and constants, enhancing readability and maintainability. - Adjusted event emissions and error handling to align with the new implementation.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| assembly { | ||
| $.slot := STORAGE_LOCATION | ||
| $.slot := slot |
There was a problem hiding this comment.
Bug: Storage slot calculation breaks EIP-7201 compatibility for upgrades
The _getS() function now computes the storage slot using simple keccak256("ethscriptions.storage.ERC404NullOwnerCapped"), but the original code used the proper EIP-7201 formula: keccak256(abi.encode(uint256(keccak256(id)) - 1)) & ~bytes32(uint256(0xff)) which produced 0x8a0c9d8e5f7b3a2c1d4e6f8a9b7c5d3e2f1a4b6c8d9e7f5a3b2c1d4e6f8a9b00. These produce completely different storage slots. For this upgradeable contract, any upgrade would cause the contract to read/write to a different storage location, making all existing state data (balances, tokens, ownership) completely inaccessible.
There was a problem hiding this comment.
Pull request overview
This PR refactors the ERC404NullOwnerCappedUpgradeable contract by removing the custom IERC404 interface and DoubleEndedQueue library, simplifying the implementation to rely solely on standard ERC20/ERC721 interfaces. The changes also include comprehensive test coverage additions for the collections header protocol, testing various edge cases and validation scenarios.
Key Changes:
- Removed IERC404 interface and DoubleEndedQueue library dependencies
- Simplified storage structure by removing queue-based state and ERC721 transfer exemption logic
- Updated function signatures to remove IERC404 interface references
- Modified storage slot calculation from constant to runtime computation
- Added extensive test coverage for merkle proof validation, collection limits, and error scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
spec/integration/collections_header_protocol_spec.rb |
Added comprehensive unhappy path tests for merkle proof validation, collection validation, supply limits, and ownership privileges; refactored helper methods to use let bindings |
contracts/src/lib/DoubleEndedQueue.sol |
Deleted unused queue library that was previously used for ERC721 token management |
contracts/src/interfaces/IERC404.sol |
Deleted custom ERC404 interface, moving to standard ERC20/ERC721 interfaces only |
contracts/src/ERC404NullOwnerCappedUpgradeable.sol |
Removed IERC404 dependencies, simplified storage structure, removed queue-based state, removed permit/domain separator functionality, and changed storage slot calculation method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [create_input(creator: owner, to: owner, data_uri: uri)], | ||
| esip_overrides: { esip6_is_enabled: true } | ||
| ) | ||
| # binding.irb |
There was a problem hiding this comment.
Left-over debug code should be removed. The commented binding.irb line is a Ruby debugging breakpoint that should not be committed to the codebase.
| # binding.irb |
| // ============================================================= | ||
|
|
||
| function _getS() internal pure returns (TokenStorage storage $) { | ||
| bytes32 slot = keccak256("ethscriptions.storage.ERC404NullOwnerCapped"); |
There was a problem hiding this comment.
The storage slot calculation has been simplified from a constant to runtime computation, but this changes the actual storage slot location. The old implementation used a constant STORAGE_LOCATION = 0x8a0c9d8e5f7b3a2c1d4e6f8a9b7c5d3e2f1a4b6c8d9e7f5a3b2c1d4e6f8a9b00 (supposedly following EIP-7201's formula), while the new code directly hashes the namespace string.
Critical Issue: If this contract has already been deployed, this change will cause the contract to read from/write to a completely different storage slot, resulting in:
- Loss of access to existing state (balances, tokens, etc.)
- Potential corruption of unrelated storage slots
- Breaking existing deployed proxies
If this is a new deployment: Verify that the new slot calculation matches EIP-7201 specification: keccak256(abi.encode(uint256(keccak256("namespace")) - 1)) & ~bytes32(uint256(0xff)). The current implementation keccak256("namespace") does not follow this formula.
Recommend either:
- Reverting to the constant if contracts are already deployed
- Using the proper EIP-7201 formula if this is intentional
- Documenting if EIP-7201 compliance is not required
| bytes32 slot = keccak256("ethscriptions.storage.ERC404NullOwnerCapped"); | |
| bytes32 slot = keccak256(abi.encode(uint256(keccak256("ethscriptions.storage.ERC404NullOwnerCapped")) - 1)) & ~bytes32(uint256(0xff)); |
| /// keccak256(abi.encode(uint256(keccak256("ethscriptions.storage.ERC404NullOwnerCapped")) - 1)) & ~bytes32(uint256(0xff)) | ||
| bytes32 private constant STORAGE_LOCATION = 0x8a0c9d8e5f7b3a2c1d4e6f8a9b7c5d3e2f1a4b6c8d9e7f5a3b2c1d4e6f8a9b00; | ||
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. The line contains only spaces after the closing brace, which violates code formatting standards.
|
|
||
| function _getS() internal pure returns (TokenStorage storage $) { | ||
| bytes32 slot = keccak256("ethscriptions.storage.ERC404NullOwnerCapped"); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected. The line contains only spaces, which violates code formatting standards.
| } | ||
|
|
||
| # Build a single-item merkle tree for this new item (so proof is valid) | ||
| single_plan = build_merkle_plan([different_item_at_index_0]) |
There was a problem hiding this comment.
This assignment to single_plan is useless, since its value is never read.
| single_plan = build_merkle_plan([different_item_at_index_0]) |
… interfaces and dependencies
IERC404interface and theDoubleEndedQueuelibrary, simplifying the contract structure.Note
Removes IERC404 and DoubleEndedQueue, simplifies the hybrid ERC20/ERC721 contract/storage/interfaces, and adds comprehensive header-based collections protocol tests covering merkle enforcement and edge cases.
contracts/src/ERC404NullOwnerCappedUpgradeable.sol):IERC404andDoubleEndedQueuedependencies; delete related imports, storage, views, and queue helpers.keccak256string.supportsInterfaceaccordingly.NotFound,InvalidTokenId,AlreadyExists,OwnedIndexOverflow); keepERC721Transferevent only.IERC404; keep core ERC20/721 internals (_transferERC20,_transferERC721,_mint*).contracts/src/interfaces/IERC404.solandcontracts/src/lib/DoubleEndedQueue.sol.spec/integration/collections_header_protocol_spec.rb):expect_protocol_failure,create_header_collection, randomizedmetadata_payloadname) and constants (force_merkle_sender,zero_merkle_root).Written by Cursor Bugbot for commit 870e2e1. This will update automatically on new commits. Configure here.