Refactor ERC20FixedDenomination and Manager for Hybrid NFT Support #146
Conversation
- Updated `ERC20FixedDenomination` to implement a hybrid ERC-20/ERC-721 model, allowing fixed denomination management through NFTs. - Introduced `ERC404NullOwnerCappedUpgradeable` for enhanced null owner support and upgradeability. - Modified minting functions to support NFT creation alongside ERC20 token minting, ensuring proper ownership handling. - Removed references to the previous ERC721 collection implementation, streamlining the contract structure. - Enhanced metadata handling for NFTs, including dynamic token URI generation based on ethscription data. - Updated tests to validate new functionalities and ensure compliance with the hybrid model.
There was a problem hiding this comment.
Pull request overview
This PR refactors ERC20FixedDenomination and its manager to implement a hybrid ERC-20/ERC-721 model (ERC404), replacing the previous separate ERC721 collection approach. The refactoring allows fixed denomination tokens to be represented as both fungible ERC20 tokens and non-fungible ERC721 tokens within a single contract, with built-in null owner support and upgradeability.
- Introduces
ERC404NullOwnerCappedUpgradeablebase contract combining ERC20/ERC721 functionality with null owner semantics - Updates minting logic to create both ERC20 tokens and specific NFTs with matching mint IDs
- Removes separate ERC721 collection contract references, streamlining the architecture
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/src/ERC404NullOwnerCappedUpgradeable.sol | New base contract implementing hybrid ERC20/ERC721 functionality with null owner support and EIP-7201 namespaced storage |
| contracts/src/interfaces/IERC404.sol | New interface defining the ERC404 hybrid token standard |
| contracts/src/lib/DoubleEndedQueue.sol | New utility library for managing double-ended queues used in NFT tracking |
| contracts/src/ERC20FixedDenomination.sol | Refactored to extend ERC404 base, adding NFT-specific minting and tokenURI metadata generation |
| contracts/src/ERC20FixedDenominationManager.sol | Updated to mint/transfer both ERC20 and ERC721 tokens together, removing separate collection contract logic |
| contracts/test/EthscriptionsToken.t.sol | Added comprehensive tests for NFT enumeration, transfers, and ownership consistency in the hybrid model |
| contracts/test/ERC404FixedDenominationNullOwner.t.sol | New test file validating null owner support and supply cap enforcement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interfaceId == type(IERC404).interfaceId; | ||
| } | ||
|
|
||
| /// @notice Internal function to compute domain separator for EIP-2612 permits |
There was a problem hiding this comment.
Inconsistent indentation: the function comment starts with 2 spaces while the function declaration starts with 4 spaces. The comment should align with the function at 4 spaces.
| /// @notice Internal function to compute domain separator for EIP-2612 permits | |
| /// @notice Internal function to compute domain separator for EIP-2612 permits |
| /// @notice Returns metadata URI for NFT tokens | ||
| /// @dev Returns a data URI with JSON metadata fetched from the main Ethscriptions contract | ||
| function tokenURI(uint256 id_) public view virtual override returns (string memory) { | ||
| uint256 mintId = id_ & ~ID_ENCODING_PREFIX; |
There was a problem hiding this comment.
The bitwise operation id_ & ~ID_ENCODING_PREFIX will not extract the mintId correctly. Since ID_ENCODING_PREFIX = 1 << 255, the complement ~ID_ENCODING_PREFIX has all bits set except bit 255. To extract just the lower bits (the mintId), use id_ - ID_ENCODING_PREFIX or id_ & (ID_ENCODING_PREFIX - 1) instead.
| uint256 mintId = id_ & ~ID_ENCODING_PREFIX; | |
| uint256 mintId = id_ & (ID_ENCODING_PREFIX - 1); |
|
|
||
| /// @notice Returns metadata URI for NFT tokens | ||
| /// @dev Returns a data URI with JSON metadata fetched from the main Ethscriptions contract | ||
| function tokenURI(uint256 id_) public view virtual override returns (string memory) { |
There was a problem hiding this comment.
Missing validation that the token exists before generating metadata. The function should call ownerOf(id_) or check token existence to ensure it reverts for non-existent tokens, as per ERC721 standard behavior.
| function tokenURI(uint256 id_) public view virtual override returns (string memory) { | |
| function tokenURI(uint256 id_) public view virtual override returns (string memory) { | |
| // Ensure the token exists, as per ERC721 standard | |
| ownerOf(id_); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ontract - Updated the `_transferERC721` function to revert with `Unauthorized()` if the `from_` address does not match the owner of the token, enhancing security and ownership validation during transfers.
…adata handling - Updated the `tokenURI` function to include a revert for invalid token IDs, enhancing error handling. - Simplified JSON metadata construction by removing unnecessary attributes, streamlining the output for fixed denomination tokens.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| amount: mintOp.amount, | ||
| mintId: mintOp.id | ||
| }); | ||
| mintIds[token.deployEthscriptionId][mintOp.id] = ethscriptionId; |
There was a problem hiding this comment.
Missing validation to prevent duplicate mints with the same mintId. Before setting mintIds[token.deployEthscriptionId][mintOp.id], check if it's already non-zero and revert with an appropriate error like MintIdAlreadyUsed(). Without this check, a duplicate mint operation could overwrite the mapping and cause the ERC721 mint on line 198 to fail with AlreadyExists(), leaving inconsistent state where mintIds points to the new ethscriptionId but the NFT wasn't minted.
| ERC20FixedDenomination(token.tokenContract).mint({to: recipient, nftId: mintOp.id}); | ||
|
|
||
| // If the initial owner is the null owner, mirror the ERC721 null-owner pattern: | ||
| // mint to creator, then move balances to address(0) (NFT will be burned via forceTransfer logic). |
There was a problem hiding this comment.
The comment states the NFT will be 'burned via forceTransfer logic' but this is misleading. The NFT is not burned; it is transferred to and owned by address(0) according to the null-owner semantics. The _transferERC721 function (lines 395-402 in ERC404NullOwnerCappedUpgradeable.sol) explicitly adds the NFT to owned[address(0)] and sets t.owner = address(0), making address(0) a legitimate owner. Update the comment to clarify: 'mint to creator, then transfer both ERC20 balance and NFT to address(0) (null-owner semantics).'
| // mint to creator, then move balances to address(0) (NFT will be burned via forceTransfer logic). | |
| // mint to creator, then transfer both ERC20 balance and NFT to address(0) (null-owner semantics). |
| string memory mintContent = | ||
| string(abi.encodePacked('data:,{"p":"erc-20","op":"mint","tick":"', tick, '","id":"', id.toString(), '","amt":"', amount.toString(), '"}')); | ||
|
|
||
| vm.prank(alice); |
There was a problem hiding this comment.
The vm.prank(alice) call is inconsistent with the function parameter initialOwner. The function creates an ethscription with initialOwner as the initial owner (line 95), but always uses alice as the transaction sender (line 91). This could mask issues in tests where initialOwner != alice. For example, in testMintToOwnerAndNullOwnerViaManager (line 115), when minting to the null owner (address(0)), the test still uses alice as the sender. Change line 91 to use the actual owner or a more appropriate sender, or document why alice is always the sender regardless of initialOwner.
ERC20FixedDenominationto implement a hybrid ERC-20/ERC-721 model, allowing fixed denomination management through NFTs.ERC404NullOwnerCappedUpgradeablefor enhanced null owner support and upgradeability.Note
Refactors fixed-denomination tokens to an ERC404-based ERC20/ERC721 hybrid with NFT-synced lots, removes the standalone collection layer, and updates manager, metadata, and tests accordingly.
contracts/src/ERC20FixedDenomination.sol):ERC404NullOwnerCappedUpgradeable(hybrid ERC20/ERC721 with cap and null-owner semantics).initialize(name, symbol, cap, mintAmount, deployEthscriptionId)mapping mintAmount to ERC20units.mint(to, nftId)andforceTransfer(from, to, nftId)to sync ERC20 lot with specific NFT.tokenURIwith on-chain JSON using Ethscriptions data; addmintAmount()accessor.contracts/src/ERC20FixedDenominationManager.sol):op_deployinitializes ERC404 token with denomination units.op_mintrecordsTokenItem/mintIds, mints ERC20+specific NFT; handles null-owner by transferring toaddress(0).onTransfermirrors both ERC20 and the specific NFT viaforceTransfer.getMintEthscriptionId(deployEthscriptionId, mintId); keep token address/info queries.ERC404NullOwnerCappedUpgradeable: hybrid ERC20/ERC721 core with namespaced storage, cap enforcement, null-owner support, ID prefixing, internal ERC20/721 mint/transfer helpers; externals default to NotImplemented.IERC404interface andlib/DoubleEndedQueueutility.ERC404FixedDenominationNullOwner.t.solfor null-owner, cap, and manager controls.EthscriptionsToken.t.sol: integrate ERC404 flows, add enumeration/ownership/invariants tests; comment out legacy collection tests.Written by Cursor Bugbot for commit 8d4e965. This will update automatically on new commits. Configure here.