refactor(solidity): rename token to tokenName #784
Conversation
WalkthroughThe pull request includes significant modifications across several Solidity files related to the Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (3)
solidity/contracts/bridge/IBridgeFee.sol (1)
37-37: Consider renaming functions for consistency.While the parameter changes from
_tokento_tokenNameare good, the function namegetQuoteByTokenstill uses the old terminology. For complete consistency, consider renaming it togetQuoteByTokenName.Here's the suggested change:
- function getQuoteByToken( + function getQuoteByTokenName( string memory _chainName, string memory _tokenName ) external view returns (QuoteInfo[] memory quotes);Also applies to: 42-42
solidity/test/bridge_fee_quote.ts (1)
176-180: Consider using more realistic token names in tests.While the current implementation works, using more realistic token names (e.g., "USDT", "DAI", "WETH") would make the tests more representative of real-world scenarios.
- tokens.push("test" + i.toString()); + tokens.push(["USDT", "DAI", "WETH", "USDC", "WBTC"][i]);contract/ibridge_fee_quote.sol.go (1)
Line range hint
1-2: Reminder: This is a generated file.As indicated by the header comment, this is an auto-generated binding file. Any changes should be made to the source Solidity contract instead of this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- contract/ibridge_fee_quote.sol.go (8 hunks)
- solidity/contracts/bridge/BridgeFeeQuote.sol (17 hunks)
- solidity/contracts/bridge/IBridgeFee.sol (3 hunks)
- solidity/test/bridge_fee_quote.ts (16 hunks)
🔇 Additional comments (11)
solidity/contracts/bridge/IBridgeFee.sol (2)
20-20: LGTM! Consistent with QuoteInput changes.The change maintains consistency with the
QuoteInputstruct, ensuring a uniform approach to token representation across the interface.
8-8: Verify the migration plan for the breaking change.The change from
address tokentostring tokenNameinQuoteInputis a good improvement for cross-chain compatibility and readability. However, this is a breaking change that requires careful migration.Let's verify the impact on existing implementations:
✅ Verification successful
Breaking change is properly handled across the codebase
The verification shows that the migration from
tokentotokenNameis consistently implemented:
- The
BridgeFeeQuote.solimplementation properly handles theQuoteInputstruct with the newtokenNamefield in its functions (quote,verifyInput,verifySignature)- The test file
bridge_fee_quote.tshas been updated to use the newtokenNamefield in all test cases and helper functions- No other conflicting struct definitions with
tokenfield were found that would need similar changes🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all contract implementations and tests that might be affected # by the QuoteInput struct change # Search for QuoteInput usage in contracts and tests rg -l "QuoteInput" --type solidity --type typescriptLength of output: 85
Script:
#!/bin/bash # Let's try a more generic search without file type restrictions # to find all files containing QuoteInput rg -l "QuoteInput" # Also search for potential implementations of IBridgeFee rg -l "IBridgeFee"Length of output: 411
Script:
#!/bin/bash # Let's examine the implementation and test files to see how they handle the change # Check BridgeFeeQuote implementation rg "QuoteInput" solidity/contracts/bridge/BridgeFeeQuote.sol -B 2 -A 2 # Check the test file rg "QuoteInput" solidity/test/bridge_fee_quote.ts -B 2 -A 2 # Check if there are any other struct definitions that might need similar changes rg "struct.*token" solidity/contracts/ -A 3Length of output: 1570
solidity/test/bridge_fee_quote.ts (3)
21-22: LGTM: Variable declarations and setup changes are consistent.The changes correctly implement the transition from address-like types to string representations for tokens, with appropriate test values and consistent method naming.
Also applies to: 29-30, 59-59
Line range hint
1-329: Overall changes look good and maintain test coverage.The modifications consistently implement the transition from address-based tokens to string-based token names across all test cases and helper functions, maintaining comprehensive test coverage for the contract's functionality.
277-278: Verify signature compatibility with contract.The signature generation has been updated to use string token names. Please ensure this matches the contract's signature verification logic.
✅ Verification successful
Let me analyze the contract's signature verification logic by searching for the relevant code.
Let me try a different search pattern to find the signature verification logic.
Based on the code found, I can now verify the signature compatibility. The contract's signature verification logic matches the test implementation:
- The test generates a signature using
generateSignature()with parameters:[chainName, tokenName, fee, gasLimit, expiry]- The contract verifies using
verifySignature()which:
- Creates the same message hash using
makeMessageHash()with identical parameters in the same order- Uses standard Ethereum signature recovery with
toEthSignedMessageHash().recover()- Verifies the recovered signer matches the oracle address
The signature generation and verification are compatible since:
- Both use the same parameters in the same order
- Both use standard Ethereum personal message signing/recovery
- The contract's
makeMessageHash()usesabi.encode()which matches the test's encodingNo changes needed - signature generation and verification are properly aligned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify signature verification logic in contract matches test implementation # Test: Search for signature verification in contract ast-grep --pattern 'function verify$$$($$$) { $$$ keccak256($$$) $$$ }' # Test: Search for any other signature-related functions rg -A 5 'signature|verify|hash'Length of output: 494517
Script:
#!/bin/bash # Search for signature verification in BridgeFeeQuote contract ast-grep --pattern 'function verifySignature($$$) { $$$ }' # Search for makeMessageHash function that creates the hash for signature verification ast-grep --pattern 'function makeMessageHash($$$) { $$$ }'Length of output: 125
Script:
#!/bin/bash # Search for signature verification in BridgeFeeQuote contract rg -A 10 "verifySignature" solidity/contracts/bridge/ # Search for makeMessageHash function rg -A 10 "makeMessageHash" solidity/contracts/bridge/Length of output: 3484
solidity/contracts/bridge/BridgeFeeQuote.sol (3)
11-11: LGTM: Import and error definition changes are consistent with the refactor.The addition of StringsUpgradeable and the renaming of errors from Token* to TokenName* maintain consistency with the token to tokenName refactoring.
Also applies to: 15-15, 22-22
325-327: Verify hash collision resistance.The
makeMessageHashfunction usesabi.encodewhich is safe against collisions. However, since we're now using strings instead of addresses, we should document the expected format of_tokenNameto prevent any potential issues with different string representations of the same token.Add a comment documenting the expected format:
function makeMessageHash( string memory _chainName, string memory _tokenName, uint256 _fee, uint256 _gasLimit, uint256 _expiry ) public pure returns (bytes32) { + // _tokenName should be uppercase, alphanumeric only (e.g., "USDC", "ETH") return keccak256(abi.encode(_chainName, _tokenName, _fee, _gasLimit, _expiry)); }
36-37: Verify the implications of switching from address[] to string[] for token identification.The change from
address[] tokenstostring[] tokenNamesrepresents a significant architectural shift. While this makes the contract more flexible by decoupling it from specific chain addresses, it requires careful consideration:
- How will these token names be mapped to actual token addresses on different chains?
- Is there a standardized format for these token names to ensure consistency?
- What prevents name collisions across different chains?
Let's verify the token name usage across the codebase:
✅ Verification successful
Based on the search results, I can now provide a comprehensive response about the architectural change:
The change from
address[]tostring[]for token identification is well-structured and safe.The codebase shows a robust implementation for handling token names:
Token names are managed through a structured system:
- Each chain has an
Assetstruct containingtokenNames- Token names must be registered via
registerTokenNamewith proper validation- Duplicate names are prevented via
TokenNameAlreadyExistserror- Empty names are rejected via
TokenNameInvaliderrorThe token name to quote mapping is handled securely:
- Quotes are uniquely identified by combining
chainName + tokenName + oracle + index- Token names are validated before use via
isActiveTokenName- The system maintains consistency through proper packing/unpacking of asset data
The architecture supports cross-chain flexibility:
- Token names are consistently used across quote generation and verification
- The system properly handles token name validation across different chains
- The design allows for chain-specific token mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token name standardization or mapping patterns rg -g '*.sol' -A 5 'tokenName.*mapping'Length of output: 41
Script:
#!/bin/bash # Let's search for token name usage patterns more broadly rg -g '*.sol' -B 2 -A 5 'tokenNames?' # Also search for any token to address mapping patterns rg -g '*.sol' -B 2 -A 5 'mapping.*token' # Look for functions that might handle token name to address conversion ast-grep --pattern 'function $_($$$) { $$$ tokenName $$$ }'Length of output: 24974
contract/ibridge_fee_quote.sol.go (3)
57-57: LGTM! ABI changes are consistent.The ABI has been correctly updated to reflect the parameter type changes from
address _tokentostring _tokenNameacross all method signatures.
Line range hint
206-234: LGTM! Method signatures and documentation are properly updated.The changes to method signatures and their documentation are consistent across all binding types (Caller, Session, CallerSession) and align with the new parameter types. The following methods have been correctly updated:
GetQuoteGetQuoteByTokenQuoteAlso applies to: 268-296, 330-348
36-36: Verify the impact of struct field changes.The change from
Token common.AddresstoTokenName stringis a breaking change that affects bothIBridgeFeeQuoteQuoteInfoandIBridgeFeeQuoteQuoteInputstructs. This will require updates in any code that constructs or consumes these structs.Also applies to: 46-46
Summary by CodeRabbit
New Features
tokenNameinstead oftoken, enhancing clarity in token representation.Bug Fixes
Documentation