tests: Add test for erc721 #111

Otaiki1 · 2025-10-25T22:01:42Z

Summary

Added comprehensive Foundry tests for ERC721 implementations covering both basic ERC721Facet and ERC721EnumerableFacet functionality.

Changes Made

Created test/ERC721/ERC721.t.sol - 34 tests for ERC721Facet covering minting, burning, transfers, approvals, safe transfers, error handling, and events
Created test/ERC721/ERC721Enumerable.t.sol - 34 tests for ERC721EnumerableFacet covering enumeration features, token indexing, and complex transfer scenarios
Added test helper contracts - TestableERC721Facet and TestableERC721EnumerableFacet with custom mint/burn functions
Created ERC721ReceiverMock - Mock contract for testing safe transfers with configurable behavior
Fixed ERC721Enumerable library bug - Missing ownerOf mapping assignment in mint function
Added fuzz tests - 5 randomized tests for comprehensive input validation

Checklist

Before submitting this PR, please ensure:

Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct
Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
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
Documentation updated - If applicable, update relevant documentation

Make sure to follow the CONTRIBUTING.md guidelines.

Additional Notes

73 total tests (34 ERC721 + 34 ERC721Enumerable + 5 fuzz tests) all passing
Test coverage includes: minting, burning, transfers, approvals, enumeration, error conditions, events, and edge cases
Fixed storage structure mismatch between ERC721EnumerableFacet and its library
Tests use proper diamond storage patterns
This PR closes Add foundry tests for the ERC721 implementations #11

mudgen · 2025-10-26T02:03:34Z

@Otaiki1 Thank you! We recently added tests for ERC20 functionality. Can you please review the tests for the ERC20 functionality and see that your new tests are consistent with them. It looks like you took the same/similar approach which is good. Also please review this new testing documentation for the project: https://github.com/Perfect-Abstractions/Compose/blob/main/test/README.md and make any changes you think are necessary to follow that.

panditdhamdhere · 2025-10-26T03:15:59Z

Great work legend! 👏🏼

Can you do following before merging?

Create test/README.md documentation
Add specific tests for the enumeration bug fix

Otaiki1 · 2025-10-26T09:01:36Z

@mudgen , i have made all appropriate changes

panditdhamdhere

Excellent job!

mudgen · 2025-10-26T17:41:10Z

Looks good. @adamgall, can you review this pull request?

github-actions · 2025-10-26T17:41:33Z

Coverage Report

Metric	Coverage	Details
Lines	68%	817/1195 lines
Functions	80%	181/227 functions
Branches	51%	95/186 branches

Last updated: Wed, 29 Oct 2025 01:12:46 GMT for commit 13b099c

adamgall

Thanks for this PR @Otaiki1! Some feedback:

