Add comprehensive test suite for ERC20Facet and LibERC20 #78
Conversation
This PR adds a complete test suite with 93 tests covering all functionality of ERC20Facet and LibERC20, including edge cases and security considerations. ## Test Coverage ### ERC20Facet Tests (56 tests) - Metadata: name, symbol, decimals, totalSupply - Transfers: transfer, transferFrom with all validations - Approvals: approve, allowance edge cases - Burn: burn, burnFrom with correct balance tracking - EIP-2612 Permit: signature validation, nonce handling, replay protection - DOMAIN_SEPARATOR: chain fork detection - Security: overflow protection, zero address validation - Fuzz Testing: comprehensive fuzzing of all core functions ### LibERC20 Tests (37 tests) - Internal Functions: direct testing of library functions - Minting: mint with overflow protection - Complete Flows: mint → transfer → burn sequences - Edge Cases: zero amounts, self-transfers, maximum values ## Architecture ### Test Harnesses - ERC20FacetHarness: Adds initialize() and mint() for testing the Facet - LibERC20Harness: Exposes internal library functions as external ### Organization - Separate test files for Facet (ERC20Facet.t.sol) and Library (LibERC20.t.sol) - Clean separation of concerns with focused test suites - Comprehensive documentation in test/README.md ## Key Features - ✅ All 93 tests passing - ✅ Full coverage of standard ERC20 functionality - ✅ Complete EIP-2612 permit implementation tests - ✅ Extensive edge case and security testing - ✅ Fuzz testing for critical paths - ✅ Well-documented testing approach This implementation provides the most thorough test coverage and follows best practices for testing diamond facets and their libraries.
|
hi! great work! for comprehensive test, is it necessary to add decimal variable that is commonly used like 6 and 8 instead of only 18? what do you think? in case this Facet is used as a wrapper/synthetic of popular 6/8 decimal token |
- Add tests for unlimited allowance optimization (commit 4092fbd) - test_TransferFrom_UnlimitedAllowance: verify allowance remains max after transfers - test_TransferFrom_UnlimitedAllowance_MultipleTransfers: test persistence across multiple ops - test_BurnFrom_UnlimitedAllowance: verify allowance remains max after burns - test_BurnFrom_UnlimitedAllowance_MultipleBurns: test persistence across multiple burns - Add tests for EIP-20 boolean return values (commit 318fe4d) - test_Transfer_ReturnsTrue: verify transfer returns true - test_Approve_ReturnsTrue: verify approve returns true - test_TransferFrom_ReturnsTrue: verify transferFrom returns true - Update existing tests to capture and assert return values All tests verify gas optimization for type(uint256).max allowances and EIP-20 compliance for return values.
| @@ -0,0 +1,272 @@ | |||
| # Compose Test Suite | |||
There was a problem hiding this comment.
This documentation looks great. I'm thinking this documentation should go into the CONTRIBUTING.md file (and what's currently in CONTRIBUTING.md about testing should go in README.md), because we are keeping README.md with more concise information and more detailed information in CONTRIBUTING.md.
There was a problem hiding this comment.
I'd defer to @maxnorm on that, they originally created the CONTRIBUTING.md doc.
There was a problem hiding this comment.
however, to give my own opinion:
- README.md
- imo, shouldn't include "codebase implementation details", like explaining how tests work or what their directory structure is.
- i would even say that the whole "Quick Start" section should be rewritten to speak to the end user of this library, not the developer building it. Those instructions should move into CONTRIBUTING.md
- this file should target end users: library consumers. Explain what the project is, who it's for and why it helps them, clear instructions on how to "install it" or "use it", whatever that might look like, some examples of what an implementation into a client codebase looks like, and some links to external docs (when those exist) and CONTRIBUTING.md and legal stuff, etc.
- I'm taking inspiration from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/README.md
- CONTRIBUTING.md
- i like how this is right now. it's very full of information for other developers working on this codebase
- for as important as testing is, IMO it commands its own dedicated document (test/README/md).
- the "testing" section in this document calls out the high level structure and important parts, then directs the reader/developer to the specific test/README.md doc for full details on how to follow this project's testing conventions.
- ./test/README.md
- Keep mostly all of this content (there's some slop at the end that we can probably delete), right here in this file.
- it's extensive enough that IMO it would clutter CONTRIBUTING.md, if we merged it in.
There was a problem hiding this comment.
@adamgall I agree with you. Originally the README was written for contributors because the library is currently only open to contributors. But I think we should start changing the README, to make it written for the end user, like you described.
When I wrote the comment/feedback I didn't realize that this README is in the test directory. I thought you were including this content in the main README file, which is was my misunderstanding. Sorry about that. Great work.
|
@adamgall I looked over your tests and they look pretty good to me. I like your harness approach. @farismln left a comment about testing an ERC20 token that is 6 or 8 decimals. I am interested to know what you think about that. Do you think it makes sense to test that with our current implementations? |
|
@adamgall I changed the directory structure. Please update the directory paths to LibERC20.sol etc. |
I personally would say that it doesn't make much functional sense to "test" various decimals. There is nowhere in the code where the We have a test for this function already, to make sure that "18" is returned when we initialize it with "18". Do we gain more confidence if we test that initializing with "6" returns a "6"? I would say no. |
Merged and fixed. All tests passing still. Looks like the new slither job is failing though. Seems out of scope for this PR to fix those issues here. |
Coverage Report
Last updated: Sun, 26 Oct 2025 00:15:21 GMT for commit |
|
Looks great, thanks @adamgall! |
…est-suite Add comprehensive test suite for ERC20Facet and LibERC20
3 participants
Closes #12
Closes #59
This PR adds a complete test suite with 93 tests covering all functionality of ERC20Facet and LibERC20, including edge cases and security considerations.
Test Coverage
ERC20Facet Tests (63 tests)
LibERC20 Tests (37 tests)
Architecture
Test Harnesses
Organization
Key Features
This implementation provides thorough test coverage and follows best practices for testing diamond facets and their libraries.