Skip to content

Refactor token ID handling in ERC20FixedDenomination and ERC404NullOw… #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RogerPodacter merged 2 commits into from
Nov 24, 2025
Merged

Refactor token ID handling in ERC20FixedDenomination and ERC404NullOw… #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ab3767
Refactor token ID handling in ERC20FixedDenomination and ERC404NullOw…
RogerPodacter Nov 24, 2025
83ef4a3
Refactor token ID handling and improve metadata functions in ERC20Fix…
RogerPodacter Nov 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions contracts/src/ERC20FixedDenomination.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ contract ERC20FixedDenomination is ERC404NullOwnerCappedUpgradeable {
_transferERC20(from, to, units());

// Transfer the specific NFT using the proper function
uint256 id = ID_ENCODING_PREFIX + nftId;
_transferERC721(from, to, id);
_transferERC721(from, to, nftId);
}

// =============================================================
Expand Down Expand Up @@ -154,25 +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) {
// This will revert InvalidTokenId / NotFound on bad ids
ownerOf(id_);

uint256 mintId = id_ & ~ID_ENCODING_PREFIX;
function tokenURI(uint256 mintId) public view virtual override returns (string memory) {
_validateTokenId(mintId);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
_validateTokenId(mintId);

Copilot uses AI. Check for mistakes.
ownerOf(mintId); // reverts on invalid / nonexistent
Copy link

@cursor cursor bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web


// Get the ethscriptionId for this mintId from the manager
ERC20FixedDenominationManager mgr = ERC20FixedDenominationManager(manager);
bytes32 ethscriptionId = mgr.getMintEthscriptionId(deployEthscriptionId, mintId);

Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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)));
}

Copilot uses AI. Check for mistakes.
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);
Expand All @@ -183,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"'
);

Expand Down
66 changes: 32 additions & 34 deletions contracts/src/ERC404NullOwnerCappedUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,15 @@ 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
// =============================================================

// 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);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
event ERC20Transfer(address indexed from, address indexed to, uint256 value);

Copilot uses AI. Check for mistakes.
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);
Expand Down Expand Up @@ -125,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_;
Expand Down Expand Up @@ -173,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();
Expand Down Expand Up @@ -202,16 +207,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is
}

function ownerOf(uint256 id_) public view virtual override(IERC404) returns (address) {
if (!_isValidTokenId(id_)) {
revert InvalidTokenId();
}

_validateTokenId(id_);
TokenStorage storage $ = _getS();
TokenData storage t = $.tokens[id_];

if (!t.exists) {
revert NotFound();
}
if (!t.exists) revert NotFound();

return t.owner;
}
Expand All @@ -222,7 +222,10 @@ abstract contract ERC404NullOwnerCappedUpgradeable is
}

function getApproved(uint256 id_) public view virtual returns (address) {
_validateTokenId(id_);
TokenStorage storage $ = _getS();
if (!$.tokens[id_].exists) revert NotFound();

return $.getApproved[id_];
}

Expand Down Expand Up @@ -347,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
Expand All @@ -367,16 +367,16 @@ abstract contract ERC404NullOwnerCappedUpgradeable is
}

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_)) {
revert Unauthorized();
}

if (!t.exists) revert NotFound();
if (from_ != t.owner) revert Unauthorized();

if (from_ != address(0)) {
// Clear approval
Expand Down Expand Up @@ -417,24 +417,19 @@ 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 = ID_ENCODING_PREFIX + nftId_;

TokenData storage t = $.tokens[id];
TokenData storage t = $.tokens[nftId_];

// Check if this NFT already exists (including null-owner)
if (t.exists) {
revert AlreadyExists();
}

t.exists = true;
_transferERC721(address(0), to_, id);
_transferERC721(address(0), to_, nftId_);

// Increment minted supply counter
$.minted++;
Expand All @@ -444,8 +439,11 @@ abstract contract ERC404NullOwnerCappedUpgradeable is
// HELPER FUNCTIONS
// =============================================================

function _isValidTokenId(uint256 id_) internal pure returns (bool) {
return id_ > ID_ENCODING_PREFIX && id_ != type(uint256).max;
/// @dev Simple tokenId validation: nonzero and not max uint256.
function _validateTokenId(uint256 id_) internal pure {
if (id_ == 0 || id_ == type(uint256).max) {
revert InvalidTokenId();
}
}

// =============================================================
Expand Down
6 changes: 3 additions & 3 deletions contracts/test/ERC404FixedDenominationNullOwner.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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 {
Expand Down