We don't need an ERC721-specific README file; you can delete that please.
this PR is missing harnesses and tests for LibERC721 and LibERC721Enumerable. Please look at the existing test files in this repo for more inspiration and patterns.
ideally could include some tests which include gas benchmarks (confirm some function doesn't use more than x gas).

Otaiki1 · 2025-10-28T18:17:33Z

@adamgall , i have made the specified changes

adamgall · 2025-10-29T19:57:45Z

@Otaiki1 can you please run forge fmt?

adamgall

Just a couple of comments. Also, please run forge fmt. Thank you!

adamgall · 2025-10-29T20:01:09Z

test/ERC721/harnesses/LibERC721EnumerableHarness.sol

+    /// @notice Exposes LibERC721Enumerable.mint as an external function
+    /// @dev Note: This fixes the library bug by manually setting ownerOf
+    function mint(address _to, uint256 _tokenId) external {
+        LibERC721Enumerable.mint(_to, _tokenId);
+        // Fix the library bug by manually setting ownerOf
+        LibERC721Enumerable.ERC721EnumerableStorage storage s = LibERC721Enumerable.getStorage();
+        s.ownerOf[_tokenId] = _to;
+    }


I think we should probably just fix the bug in this PR... what do you think @mudgen @maxnorm ?

Seems like the mint function in LibERC721Enumerable doesn't set ownerOf for the new token being minted.

@adamgall
Oh yea, that is a bug. We need to fix that. I added a bug issue: #159

@mudgen @adamgall Since you are more aware for this PR, 2 new PRs were submitted for the fix.

#160
#161

PR #160 provide only the fix and #161 also implemented tests for ERC721Enumerable.

Can be worth to compare the tests between this PR and the other.

@Otaiki1 #160 has been merged, so can you please merge main into your branch and update the LibERC721EnumerableHarness to remove the "fix" for the ownerOf bug.

test/ERC721/harnesses/ERC721EnumerableFacetHarness.sol

Otaiki1 · 2025-10-30T08:44:38Z

@adamgall , all implemented

adamgall · 2025-11-01T21:14:08Z

@Otaiki1 #111 (comment)

mudgen · 2025-11-03T17:58:30Z

@Otaiki1 I think we have some failing tests. I am eager to get this pull request merged. I appreciate your work.

mudgen · 2025-11-07T12:55:54Z

@Otaiki1 bump

tests: Add test for erc721

f372fec

Otaiki1 added 3 commits October 26, 2025 09:35

Merge branch 'main' into tests/erc721

2712481

fix: modify tests to suit testing documentation

efa8e16

feat: addd test doc ; refac: add spec. test for bugfix

4ab592a

panditdhamdhere approved these changes Oct 26, 2025

View reviewed changes

adamgall suggested changes Oct 26, 2025

View reviewed changes

Otaiki1 added 3 commits October 27, 2025 08:16

Merge branch 'main' into tests/erc721

bc2528d

remove unnecessary parts

317c999

Merge branch 'main' into tests/erc721

13b099c

maxnorm requested a review from adamgall October 28, 2025 23:56

adamgall suggested changes Oct 29, 2025

View reviewed changes

Otaiki1 added 2 commits October 30, 2025 09:00

Merge branch 'main' into tests/erc721

bc4052d

fix: implement changes

fc486de

mudgen mentioned this pull request Oct 31, 2025

[BUG] LibERC721Enumerable does not set owner of new tokens when minting #159

Closed

sidarth16 mentioned this pull request Oct 31, 2025

Fix #159 : LibERC721Enumerable.sol now set token owner during mint to resolve missing ownership #160

Merged

adamgall mentioned this pull request Oct 31, 2025

fix: set ownerOf in LibERC721Enumerable mint function #161

Closed

mudgen closed this Nov 16, 2025

mudgen reopened this Nov 16, 2025

mudgen added the Stale Work needs an update from the person working on it. label Nov 16, 2025

mudgen added the Request Status Update Requesting the author to give an update on the status of the work. label Nov 16, 2025

mudgen closed this Nov 23, 2025

tests: Add test for erc721 #111

tests: Add test for erc721 #111

Uh oh!

Conversation

Otaiki1 commented Oct 25, 2025

Summary

Changes Made

Checklist

Additional Notes

Uh oh!

mudgen commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panditdhamdhere commented Oct 26, 2025

Uh oh!

Otaiki1 commented Oct 26, 2025

Uh oh!

panditdhamdhere left a comment

Choose a reason for hiding this comment

Uh oh!

mudgen commented Oct 26, 2025

Uh oh!

github-actions bot commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

adamgall left a comment

Choose a reason for hiding this comment

Uh oh!

Otaiki1 commented Oct 28, 2025

Uh oh!

adamgall commented Oct 29, 2025

Uh oh!

adamgall left a comment

Choose a reason for hiding this comment

Uh oh!

adamgall Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

mudgen Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxnorm Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamgall Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Otaiki1 commented Oct 30, 2025

Uh oh!

adamgall commented Nov 1, 2025

Uh oh!

mudgen commented Nov 3, 2025

Uh oh!

mudgen commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mudgen commented Oct 26, 2025 •

edited

Loading

github-actions bot commented Oct 26, 2025 •

edited

Loading

mudgen Oct 31, 2025 •

edited

Loading

maxnorm Oct 31, 2025 •

edited

Loading

adamgall Oct 31, 2025 •

edited

Loading