Added tests #975
Added tests #975
Conversation
WalkthroughThe pull request updates several Solidity test contracts by adding new test functions and error handling scenarios. Enhancements span access control validations, chain ID lookups, bridge refund operations, and deBridge chain ID management. New events, error types, and import statements are introduced where necessary. In addition, a mock contract is added to simulate external call failures, ensuring various revert scenarios are thoroughly tested. Changes
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/solidity/Facets/DexManagerFacet.t.sol (1)
273-274: Consider using unchecked block for gas optimization.The loop counters in these test functions could use unchecked blocks for gas optimization, similar to the pattern used in the existing test on line 128.
Apply this diff to optimize the loops:
- for (uint256 i = 0; i < 3; i++) { + for (uint256 i = 0; i < 3; ) { assertTrue(dexMgr.isFunctionApproved(signatures[i])); + unchecked { ++i; } } - for (uint256 i = 0; i < 3; i++) { + for (uint256 i = 0; i < 3; ) { assertFalse(dexMgr.isFunctionApproved(signatures[i])); + unchecked { ++i; } }Also applies to: 278-279
test/solidity/Facets/DeBridgeDlnFacet.t.sol (1)
411-430: LGTM! Good low-level storage testing.The test effectively validates storage slot computation and access. Consider adding a comment explaining the storage layout pattern being tested.
Add a comment explaining the diamond storage pattern:
function test_getStorage() public { + // Test diamond storage pattern where each facet's storage is namespaced + // to avoid collisions with other facets' storage variables bytes32 namespace = keccak256("com.lifi.facets.debridgedln");test/solidity/Facets/AmarokFacetPacked.t.sol (1)
416-513: LGTM! Comprehensive chain ID mapping tests.The test suite thoroughly covers chain ID mappings for all major chains and handles invalid cases. Consider refactoring to use parameterized testing for better maintainability.
Consider refactoring the chain-specific tests into a parameterized test:
struct ChainTestCase { string name; uint32 domainId; uint32 expectedChainId; } function test_getChainIdForDomain() public { ChainTestCase[] memory testCases = new ChainTestCase[](7); testCases[0] = ChainTestCase("ETH", 6648936, 1); testCases[1] = ChainTestCase("POL", 1886350457, 137); // ... add other cases for (uint i = 0; i < testCases.length; i++) { ChainTestCase memory tc = testCases[i]; uint32 result = amarokFacetPacked.getChainIdForDomain(tc.domainId); assertEq( result, tc.expectedChainId, string.concat(tc.name, " domainId should return correct chainId") ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
test/solidity/Facets/AccessManagerFacet.t.sol(3 hunks)test/solidity/Facets/AcrossFacetPacked.t.sol(2 hunks)test/solidity/Facets/AcrossFacetV3.t.sol(2 hunks)test/solidity/Facets/AmarokFacetPacked.t.sol(3 hunks)test/solidity/Facets/CBridge.t.sol(4 hunks)test/solidity/Facets/CBridgeFacetPacked.t.sol(2 hunks)test/solidity/Facets/DeBridgeDlnFacet.t.sol(4 hunks)test/solidity/Facets/DexManagerFacet.t.sol(5 hunks)test/solidity/utils/MockFailingContract.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (18)
test/solidity/Facets/DexManagerFacet.t.sol (4)
9-9: LGTM! Import statement added for error types.The import statement is correctly added to support the new authorization test cases.
50-56: LGTM! Improved test accuracy with explicit caller context.The addition of
vm.startPrank/stopPrankcalls consistently ensures that all operations are performed with the correct owner context across all test cases.Also applies to: 60-67, 71-84, 88-105, 109-115, 119-135, 139-143, 147-151, 155-163, 167-175
178-246: LGTM! Comprehensive authorization test coverage added.The new test cases thoroughly verify access control by:
- Testing all owner-restricted functions
- Verifying proper error handling for unauthorized access
- Including edge cases like self-authorization checks
248-283: LGTM! Complete function approval lifecycle tests added.The new test cases properly verify:
- Both single and batch operations
- Both enabling and disabling of function approvals
- Proper state verification through assertions
test/solidity/utils/MockFailingContract.sol (1)
4-8: LGTM! Well-designed mock contract.The contract is well-designed for testing failure scenarios with a clear, single responsibility.
test/solidity/Facets/AccessManagerFacet.t.sol (3)
25-30: LGTM! Comprehensive setup of function selectors.The setup correctly includes both setCanExecute and addressCanExecuteMethod selectors.
75-82: LGTM! Good test for self-authorization prevention.Test verifies that a contract cannot authorize itself, which is a critical security check.
84-156: LGTM! Thorough test coverage for access control.Excellent set of test cases covering:
- Default access state
- Access granting
- Access revocation
- Different method selectors
- Different executors
test/solidity/Facets/CBridge.t.sol (3)
31-35: LGTM! Well-structured event definition.The CBridgeRefund event properly indexes relevant parameters for efficient filtering.
206-227: LGTM! Thorough success case testing.Test properly sets up the environment, mocks external calls, and verifies event emission.
229-285: LGTM! Comprehensive error case coverage.Tests cover all critical error scenarios:
- Unauthorized caller
- Invalid call address
- External call failures
test/solidity/Facets/AcrossFacetV3.t.sol (1)
276-293: LGTM! Good validation of information consistency.Test properly verifies that mismatched receiver addresses between bridgeData and validAcrossData are caught.
test/solidity/Facets/DeBridgeDlnFacet.t.sol (3)
34-37: LGTM! Well-structured error and event declarations.The error and event declarations follow best practices with clear naming and proper parameter indexing.
44-57: LGTM! Proper test setup initialization.The setUp function is correctly updated to include new function selectors for the deBridge chain ID management functionality.
349-394: LGTM! Comprehensive test coverage for deBridge chain ID management.The test suite effectively covers:
- Setting and retrieving chain IDs
- Handling uninitialized facet errors
- Handling unknown chain ID errors
- Event emission verification
test/solidity/Facets/CBridgeFacetPacked.t.sol (1)
495-551: LGTM! Thorough error handling tests for triggerRefund.The test suite comprehensively covers error scenarios:
- Unauthorized access control
- Invalid contract address validation
- External call failure handling
test/solidity/Facets/AmarokFacetPacked.t.sol (1)
50-76: LGTM! Proper test setup for new functionality.The function selectors array is correctly updated to include the getChainIdForDomain selector.
test/solidity/Facets/AcrossFacetPacked.t.sol (1)
621-633: LGTM! Good failure scenario testing.The test effectively validates the contract's behavior when a withdrawal operation fails using a mock contract.
Test Coverage ReportLine Coverage: 80.14% (2280 / 2845 lines) |
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)