fix: set ownerOf in LibERC721Enumerable mint function#161
Closed
Abidoyesimze wants to merge 1 commit into
Closed
Conversation
The mint function in LibERC721Enumerable was missing the critical s.ownerOf[_tokenId] = _to assignment, causing newly minted tokens to have no recorded owner. This resulted in ownerOf() queries failing and transfers being impossible. This fix adds the missing line to properly set token ownership during minting, matching the implementation in the standard LibERC721 library. Includes comprehensive test coverage: - 32 tests for LibERC721Enumerable (mint, burn, transfer, enumeration) - 42 tests for ERC721EnumerableFacet (all public functions)
Contributor
Coverage Report
Last updated: Fri, 31 Oct 2025 13:29:17 GMT for commit |
Contributor
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Fri, 31 Oct 2025 13:29:34 GMT for commit |
maxnorm
requested review from
adamgall and
mudgen
and removed request for
mudgen
Collaborator
|
Hey @Abidoyesimze , thank you for your work on this! I'm going to close this PR without merging. My reasoning is as follows:
Now that the bug is fixed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the critical bug in LibERC721Enumerable.mint() where the ownerOf mapping was not being set for newly minted tokens. This caused minted tokens to be unusable - they existed in enumeration arrays but had no recorded owner, making ownerOf() queries fail and preventing any transfers.
The fix adds the single missing line s.ownerOf[_tokenId] = _to; to properly record token ownership during minting, matching the implementation in the standard LibERC721 library.
Changes Made
Bug Fix
src/token/ERC721/ERC721Enumerable/LibERC721Enumerable.sol: Added missing s.ownerOf[_tokenId] = _to assignment in mint() function (line 126)
Test Coverage Added
test/token/ERC721/ERC721Enumerable/LibERC721Enumerable.t.sol: 32 comprehensive tests for the library's internal functions (mint, burn, transfer, enumeration)
test/token/ERC721/ERC721Enumerable/ERC721EnumerableFacet.t.sol: 42 comprehensive tests for the facet's public interface
test/token/ERC721/ERC721Enumerable/harnesses/: Test harnesses to expose internal library functions for testing
Code is formatted with forge fmt
Tests are included - All new functionality has comprehensive tests
All tests pass - Run forge test and ensure everything works
Test Results
All 429 tests pass successfully:
Test Coverage Details
LibERC721Enumerable: 32 tests covering mint (with owner verification), burn, transfer, enumeration tracking, edge cases, and fuzz tests
ERC721EnumerableFacet: 42 tests covering all public functions including metadata, balance/ownership queries, enumeration, approvals, transfers, and safe transfers
The tests specifically validate the bug fix by asserting that ownerOf() returns the correct address after minting, which was the core issue.