feat(solidity): add max quote index to improve data retrieval efficiency #763
Conversation
WalkthroughThe pull request introduces significant modifications 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: 3
🧹 Outside diff range and nitpick comments (5)
solidity/contracts/bridge/IBridgeFee.sol (1)
10-10: LGTM. Consider adding documentation for the new field.The addition of
quoteIndexto theQuoteInputstruct is a good enhancement, allowing for multiple quotes per oracle or token. This aligns well with the new indexing system mentioned in the PR summary.Consider adding a comment to explain the purpose and usage of the
quoteIndexfield for better code readability and maintainability.solidity/test/bridge_fee_quote.ts (2)
51-51: Consider replacing the hardcoded '3' with a named constant for clarityUsing a named constant instead of a magic number enhances readability and maintainability. Defining
MAX_QUOTE_INDEXat the beginning improves code clarity.Here is the suggested change:
+const MAX_QUOTE_INDEX = 3; -await bridgeFeeQuote.initialize(bridgeFeeOracle.getAddress(), 3); +await bridgeFeeQuote.initialize(bridgeFeeOracle.getAddress(), MAX_QUOTE_INDEX);
234-234: Use strict equality operator===instead of==For consistency and to avoid unexpected type coercion, it's recommended to use the strict equality operator
===in comparisons.Apply this diff:
- if (i == 0) { + if (i === 0) {solidity/contracts/bridge/BridgeFeeQuote.sol (2)
Line range hint
314-327: IncludequoteIndexin Signature Hash for SecurityThe
makeMessageHashfunction does not include_input.quoteIndexin the hash computation. ExcludingquoteIndexmay allow replay attacks where a signature for one quote index is used for another.Include
_input.quoteIndexin the hash to ensure the signature uniquely represents the quote.function verifySignature(QuoteInput memory _input) private pure { bytes32 hash = makeMessageHash( _input.chainName, _input.token, _input.fee, _input.gasLimit, _input.expiry + , _input.quoteIndex ); address signer = hash.toEthSignedMessageHash().recover( _input.signature ); if (_input.oracle != signer) { revert VerifySignatureFailed(_input.oracle, signer); } } function makeMessageHash( string memory _chainName, address _token, uint256 _fee, uint256 _gasLimit, uint256 _expiry + , uint256 _quoteIndex ) public pure returns (bytes32) { return keccak256(abi.encode(_chainName, _token, _fee, _gasLimit, _expiry, _quoteIndex)); }
430-435: Emit Event When UpdatingmaxQuoteIndexTo maintain transparency, emit an event when
maxQuoteIndexis updated. This allows external parties to track changes to critical contract parameters.Add an event declaration and emit it in the
updateMaxQuoteIndexfunction.+ event MaxQuoteIndexUpdated(uint256 oldMaxQuoteIndex, uint256 newMaxQuoteIndex); function updateMaxQuoteIndex( uint256 _maxQuoteIndex ) external onlyOwner returns (bool) { + emit MaxQuoteIndexUpdated(maxQuoteIndex, _maxQuoteIndex); maxQuoteIndex = _maxQuoteIndex; return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- contract/ibridge_fee_quote.sol.go (4 hunks)
- solidity/contracts/bridge/BridgeFeeQuote.sol (12 hunks)
- solidity/contracts/bridge/IBridgeFee.sol (2 hunks)
- solidity/test/bridge_fee_quote.ts (10 hunks)
🧰 Additional context used
🔇 Additional comments (10)
solidity/contracts/bridge/IBridgeFee.sol (2)
40-45: LGTM. Verify usage and consider documenting the changes.The modifications to the
getQuotefunction signature are appropriate:
- Adding
_oracleand_indexparameters allows for more precise quote retrieval.- Removing the
_amountparameter suggests a shift to pre-computed quotes.These changes align well with the PR objectives of improving data retrieval efficiency.
Please run the following script to verify that all calls to
getQuotehave been updated to match the new signature:#!/bin/bash # Description: Verify all function calls to `getQuote` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg -A 5 $'getQuote\s*\('Consider adding inline comments or updating the function documentation to explain the purpose of the new parameters and the removal of the
_amountparameter. This will help maintain clear understanding of the function's behavior for future developers.
36-38: LGTM. Verify usage across the codebase.The changes to
getQuoteByTokenfunction signature are appropriate:
- Adding
_chainNameparameter allows for more specific quote retrieval.- Returning an array of
QuoteInfoobjects supports multiple quotes per token.These modifications align well with the PR objectives.
Please run the following script to verify that all calls to
getQuoteByTokenhave been updated to match the new signature:solidity/test/bridge_fee_quote.ts (1)
82-83: LGTMThe
newBridgeFeeQuotefunction is correctly called with the updated parameters, including theoracleandquoteIndex.solidity/contracts/bridge/BridgeFeeQuote.sol (1)
147-165: Ensure Consistent Loop Bounds formaxQuoteIndexThe loops iterating over
kusek < maxQuoteIndex, which is correct if indexes range from0tomaxQuoteIndex - 1. However, to align with the updated validation inverifyInput, ensure that all loops consistently use the correctedmaxQuoteIndexvalue.No changes needed if the validation condition is updated as suggested.
Also applies to: 241-250, 375-384
contract/ibridge_fee_quote.sol.go (6)
45-52: LGTM!The
QuoteIndexfield is appropriately added to theIBridgeFeeQuoteQuoteInputstruct. The types are consistent with the existing fields.
206-221: LGTM!The new
GetQuotemethod is implemented correctly and matches the contract's ABI. The parameters and return types are properly defined.
223-236: LGTM!The session methods for
GetQuoteare correctly implemented in bothIBridgeFeeQuoteSessionandIBridgeFeeQuoteCallerSession. They correctly delegate to the mainGetQuotemethod.
285-296: LGTM!The session methods for
GetQuoteByTokenare correctly updated to reflect the new return type. They are consistent with the updated method in the caller.
330-346: LGTM!The
Quotemethod has been correctly updated to align with the new ABI definitions. The parameters and transaction bindings are properly defined.
268-281:⚠️ Potential issueUpdate downstream code to handle the new return type of
GetQuoteByTokenThe
GetQuoteByTokenmethod now returns a slice ofIBridgeFeeQuoteQuoteInfoinstead of a single quote and a boolean. Please ensure that any downstream code that calls this method is updated to handle the new return type appropriately.Run the following script to identify all usages of
GetQuoteByToken:
…rror (#766) Co-authored-by: todd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
solidity/contracts/bridge/BridgeFeeQuote.sol (2)
Line range hint
286-295: IncludequoteIndexin the signature to ensure uniqueness across indicesThe
verifySignaturefunction andmakeMessageHashdo not include_input.quoteIndexin the message hash used for signature verification. This omission can lead to security vulnerabilities where a valid signature for onequoteIndexcould be reused for anotherquoteIndex, potentially allowing unauthorized quotes to be accepted. IncludingquoteIndexin the message hash ensures that each signature is unique to a specific quote index.Apply this diff to include
quoteIndexin the signature:// In verifySignature function function verifySignature(QuoteInput memory _input) private pure { bytes32 hash = makeMessageHash( _input.chainName, _input.token, _input.fee, _input.gasLimit, _input.expiry, + _input.quoteIndex ); address signer = hash.toEthSignedMessageHash().recover( _input.signature ); if (_input.oracle != signer) { revert VerifySignatureFailed(_input.oracle, signer); } } // Update makeMessageHash function to include quoteIndex -function makeMessageHash( - string memory _chainName, - address _token, - uint256 _fee, - uint256 _gasLimit, - uint256 _expiry -) public pure returns (bytes32) { +function makeMessageHash( + string memory _chainName, + address _token, + uint256 _fee, + uint256 _gasLimit, + uint256 _expiry, + uint256 _quoteIndex +) public pure returns (bytes32) { return - keccak256(abi.encode(_chainName, _token, _fee, _gasLimit, _expiry)); + keccak256(abi.encode(_chainName, _token, _fee, _gasLimit, _expiry, _quoteIndex)); }Also applies to: 317-326
430-435: Emit an event when updatingmaxQuoteIndexfor transparencyThe
updateMaxQuoteIndexfunction alters a crucial parameter that affects the contract's behavior. Emitting an event whenmaxQuoteIndexis updated enhances transparency and allows off-chain systems to track changes to this value.Apply this diff to emit an event:
+ event MaxQuoteIndexUpdated(uint256 oldMaxQuoteIndex, uint256 newMaxQuoteIndex); function updateMaxQuoteIndex( uint256 _maxQuoteIndex ) external onlyOwner returns (bool) { + emit MaxQuoteIndexUpdated(maxQuoteIndex, _maxQuoteIndex); maxQuoteIndex = _maxQuoteIndex; return true; }
Summary by CodeRabbit
New Features
getQuotemethod for retrieving quotes based on chain name, token, oracle, and index.getQuoteByTokenmethod to return an array of quotes.Bug Fixes
Tests