From 7ab376752ea2c34302a1af95306848faeb0428df Mon Sep 17 00:00:00 2001 From: Tom Lehman Date: Mon, 24 Nov 2025 12:54:45 -0500 Subject: [PATCH 1/2] Refactor token ID handling in ERC20FixedDenomination and ERC404NullOwnerCappedUpgradeable 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. --- contracts/src/ERC20FixedDenomination.sol | 11 +-- .../src/ERC404NullOwnerCappedUpgradeable.sol | 72 ++++++++++++++----- 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/contracts/src/ERC20FixedDenomination.sol b/contracts/src/ERC20FixedDenomination.sol index 9f0da96..37afc91 100644 --- a/contracts/src/ERC20FixedDenomination.sol +++ b/contracts/src/ERC20FixedDenomination.sol @@ -90,7 +90,7 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable { _transferERC20(from, to, units()); // Transfer the specific NFT using the proper function - uint256 id = ID_ENCODING_PREFIX + nftId; + uint256 id = _encodeMintId(nftId); _transferERC721(from, to, id); } @@ -155,10 +155,11 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable { /// @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) { - // This will revert InvalidTokenId / NotFound on bad ids - ownerOf(id_); - - uint256 mintId = id_ & ~ID_ENCODING_PREFIX; + // Normalize and enforce existence (accepts human mintId or encoded tokenId) + uint256 tokenId = _normalizeTokenId(id_); + ownerOf(tokenId); // reverts on invalid / nonexistent + + uint256 mintId = _decodeTokenId(tokenId); // Get the ethscriptionId for this mintId from the manager ERC20FixedDenominationManager mgr = ERC20FixedDenominationManager(manager); diff --git a/contracts/src/ERC404NullOwnerCappedUpgradeable.sol b/contracts/src/ERC404NullOwnerCappedUpgradeable.sol index b8c4380..be6b1e4 100644 --- a/contracts/src/ERC404NullOwnerCappedUpgradeable.sol +++ b/contracts/src/ERC404NullOwnerCappedUpgradeable.sol @@ -79,6 +79,8 @@ abstract contract ERC404NullOwnerCappedUpgradeable is // ERC20 Events are inherited from IERC20 (Transfer, Approval) // 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); event ERC721Transfer(address indexed from, address indexed to, uint256 indexed id); event ERC721Approval(address indexed owner, address indexed spender, uint256 indexed id); event ApprovalForAll(address indexed owner, address indexed operator, bool approved); @@ -202,12 +204,9 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } function ownerOf(uint256 id_) public view virtual override(IERC404) returns (address) { - if (!_isValidTokenId(id_)) { - revert InvalidTokenId(); - } - TokenStorage storage $ = _getS(); - TokenData storage t = $.tokens[id_]; + uint256 tokenId = _normalizeTokenId(id_); + TokenData storage t = $.tokens[tokenId]; if (!t.exists) { revert NotFound(); @@ -223,7 +222,13 @@ abstract contract ERC404NullOwnerCappedUpgradeable is function getApproved(uint256 id_) public view virtual returns (address) { TokenStorage storage $ = _getS(); - return $.getApproved[id_]; + uint256 tokenId = _normalizeTokenId(id_); + + if (!$.tokens[tokenId].exists) { + revert NotFound(); + } + + return $.getApproved[tokenId]; } function isApprovedForAll(address owner_, address operator_) public view virtual override(IERC404) returns (bool) { @@ -366,25 +371,29 @@ abstract contract ERC404NullOwnerCappedUpgradeable is $.balances[to_] += value_; } - emit Transfer(from_, to_, value_); + emit ERC20Transfer(from_, to_, value_); } /// @notice Transfer an ERC721 token function _transferERC721(address from_, address to_, uint256 id_) internal virtual { TokenStorage storage $ = _getS(); - TokenData storage t = $.tokens[id_]; - - if (from_ != ownerOf(id_)) { + uint256 tokenId = _normalizeTokenId(id_); + TokenData storage t = $.tokens[tokenId]; + + if (!t.exists) { + revert NotFound(); + } + if (from_ != t.owner) { revert Unauthorized(); } if (from_ != address(0)) { // Clear approval - delete $.getApproved[id_]; + delete $.getApproved[tokenId]; // Remove from sender's owned list uint256 lastTokenId = $.owned[from_][$.owned[from_].length - 1]; - if (lastTokenId != id_) { + if (lastTokenId != tokenId) { uint256 updatedIndex = t.index; $.owned[from_][updatedIndex] = lastTokenId; $.tokens[lastTokenId].index = uint88(updatedIndex); @@ -399,9 +408,9 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } t.owner = to_; t.index = uint88(newIndex); - $.owned[to_].push(id_); + $.owned[to_].push(tokenId); - emit ERC721Transfer(from_, to_, id_); + emit ERC721Transfer(from_, to_, tokenId); } /// @notice Mint ERC20 tokens without triggering NFT creation @@ -424,7 +433,7 @@ abstract contract ERC404NullOwnerCappedUpgradeable is TokenStorage storage $ = _getS(); // Add the ID_ENCODING_PREFIX to the provided ID - uint256 id = ID_ENCODING_PREFIX + nftId_; + uint256 id = _encodeMintId(nftId_); TokenData storage t = $.tokens[id]; @@ -444,8 +453,37 @@ abstract contract ERC404NullOwnerCappedUpgradeable is // HELPER FUNCTIONS // ============================================================= - function _isValidTokenId(uint256 id_) internal pure returns (bool) { - return id_ > ID_ENCODING_PREFIX && id_ != type(uint256).max; + /// @dev Normalizes caller input into the encoded tokenId form (adds prefix for human mint IDs). + function _normalizeTokenId(uint256 id_) internal pure returns (uint256 tokenId_) { + if (id_ == 0 || id_ == type(uint256).max) { + revert InvalidTokenId(); + } + + tokenId_ = id_ < ID_ENCODING_PREFIX ? _encodeMintId(id_) : id_; + + if (tokenId_ <= ID_ENCODING_PREFIX || tokenId_ == type(uint256).max) { + revert InvalidTokenId(); + } + } + + /// @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) { + revert InvalidTokenId(); + } + uint256 tokenId = ID_ENCODING_PREFIX + mintId_; + if (tokenId == type(uint256).max) { + revert InvalidTokenId(); + } + return tokenId; + } + + /// @dev Decodes an encoded tokenId back to the human-friendly mintId. + function _decodeTokenId(uint256 tokenId_) internal pure returns (uint256) { + if (tokenId_ <= ID_ENCODING_PREFIX || tokenId_ == type(uint256).max) { + revert InvalidTokenId(); + } + return tokenId_ - ID_ENCODING_PREFIX; } // ============================================================= From 83ef4a3005501c01da4a700089f176c67fa57efb Mon Sep 17 00:00:00 2001 From: Tom Lehman Date: Mon, 24 Nov 2025 17:30:21 -0500 Subject: [PATCH 2/2] Refactor token ID handling and improve metadata functions in ERC20FixedDenomination 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. --- contracts/src/ERC20FixedDenomination.sol | 23 +--- .../src/ERC404NullOwnerCappedUpgradeable.sol | 106 ++++++------------ .../ERC404FixedDenominationNullOwner.t.sol | 6 +- 3 files changed, 41 insertions(+), 94 deletions(-) diff --git a/contracts/src/ERC20FixedDenomination.sol b/contracts/src/ERC20FixedDenomination.sol index 37afc91..059e8eb 100644 --- a/contracts/src/ERC20FixedDenomination.sol +++ b/contracts/src/ERC20FixedDenomination.sol @@ -90,8 +90,7 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable { _transferERC20(from, to, units()); // Transfer the specific NFT using the proper function - uint256 id = _encodeMintId(nftId); - _transferERC721(from, to, id); + _transferERC721(from, to, nftId); } // ============================================================= @@ -154,26 +153,14 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable { /// @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) { - // Normalize and enforce existence (accepts human mintId or encoded tokenId) - uint256 tokenId = _normalizeTokenId(id_); - ownerOf(tokenId); // reverts on invalid / nonexistent - - uint256 mintId = _decodeTokenId(tokenId); + function tokenURI(uint256 mintId) public view virtual override returns (string memory) { + _validateTokenId(mintId); + ownerOf(mintId); // reverts on invalid / nonexistent // Get the ethscriptionId for this mintId from the manager ERC20FixedDenominationManager mgr = ERC20FixedDenominationManager(manager); bytes32 ethscriptionId = mgr.getMintEthscriptionId(deployEthscriptionId, mintId); - if (ethscriptionId == bytes32(0)) { - // If no ethscription found, return minimal metadata - return string(abi.encodePacked( - "data:application/json;utf8,", - '{"name":"', name(), ' Note #', mintId.toString(), '",', - '"description":"Denomination note for ', mintAmount().toString(), ' tokens"}' - )); - } - // Get the ethscription data from the main contract Ethscriptions ethscriptionsContract = Ethscriptions(Predeploys.ETHSCRIPTIONS); Ethscriptions.Ethscription memory ethscription = ethscriptionsContract.getEthscription(ethscriptionId, false); @@ -184,7 +171,7 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable { // Build the JSON metadata string memory jsonStart = string.concat( - '{"name":"', name(), ' Note #', mintId.toString(), '"', + '{"name":"', name(), ' Token #', mintId.toString(), '"', ',"description":"Fixed denomination token for ', mintAmount().toString(), ' ', symbol(), ' tokens"' ); diff --git a/contracts/src/ERC404NullOwnerCappedUpgradeable.sol b/contracts/src/ERC404NullOwnerCappedUpgradeable.sol index be6b1e4..ee66299 100644 --- a/contracts/src/ERC404NullOwnerCappedUpgradeable.sol +++ b/contracts/src/ERC404NullOwnerCappedUpgradeable.sol @@ -69,9 +69,6 @@ abstract contract ERC404NullOwnerCappedUpgradeable is /// keccak256(abi.encode(uint256(keccak256("ethscriptions.storage.ERC404NullOwnerCapped")) - 1)) & ~bytes32(uint256(0xff)) bytes32 private constant STORAGE_LOCATION = 0x8a0c9d8e5f7b3a2c1d4e6f8a9b7c5d3e2f1a4b6c8d9e7f5a3b2c1d4e6f8a9b00; - /// @dev Constant for token id encoding - uint256 public constant ID_ENCODING_PREFIX = 1 << 255; - // ============================================================= // EVENTS // ============================================================= @@ -127,14 +124,9 @@ abstract contract ERC404NullOwnerCappedUpgradeable is ) internal onlyInitializing { TokenStorage storage $ = _getS(); - if (cap_ == 0 || cap_ > ID_ENCODING_PREFIX - 1) { - revert ERC20InvalidCap(cap_); - } - + if (cap_ == 0) revert ERC20InvalidCap(cap_); uint256 base = 10 ** decimals(); - if (units_ == 0 || units_ % base != 0) { - revert InvalidUnits(units_); - } + if (units_ == 0 || units_ % base != 0) revert InvalidUnits(units_); $.name = name_; $.symbol = symbol_; @@ -175,6 +167,17 @@ abstract contract ERC404NullOwnerCappedUpgradeable is TokenStorage storage $ = _getS(); return $.balances[account]; } + + function balanceOf(address owner_, uint256 id_) + public + view + returns (uint256) + { + TokenStorage storage $ = _getS(); + TokenData storage t = $.tokens[id_]; + if (!t.exists) return 0; + return t.owner == owner_ ? 1 : 0; + } function allowance(address owner, address spender) public view virtual override(IERC404, IERC20) returns (uint256) { TokenStorage storage $ = _getS(); @@ -204,13 +207,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } function ownerOf(uint256 id_) public view virtual override(IERC404) returns (address) { + _validateTokenId(id_); TokenStorage storage $ = _getS(); - uint256 tokenId = _normalizeTokenId(id_); - TokenData storage t = $.tokens[tokenId]; + TokenData storage t = $.tokens[id_]; - if (!t.exists) { - revert NotFound(); - } + if (!t.exists) revert NotFound(); return t.owner; } @@ -221,14 +222,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } function getApproved(uint256 id_) public view virtual returns (address) { + _validateTokenId(id_); TokenStorage storage $ = _getS(); - uint256 tokenId = _normalizeTokenId(id_); + if (!$.tokens[id_].exists) revert NotFound(); - if (!$.tokens[tokenId].exists) { - revert NotFound(); - } - - return $.getApproved[tokenId]; + return $.getApproved[id_]; } function isApprovedForAll(address owner_, address operator_) public view virtual override(IERC404) returns (bool) { @@ -352,9 +350,6 @@ abstract contract ERC404NullOwnerCappedUpgradeable is if (newSupply > $.cap) { revert ERC20ExceededCap(newSupply, $.cap); } - if (newSupply > ID_ENCODING_PREFIX) { - revert MintLimitReached(); - } $.totalSupply = newSupply; } else { // Transfer @@ -371,29 +366,25 @@ abstract contract ERC404NullOwnerCappedUpgradeable is $.balances[to_] += value_; } - emit ERC20Transfer(from_, to_, value_); + emit Transfer(from_, to_, value_); + // emit ERC20Transfer(from_, to_, value_); } /// @notice Transfer an ERC721 token function _transferERC721(address from_, address to_, uint256 id_) internal virtual { TokenStorage storage $ = _getS(); - uint256 tokenId = _normalizeTokenId(id_); - TokenData storage t = $.tokens[tokenId]; + TokenData storage t = $.tokens[id_]; - if (!t.exists) { - revert NotFound(); - } - if (from_ != t.owner) { - revert Unauthorized(); - } + if (!t.exists) revert NotFound(); + if (from_ != t.owner) revert Unauthorized(); if (from_ != address(0)) { // Clear approval - delete $.getApproved[tokenId]; + delete $.getApproved[id_]; // Remove from sender's owned list uint256 lastTokenId = $.owned[from_][$.owned[from_].length - 1]; - if (lastTokenId != tokenId) { + if (lastTokenId != id_) { uint256 updatedIndex = t.index; $.owned[from_][updatedIndex] = lastTokenId; $.tokens[lastTokenId].index = uint88(updatedIndex); @@ -408,9 +399,9 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } t.owner = to_; t.index = uint88(newIndex); - $.owned[to_].push(tokenId); + $.owned[to_].push(id_); - emit ERC721Transfer(from_, to_, tokenId); + emit ERC721Transfer(from_, to_, id_); } /// @notice Mint ERC20 tokens without triggering NFT creation @@ -426,16 +417,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is if (to_ == address(0)) { revert InvalidRecipient(); } - if (nftId_ == 0 || nftId_ >= ID_ENCODING_PREFIX - 1) { - revert InvalidTokenId(); - } + _validateTokenId(nftId_); TokenStorage storage $ = _getS(); - // Add the ID_ENCODING_PREFIX to the provided ID - uint256 id = _encodeMintId(nftId_); - - TokenData storage t = $.tokens[id]; + TokenData storage t = $.tokens[nftId_]; // Check if this NFT already exists (including null-owner) if (t.exists) { @@ -443,7 +429,7 @@ abstract contract ERC404NullOwnerCappedUpgradeable is } t.exists = true; - _transferERC721(address(0), to_, id); + _transferERC721(address(0), to_, nftId_); // Increment minted supply counter $.minted++; @@ -453,37 +439,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is // HELPER FUNCTIONS // ============================================================= - /// @dev Normalizes caller input into the encoded tokenId form (adds prefix for human mint IDs). - function _normalizeTokenId(uint256 id_) internal pure returns (uint256 tokenId_) { + /// @dev Simple tokenId validation: nonzero and not max uint256. + function _validateTokenId(uint256 id_) internal pure { if (id_ == 0 || id_ == type(uint256).max) { revert InvalidTokenId(); } - - tokenId_ = id_ < ID_ENCODING_PREFIX ? _encodeMintId(id_) : id_; - - if (tokenId_ <= ID_ENCODING_PREFIX || tokenId_ == type(uint256).max) { - revert InvalidTokenId(); - } - } - - /// @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) { - revert InvalidTokenId(); - } - uint256 tokenId = ID_ENCODING_PREFIX + mintId_; - if (tokenId == type(uint256).max) { - revert InvalidTokenId(); - } - return tokenId; - } - - /// @dev Decodes an encoded tokenId back to the human-friendly mintId. - function _decodeTokenId(uint256 tokenId_) internal pure returns (uint256) { - if (tokenId_ <= ID_ENCODING_PREFIX || tokenId_ == type(uint256).max) { - revert InvalidTokenId(); - } - return tokenId_ - ID_ENCODING_PREFIX; } // ============================================================= diff --git a/contracts/test/ERC404FixedDenominationNullOwner.t.sol b/contracts/test/ERC404FixedDenominationNullOwner.t.sol index 6ab5496..90a7c33 100644 --- a/contracts/test/ERC404FixedDenominationNullOwner.t.sol +++ b/contracts/test/ERC404FixedDenominationNullOwner.t.sol @@ -108,13 +108,13 @@ contract ERC404FixedDenominationNullOwnerTest is TestSetup { // Mint to bob mintNote(tokenAddr, "TNULL", 1, 1000, bytes32(uint256(0xAAAA)), bob); assertEq(token.balanceOf(bob), 1000 * 1e18); - assertEq(token.ownerOf(token.ID_ENCODING_PREFIX() + 1), bob); + assertEq(token.ownerOf(1), bob); assertEq(token.totalSupply(), 1000 * 1e18); // Mint to null owner (initialOwner = 0) should end with 0x0 owning NFT and ERC20 mintNote(tokenAddr, "TNULL", 2, 1000, bytes32(uint256(0xBBBB)), address(0)); assertEq(token.balanceOf(address(0)), 1000 * 1e18); - assertEq(token.ownerOf(token.ID_ENCODING_PREFIX() + 2), address(0)); + assertEq(token.ownerOf(2), address(0)); assertEq(token.totalSupply(), 2000 * 1e18); } @@ -132,7 +132,7 @@ contract ERC404FixedDenominationNullOwnerTest is TestSetup { assertEq(token.totalSupply(), supplyBefore); assertEq(token.balanceOf(address(0)), 1000 * 1e18); - assertEq(token.ownerOf(token.ID_ENCODING_PREFIX() + 1), address(0)); + assertEq(token.ownerOf(1), address(0)); } function testCapEnforcedOnMint() public {