feat: get bridge fee quote in BuildOutgoingTxBatch #788
Conversation
WalkthroughThe changes in this pull request enhance the cross-chain functionality by introducing a new Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)x/crosschain/mock/expected_keepers_mocks.go (2)
The contract package import is properly added to support the new BridgeFeeQuoteKeeper mock implementation.
The mock implementation follows gomock best practices and maintains consistency with other keeper mocks in the file. It properly handles:
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: 7
🧹 Outside diff range and nitpick comments (8)
contract/bridge_fee_quote.go (1)
14-19: Add documentation for the struct and its fields
Consider adding documentation to explain:
- The purpose of the keeper
- The role of each field
- The relationship with the smart contract
Example documentation:
+// BridgeFeeQuoteKeeper manages interactions with the bridge fee quote smart contract.
+// It handles querying fee quotes for cross-chain token transfers.
type BrideFeeQuoteKeeper struct {
+ // Caller provides methods for interacting with the blockchain
Caller
+ // abi contains the smart contract's ABI
abi abi.ABI
+ // from is the module's address used for contract calls
from common.Address
+ // contract is the bridge fee quote contract address
contract common.Address
}solidity/contracts/bridge/IBridgeFee.sol (1)
40-43: LGTM! Consider adding NatSpec documentation.
The method signature is well-designed and the plural form better reflects its return type. The view modifier is appropriate for this read-only operation.
Consider adding NatSpec documentation to improve interface clarity:
+ /// @notice Retrieves all quotes for a specific token on a given chain
+ /// @param _chainName The name of the chain
+ /// @param _tokenName The name of the token
+ /// @return quotes Array of QuoteInfo structs containing the quote details
function getQuotesByToken(
string memory _chainName,
string memory _tokenName
) external view returns (QuoteInfo[] memory quotes);x/crosschain/types/expected_keepers.go (1)
90-92: Add documentation comments for the interface and method
Consider adding GoDoc comments to describe:
- The purpose and responsibility of the BridgeFeeQuoteKeeper interface
- The parameters and return values of GetQuotesByToken method
+// BridgeFeeQuoteKeeper defines the expected interface for retrieving bridge fee quotes
+// from smart contracts across different chains.
type BridgeFeeQuoteKeeper interface {
+ // GetQuotesByToken retrieves bridge fee quotes for a specific token on a given chain
+ // Parameters:
+ // - ctx: The context for the query
+ // - chainName: The name of the chain to query quotes from
+ // - denom: The denomination of the token to get quotes for
+ // Returns: A slice of quote information and any error encountered
GetQuotesByToken(ctx context.Context, chainName, denom string) ([]contract.IBridgeFeeQuoteQuoteInfo, error)
}x/crosschain/keeper/outgoing_tx.go (1)
19-33: Consider enhancing error handling and logging for bridge fee quotes.
While the core logic is sound, consider the following improvements:
- Add debug logging when quotes fail validation to help with troubleshooting
- Consider handling the case where quoteInfos is empty
- Consider adding metrics for quote validation failures
Here's a suggested improvement:
quoteInfos, err := k.brideFeeQuoteKeeper.GetQuotesByToken(ctx, k.moduleName, fee.Denom)
if err != nil {
return 0, err
}
+if len(quoteInfos) == 0 {
+ return 0, types.ErrInvalid.Wrapf("no bridge fee quotes available for token %s", fee.Denom)
+}
var quoteInfo *contract.IBridgeFeeQuoteQuoteInfo
for _, quote := range quoteInfos {
if fee.Amount.GTE(sdkmath.NewIntFromBigInt(quote.Fee)) &&
new(big.Int).Sub(quote.Expiry, big.NewInt(ctx.BlockTime().UnixNano())).Sign() > 0 {
+ ctx.Logger().Debug("found valid bridge fee quote",
+ "fee", fee.Amount,
+ "quote_fee", quote.Fee,
+ "expiry", quote.Expiry)
quoteInfo = "e
break
}
+ ctx.Logger().Debug("skipping invalid bridge fee quote",
+ "fee", fee.Amount,
+ "quote_fee", quote.Fee,
+ "expiry", quote.Expiry)
}solidity/test/bridge_fee_quote.ts (1)
Line range hint 227-240: Improve test clarity and documentation.
While the test logic is correct, consider these improvements for better maintainability:
- The test name "first oracle quote" could be more descriptive, e.g., "should pad quotes array with empty quotes when fewer quotes exist"
- The hardcoded value '3' should be explained or referenced from a constant that defines the expected quote array size
- The test could be more explicit about the padding behavior in its description
Example improvement:
- it("first oracle quote", async function () {
+ it("should pad quotes array with empty quotes when fewer quotes exist", async function () {
+ // The system maintains a fixed-size array of 3 quotes per token,
+ // padding with empty quotes when fewer quotes exist
await bridgeFeeOracle.setDefaultOracle(oracle);solidity/contracts/bridge/BridgeFeeQuote.sol (2)
204-204: Consider upgrade implications of function rename.
The function rename from getQuoteByToken to getQuotesByToken is a breaking change that will affect existing integrations. Since this is an upgradeable contract, ensure that:
- All dependent contracts and dApps are notified about this change
- Proper documentation is provided for the upgrade
- Consider providing a migration period where both function names are supported
Consider adding a deprecated version of the old function that calls the new one:
function getQuoteByToken(
string memory _chainName,
string memory _tokenName
) external view returns (QuoteInfo[] memory) {
return getQuotesByToken(_chainName, _tokenName);
}Optimization suggestion needs to be revised
After analyzing the complete contract, I need to revise my previous review. The contract already has proper optimizations in other functions like getQuoteList which uses currentActiveQuoteNum and filters expired quotes. However, getQuotesByToken could benefit from similar optimizations:
function getQuotesByToken(
string memory _chainName,
string memory _tokenName
) external view returns (QuoteInfo[] memory) {
if (!assets[_chainName].isActive) {
revert ChainNameInvalid();
}
+ if (!isActiveTokenName(_chainName, _tokenName)) {
+ revert TokenNameInvalid();
+ }
address oracle = IBridgeFeeOracle(oracleContract).defaultOracle();
- QuoteInfo[] memory quotesList = new QuoteInfo[](maxQuoteIndex);
+ // Count valid quotes first
+ uint256 validCount = 0;
+ for (uint256 i = 0; i < maxQuoteIndex; i++) {
+ bytes memory asset = packAsset(_chainName, _tokenName, oracle, i);
+ if (quotes[asset].expiry >= block.timestamp) {
+ validCount++;
+ }
+ }
+
+ QuoteInfo[] memory quotesList = new QuoteInfo[](validCount);
+ uint256 currentIndex = 0;
for (uint256 i = 0; i < maxQuoteIndex; i++) {
bytes memory asset = packAsset(_chainName, _tokenName, oracle, i);
+ if (quotes[asset].expiry >= block.timestamp) {
quotesList[currentIndex] = QuoteInfo({
id: quotes[asset].id,
chainName: _chainName,
tokenName: _tokenName,
oracle: oracle,
fee: quotes[asset].fee,
gasLimit: quotes[asset].gasLimit,
expiry: quotes[asset].expiry
});
+ currentIndex++;
+ }
}
return quotesList;
}This change:
- Adds token name validation (consistent with other functions)
- Only returns non-expired quotes (consistent with
getQuoteList) - Optimizes array size (consistent with
getQuoteList)
🔗 Analysis chain
Line range hint 204-225: Several improvements needed in the getQuotesByToken function.
The function has multiple areas that could be improved for better efficiency and security:
- The function creates a fixed-size array of
maxQuoteIndexlength without checking if valid quotes exist, potentially wasting gas and memory. - Missing validation for
_tokenNameparameter. - Returns expired quotes without filtering.
Consider applying these improvements:
function getQuotesByToken(
string memory _chainName,
string memory _tokenName
) external view returns (QuoteInfo[] memory) {
if (!assets[_chainName].isActive) {
revert ChainNameInvalid();
}
+ if (!isActiveTokenName(_chainName, _tokenName)) {
+ revert TokenNameInvalid();
+ }
address oracle = IBridgeFeeOracle(oracleContract).defaultOracle();
- QuoteInfo[] memory quotesList = new QuoteInfo[](maxQuoteIndex);
+ // First pass: count valid quotes
+ uint256 validQuoteCount = 0;
+ for (uint256 i = 0; i < maxQuoteIndex; i++) {
+ bytes memory asset = packAsset(_chainName, _tokenName, oracle, i);
+ if (quotes[asset].expiry >= block.timestamp) {
+ validQuoteCount++;
+ }
+ }
+
+ // Second pass: populate array with only valid quotes
+ QuoteInfo[] memory quotesList = new QuoteInfo[](validQuoteCount);
+ uint256 currentIndex = 0;
for (uint256 i = 0; i < maxQuoteIndex; i++) {
bytes memory asset = packAsset(_chainName, _tokenName, oracle, i);
- quotesList[i] = QuoteInfo({
- id: quotes[asset].id,
- chainName: _chainName,
- tokenName: _tokenName,
- oracle: oracle,
- fee: quotes[asset].fee,
- gasLimit: quotes[asset].gasLimit,
- expiry: quotes[asset].expiry
- });
+ if (quotes[asset].expiry >= block.timestamp) {
+ quotesList[currentIndex] = QuoteInfo({
+ id: quotes[asset].id,
+ chainName: _chainName,
+ tokenName: _tokenName,
+ oracle: oracle,
+ fee: quotes[asset].fee,
+ gasLimit: quotes[asset].gasLimit,
+ expiry: quotes[asset].expiry
+ });
+ currentIndex++;
+ }
}
return quotesList;
}This refactored version:
- Validates the token name
- Only returns active (non-expired) quotes
- Allocates memory efficiently based on the actual number of valid quotes
Let's verify the impact of these changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar patterns in other quote-related functions
rg -A 5 "new QuoteInfo.*memory.*\(" .
Length of output: 39
Script:
#!/bin/bash
# Let's verify the existence and usage of QuoteInfo struct and related functions
ast-grep --pattern 'struct QuoteInfo {
$$$
}'
# Check for any functions returning QuoteInfo
ast-grep --pattern 'function $_($$$) $$ returns (QuoteInfo[] memory) {
$$$
}'
# Check for token name validation patterns
ast-grep --pattern 'function isActiveTokenName($$$) $$ {
$$$
}'
# Check for existing quote expiry checks
rg -B 2 -A 2 "expiry.*>=.*block\.timestamp" .
Length of output: 1534
Script:
#!/bin/bash
# Let's check the complete BridgeFeeQuote contract to understand the context better
cat solidity/contracts/bridge/BridgeFeeQuote.sol
# Also check for token validation patterns in a broader context
rg -B 2 -A 2 "revert TokenNameInvalid" .
# Check for any existing array size optimizations
rg -B 2 -A 2 "validQuoteCount|currentIndex" .
Length of output: 16762
contract/ibridge_fee_quote.sol.go (1)
Line range hint 305-333: Consider enhancing method documentation.
While the implementation is correct, consider adding a note in the documentation about the named return parameter 'quotes' to maintain consistency with the Solidity contract.
Update the documentation comment to:
-// Solidity: function getQuotesByToken(string _chainName, string _tokenName) view returns((uint256,string,string,address,uint256,uint256,uint256)[] quotes)
+// Solidity: function getQuotesByToken(string _chainName, string _tokenName) view returns((uint256,string,string,address,uint256,uint256,uint256)[] quotes)
+// Returns an array of quotes (named return parameter 'quotes') for the specified chain and token📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- app/keepers/keepers.go (8 hunks)
- contract/bridge_fee_quote.go (1 hunks)
- contract/contract.go (2 hunks)
- contract/ibridge_fee_quote.sol.go (4 hunks)
- solidity/contracts/bridge/BridgeFeeQuote.sol (1 hunks)
- solidity/contracts/bridge/IBridgeFee.sol (1 hunks)
- solidity/test/bridge_fee_quote.ts (1 hunks)
- x/crosschain/keeper/keeper.go (3 hunks)
- x/crosschain/keeper/keeper_test.go (2 hunks)
- x/crosschain/keeper/outgoing_tx.go (2 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (2 hunks)
- x/crosschain/types/expected_keepers.go (2 hunks)
🔇 Additional comments (18)
contract/bridge_fee_quote.go (1)
1-45: Verify contract interface alignment
The implementation assumes the existence of getQuotesByToken and getQuoteById methods in the smart contract. Let's verify this assumption.
#!/bin/bash
# Search for the contract interface definition
echo "Searching for contract interface..."
rg -A 10 "interface IBridgeFeeQuote"
# Search for the method definitions
echo "Searching for method definitions..."
ast-grep --pattern 'function getQuotesByToken'
ast-grep --pattern 'function getQuoteById'x/crosschain/keeper/keeper.go (4)
23-31: LGTM! Clean struct field organization.
The new brideFeeQuoteKeeper field is well-integrated with existing keeper dependencies, and the consistent field alignment improves readability.
41-41: LGTM! Constructor parameter addition is consistent.
The brideFeeQuoteKeeper parameter is appropriately placed alongside other keeper dependencies.
52-63: LGTM! Clean initialization with logical grouping.
The initialization maintains a clear structure with:
- Consistent field alignment
- Logical grouping of keeper dependencies
- Clear separation of authority and callback fields
31-31: Verify keeper initialization order in app module.
Since brideFeeQuoteKeeper is a new dependency, ensure that:
- The keeper is initialized before the crosschain keeper in the app module
- Methods using this keeper implement proper error handling for failed quote retrievals
x/crosschain/types/expected_keepers.go (1)
16-16: LGTM!
The import statement for the contract package is correctly placed and necessary for the IBridgeFeeQuoteQuoteInfo type.
x/crosschain/keeper/keeper_test.go (1)
17-17: LGTM!
The import statement is correctly placed and necessary for the BridgeFeeQuoteKeeper integration.
x/crosschain/keeper/outgoing_tx.go (2)
5-7: LGTM: Import additions are appropriate.
The new imports for big integer operations are necessary for handling bridge fee quotes and follow standard Go conventions.
60-60: 🛠️ Refactor suggestion
Consider validating the Oracle address format.
The Oracle address should be validated before using it as the FeeReceive value to ensure it's in the correct format.
Let's verify the Oracle address validation in the BridgeFeeQuoteKeeper:
Consider adding address validation:
- FeeReceive: quoteInfo.Oracle.String(),
+ FeeReceive: validateAndGetOracleAddress(quoteInfo.Oracle),Add this helper function:
func validateAndGetOracleAddress(oracle *common.Address) string {
if oracle == nil {
return ""
}
addr := oracle.String()
if !common.IsHexAddress(addr) {
return ""
}
return addr
}solidity/test/bridge_fee_quote.ts (1)
227-227: Verify the method name change across the codebase.
The method name has been changed from getQuoteByToken to getQuotesByToken. Let's verify this change is consistent across all related files.
✅ Verification successful
Method name change is consistent across the codebase
The search results confirm that the method name getQuotesByToken is used consistently across all relevant files:
- Interface definition in
IBridgeFee.sol - Implementation in
BridgeFeeQuote.sol - Test usage in
bridge_fee_quote.ts - Generated Go bindings in contract files
There are no instances of the old method name getQuoteByToken found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of the old method name
# and verify the new method name is used consistently.
echo "Checking for old method name 'getQuoteByToken'..."
rg "getQuoteByToken"
echo -e "\nVerifying new method name 'getQuotesByToken' usage..."
rg "getQuotesByToken"
Length of output: 6379
app/keepers/keepers.go (2)
383-384: Verify BridgeFeeQuoteKeeper initialization parameters.
The BridgeFeeQuoteKeeper is initialized with:
- EvmKeeper - for interacting with EVM functionality
- BridgeFeeAddress - a constant address for fee collection
- Each initialization is followed by the authAddr parameter for proper authorization
Let's verify these dependencies are properly set up.
Also applies to: 398-399, 413-414, 428-429, 443-444, 458-459, 473-474, 488-489
383-383: Consistent integration of BridgeFeeQuoteKeeper across all chain-specific keepers.
The BridgeFeeQuoteKeeper is consistently initialized and integrated across all chain-specific keepers (BSC, Polygon, Avalanche, Ethereum, Arbitrum, Optimism, Layer2, and Tron) using the same pattern and parameters.
Let's verify that the BridgeFeeAddress constant is properly defined:
Also applies to: 398-398, 413-413, 428-428, 443-443, 458-458, 473-473, 488-488
✅ Verification successful
Let me verify the BridgeFeeQuoteKeeper initialization and its dependencies further:
Let me try a different approach to verify the BridgeFeeQuoteKeeper implementation:
Let me check the BridgeFeeQuoteKeeper's structure and implementation:
BridgeFeeQuoteKeeper integration is properly implemented and consistent
The verification confirms:
- BridgeFeeQuoteKeeper is properly defined with required fields (Caller, abi, from, contract)
- It implements the expected interface
BrideFeeQuoteKeeperwithGetQuotesByTokenmethod - The constant
BridgeFeeAddressis correctly defined as "0x0000000000000000000000000000000000001005" - The keeper is consistently initialized across all chain-specific keepers with the correct dependencies (EvmKeeper and BridgeFeeAddress)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BridgeFeeAddress definition
rg -A 2 "BridgeFeeAddress.*=.*"
Length of output: 165
Script:
#!/bin/bash
# Check BridgeFeeQuoteKeeper implementation and usage
ast-grep --pattern 'type BridgeFeeQuoteKeeper struct {
$$$
}'
# Also check the constructor
ast-grep --pattern 'func NewBridgeFeeQuoteKeeper($$$) $$ {
$$$
}'
Length of output: 135
Script:
#!/bin/bash
# Search for BridgeFeeQuoteKeeper in the codebase
rg -A 5 "BridgeFeeQuoteKeeper" --type go
# Also check for any interfaces it might implement
rg -B 3 -A 3 "interface.*BridgeFee" --type go
Length of output: 3688
Script:
#!/bin/bash
# Check the BridgeFeeQuoteKeeper type and implementation
rg -A 10 "type BrideFeeQuoteKeeper struct" --type go
# Check for any related interfaces or types
rg -A 5 "type.*BrideFee" --type go
Length of output: 2905
contract/ibridge_fee_quote.sol.go (2)
63-63: LGTM: ABI definition is consistent with the implementation.
The ABI definition correctly includes the new methods getQuoteList and getQuotesByToken with appropriate signatures and return types.
Line range hint 274-302: LGTM: GetQuoteList implementation is well-structured.
The implementation:
- Properly handles errors
- Correctly converts the return type using
abi.ConvertType - Maintains consistency across caller, session, and caller session implementations
contract/contract.go (2)
21-21: LGTM: BridgeFeeAddress constant follows system contract address pattern.
The new constant follows the established pattern for system contract addresses (0x1001-0x1005) and maintains consistency with other address constants.
49-49: LGTM: bridgeFeeQuoteABI initialization is consistent.
The new ABI variable follows the established pattern of using MustABIJson for initialization and maintains consistency with other ABI declarations.
x/crosschain/mock/expected_keepers_mocks.go (2)
25-25: LGTM: Import statement correctly added
The contract package import is properly added to support the new mock implementation.
757-793: LGTM: Mock implementation follows best practices
The MockBrideFeeQuoteKeeper implementation:
- Correctly follows the gomock patterns and conventions
- Properly implements the required interface method
GetQuotesByToken - Includes appropriate helper methods (EXPECT, constructor)
- Uses correct type assertions and error handling
…idgeFeeQuoteKeeper' (#790)
Summary by CodeRabbit
Release Notes
New Features
BridgeFeeQuoteKeeperfor enhanced cross-chain fee management.Keeperstruct to include the newBridgeFeeQuoteKeeper.Bug Fixes
Documentation
Tests