Skip to content

Conversation

@ogazboiz
Copy link
Contributor

@ogazboiz ogazboiz commented Nov 25, 2025

Summary

Implemented comprehensive test coverage for ERC721Enumerable and LibERC721Enumerable as requested. This adds robust testing for enumeration, transfers, approvals, and mint/burn operations using Foundry.

Changes Made

  • Created test/token/ERC721/ERC721Enumerable/harnesses/ERC721EnumerableFacetHarness.sol: Test harness to expose facet functionality.
  • Created test/token/ERC721/ERC721Enumerable/harnesses/LibERC721EnumerableHarness.sol: Test harness to expose library functions.
  • Created test/token/ERC721/ERC721Enumerable/ERC721EnumerableFacet.t.sol: Comprehensive tests for the facet.
  • Created test/token/ERC721/ERC721Enumerable/LibERC721Enumerable.t.sol: Comprehensive tests for the library.

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 (Applied to production code; harnesses use inheritance for testing purposes as per existing patterns).

  • 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

  • Existing tests pass - Run tests to be sure existing tests pass.

  • New tests are optional - Added comprehensive tests for the requested components.

  • All tests pass - Run forge test and ensure everything works

  • Documentation updated - N/A (Test implementation only)

Make sure to follow the contributing guidelines.

Additional Notes

Test harnesses were created following the pattern in ERC721FacetHarness.sol to expose internal functions and initialize storage for testing. All 86 new tests are passing.

@netlify
Copy link

netlify bot commented Nov 25, 2025

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f852c33

feat(test): add comprehensive tests for ERC721Enumerable and LibERC72…
@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Coverage Report

Coverage

Metric Coverage Details
Lines 91% 1820/1993 lines
Functions 92% 345/376 functions
Branches 77% 257/332 branches

Last updated: Tue, 25 Nov 2025 16:28:22 GMT for commit 6a7e0e4

@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Gas Report

No gas usage changes detected between main and main.

All functions maintain the same gas costs. ✅

Last updated: Tue, 25 Nov 2025 16:29:03 GMT for commit 6a7e0e4

@ogazboiz
Copy link
Contributor Author

@maxnorm can you review

@ogazboiz ogazboiz closed this Nov 25, 2025
ogazboiz and others added 2 commits November 25, 2025 12:29
test: add comprehensive tests for ERC721Enumerable and LibERC721Enume…
@ogazboiz ogazboiz reopened this Nov 25, 2025
@mudgen
Copy link
Contributor

mudgen commented Nov 25, 2025

@akronim26 Can you please review this?

@mudgen
Copy link
Contributor

mudgen commented Nov 25, 2025

@ogazboiz Can you please run forge fmt to remove the formatting error?

@ogazboiz
Copy link
Contributor Author

@ogazboiz Can you please run forge fmt to remove the formatting error?

@mudgen I've run forge fmt and fixed the formatting issues. I initially didn't touch these files since they weren't part of my original changes, but I've now formatted them to match CI requirements. All changes have been committed and pushed.

@akronim26
Copy link
Contributor

@akronim26 Can you please review this?

Yeah sure @mudgen! @ogazboiz, please run forge update to fix this.
Screenshot from 2025-11-26 03-54-10

@ogazboiz
Copy link
Contributor Author

@

@akronim26 Can you please review this?

Yeah sure @mudgen! @ogazboiz, please run forge update to fix this. Screenshot from 2025-11-26 03-54-10

yea i have done that already

@ogazboiz
Copy link
Contributor Author

@mudgen @maxnorm can you review it

@mudgen
Copy link
Contributor

mudgen commented Nov 26, 2025

yea i have done that already

Thanks. Push the change to the pull request.

@ogazboiz
Copy link
Contributor Author

ogazboiz commented Nov 26, 2025

yea i have done that already

Thanks. Push the change to the pull request.

did that yesterday @mudgen
all checked passed

@mudgen
Copy link
Contributor

mudgen commented Nov 26, 2025

yea i have done that already

Thanks. Push the change to the pull request.

did that yesterday @mudgen all checked passed

Okay, thanks. We still have a problem because if you look at the pull request you can see this:
Screenshot from 2025-11-26 09-19-52

So maybe try it again and see if that solves the problem: forge update and commit the change to the pull request.
Please tell me what version of forge you are using with forge --version. Thanks.

Actually I found out that forge update is the wrong way to fix this. The right way is to run forge install. Can you run that and commit?

@mudgen
Copy link
Contributor

mudgen commented Nov 26, 2025

@ogazboiz Actually I found out that forge update is the wrong way to fix this. The right way is to run forge install. Can you run that and commit?

@ogazboiz
Copy link
Contributor Author

@mudgen Done! I've run forge install and updated the forge-std submodule. My forge version is 1.2.3-stable. The submodule has been updated to commit 7117c90 and the changes have been committed and pushed.

@mudgen
Copy link
Contributor

mudgen commented Nov 27, 2025

@ogazboiz Okay. I suggest you update your forge version to the latest stable version, by running foundryup.

Your pull request is modifying the package.json file in lib/forge-std. Please undo that change so that I can merge your pull request.

@ogazboiz
Copy link
Contributor Author

yea i have done that already

Thanks. Push the change to the pull request.

did that yesterday @mudgen all checked passed

Okay, thanks. We still have a problem because if you look at the pull request you can see this: Screenshot from 2025-11-26 09-19-52

So maybe try it again and see if that solves the problem: forge update and commit the change to the pull request. Please tell me what version of forge you are using with forge --version. Thanks.

Actually I found out that forge update is the wrong way to fix this. The right way is to run forge install. Can you run that and commit?

to this right ?

because i have done foundryup but still saying nothing to commit @mudgen

@mudgen
Copy link
Contributor

mudgen commented Nov 27, 2025

@ogazboiz Thanks. You can check if it is right by looking at your pull request and seeing if you are modifying any of the files in lib/forge-std. You will need to fix this somehow until it is fixed. Run forge install. And commit and push. Then look at your pull request to see if you are changing any of the files in lib/forge-std. If you are then you will need to reverse/undo those changes and commit and push. Sorry about the hassle.

@ogazboiz ogazboiz closed this by deleting the head repository Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants