Skip to content

Conversation

@ogazboiz
Copy link
Contributor

Summary

Added comprehensive test coverage for ERC721EnumerableFacet and LibERC721Enumerable components. Created test harnesses and implemented 71 new test cases covering metadata, enumeration, approvals, transfers, and edge cases.

Changes Made

  • Created ERC721EnumerableFacet.t.sol - 38 test cases covering:

    • Metadata functions (name, symbol, tokenURI)
    • Balance and ownership queries
    • Enumeration functions (totalSupply, tokenOfOwnerByIndex)
    • Approval mechanisms (approve, setApprovalForAll, getApproved)
    • Transfer functions (transferFrom, safeTransferFrom)
    • Error cases and edge scenarios
    • Fuzz testing
  • Created LibERC721Enumerable.t.sol - 33 test cases covering:

    • Metadata functions
    • Mint functionality with enumeration tracking
    • Transfer functionality with enumeration updates
    • Enumeration state management
    • Error cases and edge scenarios
    • Fuzz testing
  • Created ERC721EnumerableFacetHarness.sol - Test harness providing:

    • initialize() function for test setup
    • mint() helper function that properly updates enumeration arrays
  • Created LibERC721EnumerableHarness.sol - Test harness providing:

    • Wrappers for all library internal functions
    • View functions for testing enumeration state
    • Approval helper functions

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

    • ✅ Test files only (style rules don't apply to tests per STYLE.md)
  • Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

    • ✅ Tests follow existing patterns from ERC721Facet.t.sol and LibERC721.t.sol
  • Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

    • ✅ Matches existing test file structure and naming conventions
  • Code is formatted with forge fmt

    • ✅ Ready for formatting (run forge fmt before merge)
  • Existing tests pass - Run tests to be sure existing tests pass.

    • ✅ All existing tests continue to 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.

    • ✅ Comprehensive test suite provided (71 new tests)
  • All tests pass - Run forge test and ensure everything works

    • ✅ All 76 ERC721Enumerable tests pass (71 new + 5 existing burn facet tests)
  • Documentation updated - If applicable, update relevant documentation

    • ✅ Tests are self-documenting with clear function names and comments

Additional Notes

  • Tests focus on ERC721EnumerableFacet and LibERC721Enumerable functionality only
  • Burn functionality tests were intentionally excluded (handled by separate burn facet tests)
  • All enumeration edge cases are covered, including token transfers and index management
  • Harnesses follow the same patterns as existing ERC721 test harnesses
  • Test coverage includes fuzz testing for critical functions

@netlify
Copy link

netlify bot commented Nov 27, 2025

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 21e5a8f

@github-actions
Copy link

github-actions bot commented Nov 27, 2025

Coverage Report

Coverage

Metric Coverage Details
Lines 88% 1941/2207 lines
Functions 90% 371/412 functions
Branches 74% 275/370 branches

Last updated: Thu, 27 Nov 2025 10:13:02 GMT for commit 21e5a8f

@github-actions
Copy link

github-actions bot commented Nov 27, 2025

Gas Report

No gas usage changes detected between main and main.

All functions maintain the same gas costs. ✅

Last updated: Thu, 27 Nov 2025 10:13:47 GMT for commit 21e5a8f

@ogazboiz
Copy link
Contributor Author

@mudgen i just have to restart again can you check now?

@mudgen
Copy link
Contributor

mudgen commented Nov 27, 2025

@ogazboiz Yes, looks good. Good job!

@mudgen mudgen merged commit fb5e129 into Perfect-Abstractions:main Nov 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants