refactor(test): improve L2StandardBridge test coverage and quality #17347
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- add test for uncovered l1TokenBridge() function - convert 3 tests to fuzz tests for broader coverage: - testFuzz_withdraw_withdrawingERC20_succeeds - testFuzz_withdraw_ether_succeeds - testFuzz_withdrawTo_withdrawingERC20_succeeds - enhance test organization and documentation - add proper event expectations for fuzz tests
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
nice |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #17347 +/- ##
========================================
Coverage 95.84% 95.84%
========================================
Files 110 110
Lines 5102 5102
========================================
Hits 4890 4890
Misses 212 212
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
devin you need to run forge fmt to format this |
|
DevinAI, there are failed CI checks. Fix them. |
- Add _minGasLimit parameter to testFuzz_withdraw_withdrawingERC20_succeeds - Add _minGasLimit parameter to testFuzz_withdrawTo_withdrawingERC20_succeeds - Use consistent bounds (21000, 1000000) matching testFuzz_withdraw_ether_succeeds - Addresses alcueca's PR comment about fuzzing minGasLimit parameter
| _amount = bound(_amount, 1, 1000000); | ||
| _minGasLimit = uint32(bound(_minGasLimit, 21000, 1000000)); | ||
|
|
||
| deal(address(L2Token), alice, _amount, true); |
There was a problem hiding this comment.
Devinai, this is missing a lot of the logic included in _preBridgeERC20, are we sure we don't need that logic anymore?
Also, now we have a lot more lines and code repeated between both tests and assumedly _preBridgeERC20.
Maybe we should first see if we need to refactor _preBridgeERC20, and what is best in terms of code economy and clarity.
…uzz tests - Create parameterized _expectERC20WithdrawalCalls helper to eliminate code duplication - Add comprehensive vm.expectCall statements for cross-domain messaging, token burns, and events - Include withdrawal hash calculation and cross-domain message encoding in fuzz tests - Fix vm.expectCall logic to properly handle both withdraw and withdrawTo functions - Address alcueca's feedback about missing _preBridgeERC20 logic in fuzz tests
| internal | ||
| { | ||
| deal(_l2Token, alice, _amount, true); | ||
| assertEq(ERC20(_l2Token).balanceOf(alice), _amount); |
There was a problem hiding this comment.
It doesn't make sense to have this deal inside a _expectERC20WithdrawalCalls function.
There was a problem hiding this comment.
devinai same as above
|
|
||
| _expectWithdrawalCallsAndEvents(_amount, _minGasLimit, _to, _isLegacy, _l2Token); | ||
|
|
||
| vm.prank(alice, alice); |
There was a problem hiding this comment.
It doesn't make sense to have this vm.prank at the end of a _expectERC20WithdrawalCalls function.
| vm.expectCall( | ||
| address(l2StandardBridge), abi.encodeCall(l2StandardBridge.withdraw, (_l2Token, 100, 1000, hex"")) | ||
| ); | ||
| if (_to == alice) { |
There was a problem hiding this comment.
This is an unclear condition to discriminate between withdraw and withdrawTo. Maybe pass an enum as an argument to do that job.
There was a problem hiding this comment.
devinai agree it's weird to have alice here hard-coded, if anything this should be both a boolean input as well as having the recipient address be an input
| // Alice has 100 L2Token | ||
| deal(_l2Token, alice, 100, true); | ||
| assertEq(ERC20(_l2Token).balanceOf(alice), 100); | ||
| function _expectERC20WithdrawalCalls( |
There was a problem hiding this comment.
I think that the tests should implement deal and vm.prank themselves and call _expectWithdrawalCallsAndEvents.
There was a problem hiding this comment.
devinai I agree here, it's weird to have an expect (assertion) function doing stateful things that manipulate the test state, leave the tests to do that
|
|
||
| vm.prank(alice, alice); | ||
| function _preBridgeERC20(bool _isLegacy, address _l2Token) internal { | ||
| _expectERC20WithdrawalCalls(100, 1000, alice, _isLegacy, _l2Token); | ||
| } | ||
|
|
||
| /// @notice Sets up expected calls and emits for a successful ERC20 withdrawal to a different | ||
| /// recipient. | ||
| /// @dev `withdrawTo` and `bridgeERC20To` should behave the same when transferring ERC20 tokens | ||
| /// so they should share the same setup and expectEmit calls | ||
| function _preBridgeERC20To(bool _isLegacy, address _l2Token) internal { |
There was a problem hiding this comment.
Tbh, at this point I would refactor the other four tests that use _preBridgeERC20 or _preBridgeERC20To, and deprecate these two functions. Otherwise we leave a half done refactor which is worse than no refactor at all.
There was a problem hiding this comment.
devinai agreed, no reason for these functions anymore
Enhance L2StandardBridge test coverage with comprehensive fuzz tests and eliminate code duplication
Summary
This PR converts three regular tests to fuzz tests (
withdraw_ether,withdraw_withdrawingERC20,withdrawTo_withdrawingERC20), adds missing test coverage for thel1TokenBridge()function, and significantly refactors helper functions to eliminate code duplication while adding comprehensive cross-domain messaging expectations to fuzz tests.Key Changes:
_preBridgeERC20and_preBridgeERC20Tofunctions into a parameterized_expectERC20WithdrawalCallssystem with 4 smaller functionsl1TokenBridge()functionReview & Testing Checklist for Human
This is a medium-risk refactoring that requires careful validation of complex helper function changes:
_expectBridgeCallscorrectly handles withdraw vs withdrawTo based on recipient addressFOUNDRY_PROFILE=ciheavy forge test --match-path test/L2/L2StandardBridge.t.sol --match-test "testFuzz_withdraw_withdrawingERC20_succeeds|testFuzz_withdrawTo_withdrawingERC20_succeeds"to catch potential edge case failuresNotes
_preBridgeERC20logic in fuzz tests and code duplication concerns