SYN-100: legacy workflows for mint and withdraw#335
SYN-100: legacy workflows for mint and withdraw#335ChiTimesChi merged 6 commits intofeat/syn-bridge-unifiedfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SynapseBridge
participant Token
participant PoolMock
User->>SynapseBridge: mintAndSwap(...)
alt isLegacySendDisabled == true
SynapseBridge->>SynapseBridge: _mint(...)
SynapseBridge->>Token: mint tokens
else
SynapseBridge->>SynapseBridge: _mint(...)
SynapseBridge->>Token: mint tokens
SynapseBridge->>PoolMock: swap tokens
end
SynapseBridge-->>User: emit event / transfer tokens
sequenceDiagram
participant User
participant SynapseBridge
participant Token
participant PoolMock
User->>SynapseBridge: withdrawAndRemove(...)
alt isLegacySendDisabled == true
SynapseBridge->>SynapseBridge: _withdraw(...)
SynapseBridge->>Token: transfer tokens
else
SynapseBridge->>SynapseBridge: _withdraw(...)
SynapseBridge->>Token: transfer tokens
SynapseBridge->>PoolMock: remove liquidity
end
SynapseBridge-->>User: emit event / transfer tokens
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Pull Request Test Coverage Report for Build 15447013869Details
💛 - Coveralls |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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 (5)
test/bridge/legacy/ReenteringToken.sol (1)
10-11: Consider using private visibility for test isolation.The
targetanddatastate variables are currentlyinternal. For better test isolation and to prevent unintended inheritance issues, consider changing them toprivate.- address internal target; - bytes internal data; + address private target; + bytes private data;test/bridge/legacy/PoolMock.sol (1)
9-16: Document the default return values for mock functions.The
calculateSwapandcalculateRemoveLiquidityOneTokenfunctions have empty implementations and will return 0 by default. Consider adding comments to make this behavior explicit for test clarity.function calculateSwap( uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 amount - ) external returns (uint256) {} + ) external returns (uint256) { + // Mock returns 0 by default + } function calculateRemoveLiquidityOneToken(uint256 tokenAmount, uint8 tokenIndex) external returns (uint256) {} + // Mock returns 0 by default + }contracts/bridge/SynapseBridge.sol (2)
393-396: Consider event emission consistency in legacy fallback.When
isLegacySendDisabledis true, the function falls back to_mintwhich emitsTokenMintevent instead ofTokenMintAndSwap. This might cause confusion for off-chain monitoring systems expectingTokenMintAndSwapevents.Consider emitting a
TokenMintAndSwapevent withswapSuccess=falseto maintain event consistency:// Fallback to regular mint if legacy workflows are disabled. if (isLegacySendDisabled) { - return _mint(to, token, amount, fee, kappa); + _mint(to, token, amount, fee, kappa); + emit TokenMintAndSwap( + to, + token, + amount.sub(fee), + fee, + tokenIndexFrom, + tokenIndexTo, + minDy, + deadline, + false, + kappa + ); + return; }
501-504: Maintain event consistency for withdrawAndRemove fallback.Similar to the
mintAndSwapfunction, consider emitting aTokenWithdrawAndRemoveevent withswapSuccess=falsewhen falling back to regular withdraw.// Fallback to regular withdraw if legacy workflows are disabled. if (isLegacySendDisabled) { - return _withdraw(to, token, amount, fee, kappa); + _withdraw(to, token, amount, fee, kappa); + emit TokenWithdrawAndRemove( + to, + token, + amount.sub(fee), + fee, + swapTokenIndex, + swapMinAmount, + swapDeadline, + false, + kappa + ); + return; }test/bridge/legacy/SynapseBridge.Dst.t.sol (1)
129-338: Consider adding edge case tests for completeness.While the test coverage is comprehensive, consider adding tests for:
- Zero amount scenarios for mint/withdraw operations
- Maximum uint256 values to test for overflow conditions
- Duplicate kappa usage (should revert)
- Multiple sequential operations to test state consistency
Would you like me to generate these additional edge case tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/bridge/SynapseBridge.sol(4 hunks)test/bridge/legacy/PoolMock.sol(1 hunks)test/bridge/legacy/ReenteringToken.sol(1 hunks)test/bridge/legacy/SynapseBridge.Dst.t.sol(1 hunks)
🔇 Additional comments (6)
test/bridge/legacy/ReenteringToken.sol (1)
32-40: Good reentrancy implementation pattern!The implementation correctly clears the state variables before making the external call, preventing repeated reentrancy. This is a solid pattern for testing reentrancy protection.
contracts/bridge/SynapseBridge.sol (2)
224-225: Excellent refactoring to reduce code duplication!The extraction of common withdrawal logic into the
_withdrawinternal function improves maintainability and ensures consistency across different withdrawal paths.Also applies to: 231-252
270-271: Well-structured refactoring for mint operations!The
_mintinternal function properly consolidates the minting logic, maintaining all security checks and event emissions.Also applies to: 277-292
test/bridge/legacy/SynapseBridge.Dst.t.sol (3)
2-3: Consider the implications of using Solidity 0.6.12.While this might be intentional for testing legacy functionality, Solidity 0.6.12 is quite outdated and lacks many security improvements and optimizations from newer versions. Additionally,
ABIEncoderV2is marked as experimental in this version.Could you confirm that using this specific version is required for compatibility with the legacy SynapseBridge contract?
13-127: Well-structured test setup with comprehensive helper functions.The test contract is well-organized with:
- Clear separation of state variables, events, modifiers, and helper functions
- Comprehensive event expectation helpers that match the expected bridge behavior
- Good use of modifiers to set up different test scenarios
129-338: Comprehensive test coverage with proper edge case handling.The test implementations excellently cover:
- Normal operations (mint, mintAndSwap, withdraw, withdrawAndRemove)
- Legacy mode disabled scenarios with proper fallback verification
- Reentrancy protection validation with expected revert messages
- Access control verification using fuzzing with
vm.assume- Reverting pool scenarios
The tests properly validate events, balances, and state changes (kappa existence).
2 participants
Summary by CodeRabbit
Refactor
Tests