fix: transfer bridge fee #878
Conversation
WalkthroughThis pull request introduces significant changes to the crosschain module's bridge call and fee handling mechanisms. The modifications focus on renaming methods, simplifying fee transfer logic, and adding new functions for quote validation and fee management. The changes streamline the process of handling bridge call fees, introducing a more consolidated approach to managing quote validation, fee transfers, and module-level fee collection. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant BridgeModule
participant QuoteValidator
participant FeeTransfer
Caller->>BridgeModule: Initiate Bridge Call
BridgeModule->>QuoteValidator: Validate Quote
QuoteValidator-->>BridgeModule: Return Validated Quote
BridgeModule->>FeeTransfer: Transfer Bridge Fee
FeeTransfer-->>BridgeModule: Confirm Fee Transfer
BridgeModule-->>Caller: Complete Bridge Call
Possibly related PRs
Poem
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
🧹 Nitpick comments (5)
x/crosschain/keeper/bridge_call_in.go (1)
140-152: Add validation forgasLimitinhandlerBridgeCallInFee.Currently,
gasLimitis accepted without validation. It's important to ensure thatgasLimitis notniland has a positive value to prevent issues during comparison in downstream logic.Apply this diff to validate
gasLimit:func (k Keeper) handlerBridgeCallInFee(ctx sdk.Context, from common.Address, quoteId, gasLimit *big.Int) error { + if gasLimit == nil || gasLimit.Sign() <= 0 { + return types.ErrInvalid.Wrap("invalid gasLimit: must be positive") + } if quoteId == nil || quoteId.Sign() <= 0 { // Allow free bridgeCall return nil } quote, err := k.ValidateQuote(ctx, quoteId, gasLimit) if err != nil { return err } return k.TransferBridgeFee(ctx, from, quote.Oracle, quote.Fee, quote.TokenName) }x/crosschain/keeper/bridge_fee.go (2)
16-28: ValidategasLimitparameter inValidateQuotefunction.The function assumes that
gasLimitis notnil. To prevent potentialnilpointer dereferences, add a check to ensuregasLimitis notniland has a non-negative value before proceeding.Apply this diff to add validation:
func (k Keeper) ValidateQuote(ctx sdk.Context, quoteId, gasLimit *big.Int) (contract.IBridgeFeeQuoteQuoteInfo, error) { + if gasLimit == nil || gasLimit.Sign() < 0 { + return contract.IBridgeFeeQuoteQuoteInfo{}, types.ErrInvalid.Wrap("invalid gasLimit: must be non-negative") + } quote, err := k.bridgeFeeQuoteKeeper.GetQuoteById(ctx, quoteId) if err != nil { return contract.IBridgeFeeQuoteQuoteInfo{}, err } if quote.IsTimeout(ctx.BlockTime()) { return contract.IBridgeFeeQuoteQuoteInfo{}, types.ErrInvalid.Wrap("quote has timed out") } if quote.GasLimit.Cmp(gasLimit) < 0 { return contract.IBridgeFeeQuoteQuoteInfo{}, types.ErrInvalid.Wrap("quote gas limit is less than requested gas limit") } return quote, nil }
31-34: Ensure consistent denomination comparison forbridgeTokenName.Comparing
bridgeTokenNameusingstrings.ToUppermight lead to incorrect matches iffxtypes.DefaultDenomis not also converted to uppercase. Ensure that both are compared in a case-insensitive manner.Apply this diff to standardize the comparison:
if strings.EqualFold(bridgeTokenName, fxtypes.DefaultDenom) { fees := sdk.NewCoins(sdk.NewCoin(fxtypes.DefaultDenom, sdkmath.NewIntFromBigInt(bridgeFee))) return k.bankKeeper.SendCoins(ctx, from.Bytes(), to.Bytes(), fees) }x/crosschain/keeper/keeper.go (1)
34-36: Maintain field ordering and grouping in theKeeperstruct.For better readability and maintainability, consider grouping related fields together. Place the
bridgeFeeCollectorfield next to other address-related fields.Apply this diff to adjust field ordering:
type Keeper struct { // ... other fields ... bankKeeper types.BankKeeper ak types.AccountKeeper ibcTransferKeeper types.IBCTransferKeeper erc20Keeper types.Erc20Keeper evmKeeper types.EVMKeeper bridgeFeeQuoteKeeper types.BridgeFeeQuoteKeeper erc20TokenKeeper types.ERC20TokenKeeper - authority string - callbackFrom common.Address + authority string + callbackFrom common.Address + bridgeFeeCollector sdk.AccAddress }x/crosschain/keeper/bridge_call_out.go (1)
406-415: Consider extracting bridgeFeeAddr initialization.The
bridgeFeeAddrinitialization appears in multiple places in the code. Consider extracting it to a method or initializing it once in the Keeper struct for better maintainability.type Keeper struct { // existing fields + bridgeFeeAddress common.Address } func NewKeeper(...) Keeper { // existing initialization + k.bridgeFeeAddress = common.BytesToAddress(k.bridgeFeeCollector) return k } func (k Keeper) TransferBridgeFeeToRelayer(ctx sdk.Context, bridgeCallNonce uint64) error { quote, found := k.GetOutgoingBridgeCallQuoteInfo(ctx, bridgeCallNonce) if !found { return nil } k.DeleteOutgoingBridgeCallQuoteInfo(ctx, bridgeCallNonce) - bridgeFeeAddr := common.BytesToAddress(k.bridgeFeeCollector) - return k.TransferBridgeFee(ctx, bridgeFeeAddr, quote.OracleAddress(), quote.Fee.BigInt(), quote.Token) + return k.TransferBridgeFee(ctx, k.bridgeFeeAddress, quote.OracleAddress(), quote.Fee.BigInt(), quote.Token) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/crosschain/keeper/bridge_call_in.go(2 hunks)x/crosschain/keeper/bridge_call_out.go(3 hunks)x/crosschain/keeper/bridge_fee.go(1 hunks)x/crosschain/keeper/keeper.go(2 hunks)x/crosschain/types/key.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (6)
x/crosschain/keeper/bridge_call_in.go (1)
151-151: Confirm thatTransferBridgeFeehandles all possible error scenarios.Ensure that
TransferBridgeFeeproperly handles errors, especially in cases where the fee transfer might fail due to insufficient funds or incorrect addresses. Consider adding context to the error messages for better traceability.x/crosschain/keeper/bridge_fee.go (1)
35-40: Handle potential nil values and errors in ERC20 token operations.
- Confirm that
erc20Tokenis notnilafter retrieval.- Ensure
Transfererrors are properly handled, and consider adding more descriptive error messages if needed.x/crosschain/keeper/keeper.go (1)
66-67: Verify correct initialization ofcallbackFromandbridgeFeeCollectoraddresses.Ensure that the module addresses are correctly derived from the module names and that they correspond to valid and expected addresses within the system.
Run the following script to confirm the addresses:
✅ Verification successful
Module addresses are correctly initialized and used consistently
- Constants are properly defined in
x/crosschain/types/key.go- BridgeCallSender account is properly initialized during genesis in
genesis.go- Address format conversions are appropriate for cross-chain functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify module addresses for 'BridgeCallSender' and 'BridgeFeeCollectorName' # Expected variables (replace with actual values if known) MODULE_NAME="crosschain" BRIDGE_CALL_SENDER="bridge_call_sender" BRIDGE_FEE_COLLECTOR_NAME="bridge_fee_collector" # Check module addresses echo "Module address for BridgeCallSender:" fx-core query auth module-account $BRIDGE_CALL_SENDER echo "Module address for BridgeFeeCollectorName:" fx-core query auth module-account $BRIDGE_FEE_COLLECTOR_NAMELength of output: 601
Script:
#!/bin/bash # Find constant definitions and their usage echo "=== Searching for BridgeCallSender and BridgeFeeCollectorName constants ===" rg "BridgeCallSender|BridgeFeeCollectorName" -A 2 -B 2 echo -e "\n=== Looking for NewModuleAddress implementation ===" ast-grep --pattern 'func NewModuleAddress($$$) {$$$}' echo -e "\n=== Checking other usages of these module addresses ===" rg "NewModuleAddress.*BridgeCallSender|NewModuleAddress.*BridgeFeeCollector" -A 2 -B 2Length of output: 2591
x/crosschain/types/key.go (1)
16-18: LGTM! Improved naming conventions.The updated constant names with underscores improve readability and maintain consistency across the codebase.
x/crosschain/keeper/bridge_call_out.go (2)
124-124: LGTM! Method name better reflects its purpose.The rename from
TransferQuoteFeeToRelayertoTransferBridgeFeeToRelayerbetter describes the function's purpose.
321-324: LGTM! Proper error handling maintained.The integration of
handlerBridgeCallOutFeemaintains proper error handling and control flow.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes