feat: synapse migrator #339
Conversation
WalkthroughA new token migration system is introduced, including the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SynapseMigrator
participant OldToken (IBurnable)
participant NewToken (ERC20)
User->>SynapseMigrator: migrate(oldToken, amount)
SynapseMigrator->>OldToken: burnFrom(User, amount)
SynapseMigrator->>NewToken: transfer(User, scaledAmount)
SynapseMigrator-->>User: emit Migrated event
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 (
|
Update ISynapseMigrator interface and implementation to return both newToken and newAmount from previewMigrate, eliminating the need for internal _previewMigrate function and simplifying the contract logic. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Pull Request Test Coverage Report for Build 15740219775Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
contracts/migrator/interfaces/IBurnable.sol (1)
4-6: Interface looks clean but consider ERC20 compatibility.The
IBurnableinterface is minimal and focused, which is good. However, consider if this should extend or be compatible with OpenZeppelin'sERC20Burnableinterface for broader compatibility.Consider adding documentation comments to clarify the interface's purpose:
+/// @title IBurnable +/// @notice Interface for tokens that support burning from another address interface IBurnable { + /// @notice Burns a specific amount of tokens from the given address + /// @param from The address to burn tokens from + /// @param amount The amount of tokens to burn function burnFrom(address from, uint256 amount) external; }contracts/migrator/interfaces/ISynapseMigratorErrors.sol (1)
4-10: Well-structured error definitions with consistent naming.The custom errors are well-defined with a clear naming convention using the
SM__prefix. The error names are descriptive and cover the expected failure scenarios for a token migrator.Consider adding documentation comments to explain when each error is thrown:
+/// @title ISynapseMigratorErrors +/// @notice Custom errors used by the SynapseMigrator contract interface ISynapseMigratorErrors { + /// @notice Thrown when the same address is provided for both old and new tokens error SM__SameAddress(); + /// @notice Thrown when attempting to add a token pair that already exists error SM__TokenPairAlreadyAdded(); + /// @notice Thrown when attempting to migrate with a token pair that doesn't exist error SM__TokenPairNotAdded(); + /// @notice Thrown when a zero address is provided as input error SM__ZeroAddress(); + /// @notice Thrown when a zero amount is provided for migration error SM__ZeroAmount(); }test/mocks/MockBurnableToken.sol (1)
22-24: Consider adding access control to mintTestTokens.The
mintTestTokensfunction is public and unrestricted, which is appropriate for testing but could be made more explicit about its testing-only nature.Consider adding a comment or making the testing nature more explicit:
+ /// @notice Mint tokens for testing purposes only + /// @dev This function should only be used in tests function mintTestTokens(address to, uint256 amount) external { _mint(to, amount); }test/migrator/SynapseMigrator.SameDecimals.t.sol (1)
6-8: Appropriate test for same decimal precision scenario.The test contract correctly configures both old and new tokens to have 18 decimals, testing the migration scenario where no decimal scaling is required. This is an important edge case to verify.
Consider adding a comment to clarify the test scenario:
+/// @notice Test contract for migrating tokens with the same decimal precision (18 -> 18) contract SynapseMigratorSameDecimalsTest is SynapseMigratorTest { constructor() SynapseMigratorTest(18, 18) {} }test/migrator/SynapseMigrator.t.sol (2)
11-11: Consider making the abstract contract more explicit.The abstract contract is not explicitly marked as
abstract, which could be confusing. Consider adding theabstractkeyword for clarity.-abstract contract SynapseMigratorTest is Test, ISynapseMigratorErrors { +abstract contract SynapseMigratorTest is Test, ISynapseMigratorErrors {
27-30: Constructor parameters should be validated.The constructor accepts decimal parameters but doesn't validate them. Consider adding basic validation to prevent invalid test configurations.
constructor(uint8 oldTokenDecimals_, uint8 newTokenDecimals_) { + require(oldTokenDecimals_ <= 18, "Invalid old token decimals"); + require(newTokenDecimals_ <= 18, "Invalid new token decimals"); oldTokenDecimals = oldTokenDecimals_; newTokenDecimals = newTokenDecimals_; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/migrator/SynapseMigrator.sol(1 hunks)contracts/migrator/interfaces/IBurnable.sol(1 hunks)contracts/migrator/interfaces/ISynapseMigrator.sol(1 hunks)contracts/migrator/interfaces/ISynapseMigratorErrors.sol(1 hunks)test/migrator/SynapseMigrator.LessDecimals.t.sol(1 hunks)test/migrator/SynapseMigrator.Management.t.sol(1 hunks)test/migrator/SynapseMigrator.MoreDecimals.t.sol(1 hunks)test/migrator/SynapseMigrator.SameDecimals.t.sol(1 hunks)test/migrator/SynapseMigrator.t.sol(1 hunks)test/mocks/MockBurnableToken.sol(1 hunks)
🔇 Additional comments (15)
test/mocks/MockBurnableToken.sol (2)
11-13: Constructor implementation is correct.The constructor properly initializes the ERC20 token with the provided name and stores the custom decimals value.
4-4: Verify OpenZeppelin version compatibility.The contract imports from a specific OpenZeppelin version
@openzeppelin/contracts-4.5.0. Ensure this version is compatible with the Solidity version^0.8.0used in this contract.#!/bin/bash # Description: Check if OpenZeppelin 4.5.0 is compatible with Solidity ^0.8.0 # and if there are any known security issues with this version # Check for any references to OpenZeppelin version requirements in the codebase rg -A 3 -B 3 "openzeppelin.*4\.5\.0" # Look for any package.json or similar files that might specify OpenZeppelin versions fd -e json -e toml -e yaml | xargs grep -l "openzeppelin" 2>/dev/null || truetest/migrator/SynapseMigrator.MoreDecimals.t.sol (1)
6-8: ```bash
#!/bin/bashPreview base test contract and search for decimal logic
FILE="test/migrator/SynapseMigrator.t.sol"
echo "=== First 200 lines of $FILE ==="
sed -n '1,200p' "$FILE" || echo "Failed to read $FILE"echo -e "\n=== Search for decimal parameters ==="
rg -n "decimals" -g "$FILE" || echo "No 'decimals' occurrences found"
rg -n "decimal" -g "$FILE" || echo "No 'decimal' occurrences found"echo -e "\n=== Search for scaling operations ==="
rg -n "mul" -g "$FILE" || echo "No 'mul' occurrences found"
rg -n "div" -g "$FILE" || echo "No 'div' occurrences found"</details> <details> <summary>contracts/migrator/interfaces/ISynapseMigrator.sol (1)</summary> `1-32`: **Well-designed interface with comprehensive documentation.** The interface is well-structured with clear function signatures and comprehensive documentation. The revert conditions are properly documented, making it easy for implementers to understand the expected behavior. </details> <details> <summary>test/migrator/SynapseMigrator.t.sol (2)</summary> `52-65`: **Comprehensive migration test with good balance verification.** The test properly verifies all aspects of the migration: - Event emission - Balance changes for both tokens - Total supply changes (old token burned, new token transferred) This ensures the migration logic works correctly. --- `87-93`: **Good test for allowance checking.** The test properly verifies that insufficient allowance is handled correctly. The use of `vm.expectRevert()` without a specific error selector is acceptable here since the revert comes from the ERC20 token itself. </details> <details> <summary>test/migrator/SynapseMigrator.Management.t.sol (3)</summary> `24-38`: **Good use of mock calls for external dependencies.** The setup properly mocks the `decimals()` calls for the token contracts, which isolates the tests from external dependencies and makes them more reliable. --- `46-49`: **Test should use custom error instead of string message.** The test expects a string revert message from OpenZeppelin's Ownable, but it would be more consistent to expect a custom error. However, since this is testing OpenZeppelin's behavior, the current approach is acceptable. --- `91-96`: **Excellent use of fuzzing for access control testing.** The fuzz test with `vm.assume(caller != owner)` is a great way to ensure that any non-owner address cannot call the restricted function. </details> <details> <summary>contracts/migrator/SynapseMigrator.sol (3)</summary> `26-28`: **Constructor properly transfers ownership.** The constructor correctly transfers ownership to the specified address, which is the proper pattern for OpenZeppelin's Ownable. --- `31-43`: **Comprehensive input validation and proper state management.** The `addTokenPair` function includes all necessary validations: - Zero address checks - Same address prevention - Duplicate pair prevention - Proper event emission The storage of token decimals in the struct is a good optimization to avoid repeated external calls. --- `52-52`: ```shell #!/bin/bash # Display the IBurnable interface definition for verification sed -n '1,200p' contracts/migrator/interfaces/IBurnable.soltest/migrator/SynapseMigrator.LessDecimals.t.sol (3)
10-14: Good test for precision loss handling.The test uses a realistic decimal amount (1.23456789 tokens) and verifies that precision loss results in the expected truncated amount (1234567 units with 6 decimals = 1.234567 tokens).
16-20: Excellent edge case test for zero amount output.This test verifies that when the conversion would result in zero output due to precision loss, the contract properly reverts with
SM__ZeroAmount. The amount10**12 - 1is just below the threshold that would produce 1 unit in the new token.
28-32: Preview function correctly handles precision loss edge case.The test verifies that the preview function returns zero for amounts that would result in zero output, which is consistent with the migration function's behavior.
| function previewMigrate(address oldToken, uint256 amount) | ||
| public | ||
| view | ||
| returns (address newToken, uint256 newAmount) | ||
| { | ||
| newToken = _tokenPairs[oldToken].newToken; | ||
| uint256 oldDecimals = _tokenPairs[oldToken].oldTokenDecimals; | ||
| uint256 newDecimals = _tokenPairs[oldToken].newTokenDecimals; | ||
| if (newToken == address(0)) return (address(0), 0); | ||
| newAmount = (amount * 10**newDecimals) / 10**oldDecimals; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decimal conversion logic needs overflow protection.
The decimal conversion calculation (amount * 10**newDecimals) / 10**oldDecimals could potentially overflow for large amounts, especially with high decimal differences.
Consider using a safer calculation approach or adding overflow checks:
function previewMigrate(address oldToken, uint256 amount)
public
view
returns (address newToken, uint256 newAmount)
{
newToken = _tokenPairs[oldToken].newToken;
uint256 oldDecimals = _tokenPairs[oldToken].oldTokenDecimals;
uint256 newDecimals = _tokenPairs[oldToken].newTokenDecimals;
if (newToken == address(0)) return (address(0), 0);
- newAmount = (amount * 10**newDecimals) / 10**oldDecimals;
+ if (newDecimals >= oldDecimals) {
+ newAmount = amount * (10**(newDecimals - oldDecimals));
+ } else {
+ newAmount = amount / (10**(oldDecimals - newDecimals));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function previewMigrate(address oldToken, uint256 amount) | |
| public | |
| view | |
| returns (address newToken, uint256 newAmount) | |
| { | |
| newToken = _tokenPairs[oldToken].newToken; | |
| uint256 oldDecimals = _tokenPairs[oldToken].oldTokenDecimals; | |
| uint256 newDecimals = _tokenPairs[oldToken].newTokenDecimals; | |
| if (newToken == address(0)) return (address(0), 0); | |
| newAmount = (amount * 10**newDecimals) / 10**oldDecimals; | |
| } | |
| function previewMigrate(address oldToken, uint256 amount) | |
| public | |
| view | |
| returns (address newToken, uint256 newAmount) | |
| { | |
| newToken = _tokenPairs[oldToken].newToken; | |
| uint256 oldDecimals = _tokenPairs[oldToken].oldTokenDecimals; | |
| uint256 newDecimals = _tokenPairs[oldToken].newTokenDecimals; | |
| if (newToken == address(0)) return (address(0), 0); | |
| if (newDecimals >= oldDecimals) { | |
| newAmount = amount * (10**(newDecimals - oldDecimals)); | |
| } else { | |
| newAmount = amount / (10**(oldDecimals - newDecimals)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/migrator/SynapseMigrator.sol around lines 64 to 74, the decimal
conversion calculation multiplies amount by 10 to the power of newDecimals,
which can overflow for large values. To fix this, use a safe math approach such
as performing the multiplication and division in a way that avoids intermediate
overflow, for example by dividing first if possible or using a library like
SafeMath or unchecked blocks with explicit overflow checks. Ensure the
calculation safely handles large amounts and decimal differences without risking
overflow.
| function migrate(address oldToken, uint256 amount) external { | ||
| if (oldToken == address(0)) revert SM__ZeroAddress(); | ||
| (address newToken, uint256 newAmount) = previewMigrate(oldToken, amount); | ||
| if (newToken == address(0)) revert SM__TokenPairNotAdded(); | ||
| if (newAmount == 0) revert SM__ZeroAmount(); | ||
| // Burn old tokens from the user | ||
| IBurnable(oldToken).burnFrom(msg.sender, amount); | ||
| // Send new tokens to the user | ||
| IERC20(newToken).safeTransfer(msg.sender, newAmount); | ||
| emit Migrated(msg.sender, oldToken, amount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Migration function has potential reentrancy risk.
The function calls external contracts (burn and transfer) which could potentially lead to reentrancy attacks. Consider adding a reentrancy guard.
+import {ReentrancyGuard} from "@openzeppelin/contracts-4.5.0/security/ReentrancyGuard.sol";
-contract SynapseMigrator is Ownable, ISynapseMigrator, ISynapseMigratorErrors {
+contract SynapseMigrator is Ownable, ReentrancyGuard, ISynapseMigrator, ISynapseMigratorErrors {
- function migrate(address oldToken, uint256 amount) external {
+ function migrate(address oldToken, uint256 amount) external nonReentrant {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function migrate(address oldToken, uint256 amount) external { | |
| if (oldToken == address(0)) revert SM__ZeroAddress(); | |
| (address newToken, uint256 newAmount) = previewMigrate(oldToken, amount); | |
| if (newToken == address(0)) revert SM__TokenPairNotAdded(); | |
| if (newAmount == 0) revert SM__ZeroAmount(); | |
| // Burn old tokens from the user | |
| IBurnable(oldToken).burnFrom(msg.sender, amount); | |
| // Send new tokens to the user | |
| IERC20(newToken).safeTransfer(msg.sender, newAmount); | |
| emit Migrated(msg.sender, oldToken, amount); | |
| } | |
| function migrate(address oldToken, uint256 amount) external nonReentrant { | |
| if (oldToken == address(0)) revert SM__ZeroAddress(); | |
| (address newToken, uint256 newAmount) = previewMigrate(oldToken, amount); | |
| if (newToken == address(0)) revert SM__TokenPairNotAdded(); | |
| if (newAmount == 0) revert SM__ZeroAmount(); | |
| // Burn old tokens from the user | |
| IBurnable(oldToken).burnFrom(msg.sender, amount); | |
| // Send new tokens to the user | |
| IERC20(newToken).safeTransfer(msg.sender, newAmount); | |
| emit Migrated(msg.sender, oldToken, amount); | |
| } |
🤖 Prompt for AI Agents
In contracts/migrator/SynapseMigrator.sol around lines 46 to 56, the migrate
function calls external contracts for burning and transferring tokens without
protection against reentrancy attacks. To fix this, add a reentrancy guard
modifier to the migrate function and apply the checks-effects-interactions
pattern by performing all state changes before external calls. This will prevent
potential reentrancy exploits during token operations.
Summary by CodeRabbit
New Features
Bug Fixes
Tests