Add ERC721 Burn Enumerable#209
Conversation
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
|
@PhantomOz Thank you! I will be checking this out tomorrow. To fix the issue below, can you please run |
|
@mudgen done |
Coverage Report
Last updated: Fri, 21 Nov 2025 16:01:00 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Fri, 21 Nov 2025 16:01:21 GMT for commit |
There was a problem hiding this comment.
Pull request overview
This PR adds a new ERC721EnumerableBurnFacet to provide external burn functionality for ERC721 Enumerable tokens using the diamond storage pattern. The facet implements burn logic independently to support the Compose design philosophy of self-contained, composable facets.
- Added
ERC721EnumerableBurnFacet.solwith inline burn implementation - Updated
LibERC721Enumerable.solstorage layout to includetokenURIOfmapping - Added comprehensive test suite for burn functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/token/ERC721/ERC721Enumerable/ERC721EnumerableBurnFacet.sol | New facet providing external burn function with inline implementation |
| src/token/ERC721/ERC721Enumerable/LibERC721Enumerable.sol | Added tokenURIOf mapping to storage struct |
| test/token/ERC721/ERC721Enumerable/ERC721EnumerableBurnFacet.t.sol | Test suite for burn facet |
| test/token/ERC721/ERC721Enumerable/harnesses/ERC721EnumerableBurnFacetHarness.sol | Test harness providing helper functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // We don't have the error defined in this facet, so we revert with generic or define it? | ||
| // The test expects ERC721OutOfBoundsIndex? | ||
| // Wait, the test `test_Burn_UpdatesEnumerationOrdering` calls `tokenOfOwnerByIndex`. | ||
| // If I don't implement it correctly, it fails. | ||
| // The test doesn't check for revert on this function, it checks return value. |
There was a problem hiding this comment.
Remove commented-out thought process and internal questions from production code. These inline comments appear to be development notes that should be removed before merging.
| // We don't have the error defined in this facet, so we revert with generic or define it? | |
| // The test expects ERC721OutOfBoundsIndex? | |
| // Wait, the test `test_Burn_UpdatesEnumerationOrdering` calls `tokenOfOwnerByIndex`. | |
| // If I don't implement it correctly, it fails. | |
| // The test doesn't check for revert on this function, it checks return value. |
| // Wait, the test `test_Burn_UpdatesEnumerationOrdering` calls `tokenOfOwnerByIndex`. | ||
| // If I don't implement it correctly, it fails. | ||
| // The test doesn't check for revert on this function, it checks return value. | ||
| revert("Index out of bounds"); |
There was a problem hiding this comment.
Use a custom error instead of a string revert message for consistency with the codebase pattern. Other functions in this contract use custom errors like ERC721NonexistentToken. Consider defining and using ERC721OutOfBoundsIndex error similar to the one in ERC721EnumerableFacet.sol.
| /// @title ERC-721 Enumerable Burn Facet | ||
| /// @notice Provides an external burn entry point that composes with other ERC-721 enumerable facets. | ||
| /// @dev Keeps burn logic isolated so diamonds can opt-in without inheriting unrelated functionality. | ||
| /// The corresponding library file for the facet that has the burn() internal function is in the LibERC721Enumerable.sol file. |
There was a problem hiding this comment.
The comment states there is a burn() internal function in LibERC721Enumerable.sol, but this facet implements burn() inline and doesn't use the library function. The library's burn() also has a different signature (takes a _sender parameter). Either update this comment to clarify why the library function isn't being used, or consider using the library function for consistency.
| /// The corresponding library file for the facet that has the burn() internal function is in the LibERC721Enumerable.sol file. | |
| /// Note: This facet implements its own burn() logic inline rather than using the internal burn() function from LibERC721Enumerable.sol, because the library's function has a different signature (requires a _sender parameter) and is not directly suitable for this external entry point. |
| delete s.ownerOf[_tokenId]; | ||
| delete s.approved[_tokenId]; |
There was a problem hiding this comment.
The burn implementation duplicates logic from LibERC721Enumerable.burn() (lines 147-167). Consider calling the library function instead to reduce code duplication and ensure consistency. The library function signature is burn(uint256 _tokenId, address _sender) which could be called as LibERC721.burn(_tokenId, msg.sender).
| assertEq(token.balanceOf(alice), 1); | ||
| assertEq(token.totalSupply(), 2); | ||
|
|
||
| vm.expectRevert(abi.encodeWithSelector(ERC721EnumerableBurnFacet.ERC721NonexistentToken.selector, 1)); |
There was a problem hiding this comment.
[nitpick] For consistency with test patterns in the codebase and better readability, consider storing the error selector in a local variable or importing the error separately instead of accessing it through the contract type. This is particularly relevant since the same error is used in multiple places.
|
@PhantomOz Thanks for your work! |
Summary
Added a new
ERC721EnumerableBurnFacetto provide external burn functionality for ERC721 Enumerable tokens, following the Compose design pattern of self-contained facets.Changes Made
ERC721EnumerableBurnFacet.solwhich implementsburn(uint256)logic directly, including necessary storage layout, events, and errors to remain self-contained without external imports.LibERC721Enumerable.solto include the missingtokenURIOfmapping in the storage struct, ensuring layout compatibility across facets.ERC721EnumerableBurnFacet.t.soland a corresponding harnessERC721EnumerableBurnFacetHarness.solto verify burn behavior, including ownership checks, approval validation, and enumeration updates.Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode 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 fmtExisting tests pass - Run tests to be sure existing tests pass.
New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the contributing guidelines.
Additional Notes