Refactor token ID handling in ERC20FixedDenomination and ERC404NullOw… #147
Conversation
…nerCappedUpgradeable contracts - Updated the `_transferERC721` and `tokenURI` functions to utilize a new `_normalizeTokenId` method for improved token ID validation and handling. - Introduced `_encodeMintId` and `_decodeTokenId` methods to streamline the encoding and decoding of token IDs, enhancing clarity and maintainability. - Replaced direct ID manipulations with the new methods to ensure consistent token ID processing across the contract.
There was a problem hiding this comment.
Pull request overview
This pull request refactors token ID handling in the ERC404 hybrid ERC20/ERC721 contracts by introducing three new helper methods (_normalizeTokenId, _encodeMintId, and _decodeTokenId) to streamline the encoding and decoding of token IDs. The changes improve code clarity and maintainability by replacing direct bitwise and arithmetic operations with well-named functions.
Key Changes
- Introduced
_normalizeTokenId(),_encodeMintId(), and_decodeTokenId()helper functions for consistent token ID processing - Updated
_transferERC721(),ownerOf(),getApproved(), andtokenURI()to use the new helper methods - Added a new
ERC20Transferevent (though this breaks ERC20 standard compliance)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| contracts/src/ERC404NullOwnerCappedUpgradeable.sol | Introduced token ID encoding/decoding helper functions, refactored existing methods to use them, and added a non-standard ERC20Transfer event |
| contracts/src/ERC20FixedDenomination.sol | Updated transferEthscription() and tokenURI() to use the new encoding/decoding helper functions instead of direct arithmetic/bitwise operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// @dev Encodes a human-friendly mintId into the full tokenId with prefix. | ||
| function _encodeMintId(uint256 mintId_) internal pure returns (uint256) { | ||
| if (mintId_ == 0 || mintId_ >= ID_ENCODING_PREFIX) { |
There was a problem hiding this comment.
Inconsistent validation between _encodeMintId and _mintERC721. The _encodeMintId function rejects mintId_ >= ID_ENCODING_PREFIX, but _mintERC721 (line 429) uses nftId_ >= ID_ENCODING_PREFIX - 1. This creates an inconsistency where _mintERC721 rejects ID_ENCODING_PREFIX - 1, but _encodeMintId would accept it.
Consider aligning the validation logic. If the intent is to prevent overflow when adding the prefix, use the same bound in both places: either >= ID_ENCODING_PREFIX - 1 or >= ID_ENCODING_PREFIX. The stricter bound (>= ID_ENCODING_PREFIX - 1) seems more appropriate to prevent tokenId == type(uint256).max after encoding.
| if (mintId_ == 0 || mintId_ >= ID_ENCODING_PREFIX) { | |
| if (mintId_ == 0 || mintId_ >= ID_ENCODING_PREFIX - 1) { |
| } | ||
|
|
||
| emit Transfer(from_, to_, value_); | ||
| emit ERC20Transfer(from_, to_, value_); |
There was a problem hiding this comment.
The introduction of ERC20Transfer event breaks ERC20 standard compliance. The ERC20 standard requires emitting a Transfer(address indexed from, address indexed to, uint256 value) event, which is inherited from IERC20. By emitting ERC20Transfer instead at line 374, external tools, indexers, and contracts expecting standard ERC20 events will not detect these transfers.
The contract should emit the standard Transfer event from IERC20 for ERC20 transfers. If event naming conflicts are a concern, consider using event overloading or aliasing instead of creating a non-standard event name.
| emit ERC20Transfer(from_, to_, value_); | |
| emit Transfer(from_, to_, value_); |
|
|
||
| // ERC721 Events (using different names to avoid conflicts with ERC20) | ||
| // event Transfer(address indexed from, address indexed to, uint256 value); | ||
| event ERC20Transfer(address indexed from, address indexed to, uint256 value); |
There was a problem hiding this comment.
The introduction of ERC20Transfer event breaks ERC20 standard compliance. The standard ERC20 Transfer event (inherited from IERC20) should be emitted for ERC20 transfers, not a custom ERC20Transfer event. External tools, indexers, and contracts will not recognize this non-standard event signature.
| event ERC20Transfer(address indexed from, address indexed to, uint256 value); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…edDenomination and ERC404NullOwnerCappedUpgradeable contracts - Simplified token ID management by removing the encoding and decoding methods, directly using the mint ID in relevant functions. - Enhanced the `tokenURI` function to validate token IDs and streamline metadata generation. - Updated tests to reflect changes in token ID handling and ownership assertions, ensuring consistency with the new implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| uint256 mintId = id_ & ~ID_ENCODING_PREFIX; | ||
| function tokenURI(uint256 mintId) public view virtual override returns (string memory) { | ||
| _validateTokenId(mintId); |
There was a problem hiding this comment.
The _validateTokenId call on line 157 is redundant since ownerOf on line 158 also calls _validateTokenId internally and will revert if the token doesn't exist. Remove the duplicate validation call.
| _validateTokenId(mintId); |
| // Get the ethscriptionId for this mintId from the manager | ||
| ERC20FixedDenominationManager mgr = ERC20FixedDenominationManager(manager); | ||
| bytes32 ethscriptionId = mgr.getMintEthscriptionId(deployEthscriptionId, mintId); | ||
|
|
There was a problem hiding this comment.
Missing validation for ethscriptionId == bytes32(0). The removed code at lines 164-173 handled the case when no ethscription is found. Without this check, the code will attempt to fetch an ethscription with a zero ID, which may cause unexpected behavior or revert in the Ethscriptions contract.
| // If no ethscription is found, return default metadata | |
| if (ethscriptionId == bytes32(0)) { | |
| string memory json = string.concat( | |
| '{"name":"', name(), ' Token #', mintId.toString(), '"', | |
| ',"description":"Fixed denomination token for ', mintAmount().toString(), ' ', symbol(), ' tokens"', | |
| ',"ethscription_id":"0x0"', | |
| '}' | |
| ); | |
| return string.concat("data:application/json;base64,", Base64.encode(bytes(json))); | |
| } |
| uint256 mintId = id_ & ~ID_ENCODING_PREFIX; | ||
| function tokenURI(uint256 mintId) public view virtual override returns (string memory) { | ||
| _validateTokenId(mintId); | ||
| ownerOf(mintId); // reverts on invalid / nonexistent |
There was a problem hiding this comment.
Bug: Missing fallback for zero ethscriptionId in tokenURI
The tokenURI function no longer handles the case where getMintEthscriptionId returns bytes32(0). Previously, the code returned minimal metadata when ethscriptionId was zero. Now it proceeds to call getEthscription with a zero ID, which reverts with EthscriptionDoesNotExist(). This breaks the function for tokens where the ethscription mapping hasn't been initialized or in edge cases where the association is missing.
…nerCappedUpgradeable contracts
_transferERC721andtokenURIfunctions to utilize a new_normalizeTokenIdmethod for improved token ID validation and handling._encodeMintIdand_decodeTokenIdmethods to streamline the encoding and decoding of token IDs, enhancing clarity and maintainability.Note
Drops ID encoding prefix and uses raw
tokenId/mintIdthroughout, adds simple ID validation, streamlines transfers/minting, and updates metadata and tests accordingly.ID_ENCODING_PREFIXand all encode/decode logic; use rawtokenId/mintIddirectly across_mintERC721,_transferERC721,ownerOf,getApproved, andforceTransfer._validateTokenId(nonzero, nottype(uint256).max) and apply inownerOf,getApproved,_mintERC721, andtokenURI.tokenURI(uint256)now takes rawmintId, validates, and builds metadata (renames name field toToken #<id>); removes fallback metadata path.forceTransfernow transfers NFT with rawnftId.balanceOf(address owner, uint256 id)for per-token ownership (returns 1/0)._transferERC721uses direct storage checks (noownerOfcall); clears approval and updates indices._transferERC20mint path no longer checks encoding-based mint limit; still enforces cap.ERC20Transferevent (not emitted;Transferemission retained).ownerOf(1)/ownerOf(2)), and adjust force transfer/cap tests accordingly.Written by Cursor Bugbot for commit 83ef4a3. This will update automatically on new commits. Configure here.