feat: add support for quote ID in bridgeCall #801
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing cross-chain functionality within the application. Key modifications include the integration of ERC20 token management capabilities into various keepers, updates to the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (12)solidity/contracts/bridge/IFxBridgeLogic.sol (2)
The introduction of
Let's check for the quote management implementation: ✅ Verification successfulLet me gather more information about the quote system implementation: Based on the results, I can now provide a comprehensive verification of the quote system implementation: Quote management system is properly integrated across the bridge The codebase shows a complete quote management implementation with:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Look for quote-related implementations
# Search for quote-related functions and structures
ast-grep --pattern 'function $_($$$ quote $$$) {
$$$
}'
# Search for quote validation patterns
rg "quote.*valid|valid.*quote" -A 3
Length of output: 587 Script: #!/bin/bash
# Search for quote-related code patterns
rg "quoteId|quote_id" -A 3
# Look for quote timeout/expiration implementations
rg "quote.*timeout|quote.*expir" -A 3
# Search for quote-related events or errors
rg "Quote.*Error|Quote.*Event" -A 3
# Check for quote documentation
fd "README|SPEC" --exec grep -l "quote"
Length of output: 73645
The change from Let's verify the consistency of this change across the codebase: ✅ Verification successfulThe Based on the comprehensive review of the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that all BridgeCallEvent emissions are updated with quoteId
# Search for BridgeCallEvent emissions
rg "emit BridgeCallEvent" -A 5
# Search for potential old value parameter references
rg "_value.*BridgeCallEvent|BridgeCallEvent.*_value"
Length of output: 984 Script: #!/bin/bash
# Let's get more context around the BridgeCallEvent emissions and check the full event parameters
# Get more context for BridgeCallEvent emissions
rg "emit BridgeCallEvent" -B 5 -A 10
# Check the implementation files for bridgeCall function
ast-grep --pattern 'function bridgeCall($$$) {
$$$
}'
Length of output: 2471 Script: #!/bin/bash
# Let's check the actual implementation of bridgeCall and its parameters
# Search for bridgeCall function declarations and implementations
rg "function bridgeCall.*\(" -A 3
# Check the full file content of IFxBridgeLogic.sol to understand the complete interface
cat solidity/contracts/bridge/IFxBridgeLogic.sol
Length of output: 18997 x/crosschain/types/msgs.go (2)
The validation logic for
Potential nil pointer dereference in ClaimHash. The Apply this diff to add a nil check: -path := fmt.Sprintf("%d/%d/%s/%s/%s/%s/%v/%v/%s", m.BlockHeight, m.EventNonce, m.Sender, m.Refund, m.To, m.TokenContracts, m.Amounts, m.Data, m.QuoteId.String())
+quoteIdStr := "nil"
+if m.QuoteId != nil {
+ quoteIdStr = m.QuoteId.String()
+}
+path := fmt.Sprintf("%d/%d/%s/%s/%s/%s/%v/%v/%s", m.BlockHeight, m.EventNonce, m.Sender, m.Refund, m.To, m.TokenContracts, m.Amounts, m.Data, quoteIdStr)contract/ifx_bridge_logic.sol.go (8)
The ABI string at line 62 has been updated to include the new
The Use the previous script to check for session method invocations that may need updating.
The Confirm that transactor session method calls include the
The
The event signature in
The
The
The Run the following script to find all usages of 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: 9
🧹 Outside diff range and nitpick comments (26)
docs/swagger-ui/swagger.yaml (1)
2026-2029: Enhance the field description for better clarity.While the current description is clear, it could be more informative for API consumers. Consider expanding it to explain:
- The purpose of persisting header hashes
- How it relates to cross-chain functionality and bridge calls
- Any constraints or valid ranges for the value
Example enhancement:
- description: header_hash_num is the number of header hash to persist. + description: | + The number of header hashes to persist in the EVM module. This value affects cross-chain + transaction verification and bridge call processing. Must be a positive integer. + example: "100"Also applies to: 40887-40890, 41087-41090
solidity/contracts/bridge/IBridgeCall.sol (1)
Line range hint
6-16: Consider adding NatSpec documentation.The interface would benefit from NatSpec documentation explaining:
- The purpose of the bridge call function
- The role and validation requirements of the quote ID
- Parameter descriptions, especially the new
_quoteIdExample addition:
interface IBridgeCall { + /// @notice Initiates a cross-chain bridge call using a pre-validated quote + /// @param _dstChain Destination chain identifier + /// @param _refund Address to receive any refunds + /// @param _tokens Array of token addresses to bridge + /// @param _amounts Array of token amounts to bridge + /// @param _to Destination address to receive tokens + /// @param _data Additional call data for the destination + /// @param _quoteId Identifier of the pre-validated quote for this bridge operation + /// @param _memo Additional metadata for the bridge operation + /// @return _eventNonce Unique identifier for the bridge event function bridgeCall(x/erc20/keeper/cache.go (1)
27-27: Method name should follow Go naming conventions.The method name
ReSetCacheuses unconventional capitalization. In Go, we use MixedCaps for method names. Consider renaming to either:
ReassignCache(preferred, as it better describes the intent)ResetCache(if the intent is to reset)x/crosschain/keeper/pending_execute_claim_test.go (1)
28-30: Consider enhancing test coverage for QuoteId scenarios.The current test only verifies a zero QuoteId. Consider adding test cases that:
- Use non-zero QuoteId values
- Verify any validation rules for QuoteId
- Test edge cases (e.g., negative values if applicable)
Example test case to add:
{ name: "msg bridge call claim with non-zero quote", claim: &types.MsgBridgeCallClaim{ ChainName: s.chainName, BridgerAddress: helpers.GenAccAddress().String(), EventNonce: 2, BlockHeight: 100, Sender: helpers.GenExternalAddr(s.chainName), Refund: helpers.GenExternalAddr(s.chainName), TokenContracts: []string{helpers.GenExternalAddr(s.chainName)}, Amounts: []sdkmath.Int{sdkmath.NewInt(1)}, To: helpers.GenExternalAddr(s.chainName), Data: "", QuoteId: sdkmath.NewInt(12345), Memo: "", TxOrigin: "", }, },solidity/contracts/bridge/ICrosschain.sol (1)
62-62: Document the purpose of_quoteIdparameterConsider adding NatSpec documentation to explain the purpose and expected values of the
_quoteIdparameter in the event signature.event BridgeCallEvent( address indexed _sender, address indexed _receiver, address indexed _to, address _txOrigin, uint256 _eventNonce, string _dstChain, address[] _tokens, uint256[] _amounts, bytes _data, + /// @param _quoteId Unique identifier for the bridge call quote uint256 _quoteId, bytes _memo );x/crosschain/keeper/bridge_call_in_test.go (2)
40-40: LGTM on QuoteId initialization, but test coverage could be improved.While the QuoteId field is correctly initialized to zero, the test suite would benefit from additional test cases that verify:
- Non-zero QuoteId values
- Invalid QuoteId scenarios
- Edge cases around QuoteId handling
Consider adding these test cases:
{ Name: "error - invalid quote id", Msg: types.MsgBridgeCallClaim{ // ... other fields ... QuoteId: sdkmath.NewInt(-1), // negative quote ID }, Success: false, }, { Name: "success - non-zero quote id", Msg: types.MsgBridgeCallClaim{ // ... other fields ... QuoteId: sdkmath.NewInt(123), }, Success: true, },
Line range hint
53-67: Enhance test verification for QuoteId handling.The current test only verifies ERC20 balances but doesn't validate the QuoteId processing. Consider adding assertions to verify how the QuoteId is handled during the bridge call.
Example verification:
if tc.Success { suite.Require().NoError(err) if !tc.CallContract { // Existing ERC20 balance checks... // Add verification for QuoteId handling // Example (adjust based on actual implementation): bridgeCall, found := suite.Keeper().GetBridgeCall(suite.Ctx, tc.Msg.EventNonce) suite.Require().True(found) suite.Require().Equal(tc.Msg.QuoteId, bridgeCall.QuoteId) } }types/constant.go (1)
87-89: Add documentation for the new function.The implementation looks good and follows the module's patterns. However, consider adding a documentation comment to explain the function's purpose and usage in the context of bridge operations.
Add this documentation:
+// IsOriginDenom checks if the provided denomination is the default FX token. +// This is commonly used in bridge operations to identify and handle the native token. func IsOriginDenom(denom string) bool { return denom == DefaultDenom }x/crosschain/keeper/keeper.go (2)
42-42: Consider constructor parameter improvementsWhile the implementation is correct, consider:
- Making the parameter name
erc20TokenKeeperconsistent with the field name- Using a configuration struct pattern to manage the growing parameter list
Example refactor:
type KeeperParams struct { Cdc codec.BinaryCodec ModuleName string StoreKey storetypes.StoreKey StakingKeeper types.StakingKeeper // ... other fields ... ERC20TokenKeeper types.ERC20TokenKeeper Authority string } func NewKeeper(params KeeperParams) Keeper { // ... implementation ... }Also applies to: 62-62
77-79: Add method documentationConsider adding GoDoc comments to explain the purpose and return value of this method, especially its relationship to EVM compatibility.
Example documentation:
// GetModuleEvmAddress returns the module's address in EVM-compatible format. // This is useful for cross-chain operations that require EVM address representation. func (k Keeper) GetModuleEvmAddress() common.Address { return common.BytesToAddress(k.ak.GetModuleAddress(k.moduleName)) }solidity/contracts/test/CrosschainTest.sol (1)
Line range hint
73-91: Consider adding documentation for the quoteId parameter.Since this is a test contract, it would be helpful to add NatSpec documentation explaining the purpose and expected values of the
_quoteIdparameter. This helps other developers understand how to use this contract for testing.Add documentation like this:
+ /// @notice Executes a bridge call with the specified parameters + /// @param _dstChain The destination chain identifier + /// @param _receiver The address receiving the bridged assets + /// @param _tokens Array of token addresses to bridge + /// @param _amounts Array of amounts to bridge for each token + /// @param _to The target contract address on the destination chain + /// @param _data The calldata to execute on the destination chain + /// @param _quoteId The unique identifier of the bridge quote + /// @param _memo Additional information for the bridge call + /// @return The event nonce of the bridge call function bridgeCall( string memory _dstChain, address _receiver,x/crosschain/keeper/bridge_call_in.go (1)
86-86: Add gas limit validation and document value removal.Two suggestions for improvement:
- Add validation for the gas limit returned by
GetBridgeCallMaxGasLimit- Add a comment explaining why the value parameter is nil in
ExecuteEVMConsider adding these changes:
func (k Keeper) BridgeCallEvm(ctx sdk.Context, sender, refundAddr, to, receiverAddr common.Address, baseCoins sdk.Coins, data, memo []byte, isMemoSendCallTo bool) error { + // Note: value parameter is nil as the token transfer is handled separately through ERC20 tokens ... gasLimit := k.GetBridgeCallMaxGasLimit(ctx) + if gasLimit == 0 { + return types.ErrInvalid.Wrap("invalid gas limit") + } txResp, err := k.evmKeeper.ExecuteEVM(ctx, callEvmSender, &to, nil, gasLimit, args)Also applies to: 117-117
x/crosschain/types/expected_keepers.go (3)
54-54: Consider renaming the method for better clarity and convention.The method name
ReSetCacheuses unconventional capitalization. Consider renaming it toResetCacheorTransferCacheto better reflect its purpose and follow Go naming conventions.
93-93: Method name should use "ID" instead of "Id".The functionality looks good and aligns with the PR objectives. However, consider renaming the method to
GetQuoteByIDto follow Go naming conventions where "ID" should be capitalized.
96-98: Add interface documentation.The new interface looks good and provides a clean abstraction for ERC20 token transfers. Consider adding documentation comments to explain:
- The purpose of this interface
- The expected behavior of the Transfer method
- Any requirements or constraints for the contract address
Example documentation:
// ERC20TokenKeeper defines the expected interface for ERC20 token operations // used in cross-chain transfersx/crosschain/precompile/bridge_call_test.go (1)
164-167: Consider adding comments to explain the test data structureThe test data is comprehensive but could benefit from comments explaining the structure of the expected data and topics, making it easier to maintain and debug.
Add comments like this:
expectData := "0000..." // hex-encoded packed data: +// [0:32] - sender address padded +// [32:64] - nonce +// [64:96] - quoteId +// ... expectTopic := []common.Hash{ - common.HexToHash("96da1d63..."), + common.HexToHash("96da1d63..."), // BridgeCallEvent signature + common.HexToHash("000000..."), // indexed sender + common.HexToHash("000000..."), // indexed refund + common.HexToHash("000000..."), // indexed to }proto/fx/gravity/crosschain/v1/tx.proto (2)
Line range hint
176-180: Consider adding field documentation for quote_id.The
quote_idfield appears to be a crucial identifier for bridge call operations. Consider adding protobuf comments to document:
- The purpose and significance of quote_id
- Format/constraints of valid quote IDs
- Relationship with other message fields
Example documentation:
+ // quote_id uniquely identifies the bridge call quote + // Format: [details about format/constraints] + // This ID is used to [explain purpose] string quote_id = 11 [ (cosmos_proto.scalar) = "cosmos.Int", (gogoproto.customtype) = "cosmossdk.io/math.Int", (gogoproto.nullable) = false ];
Line range hint
15-40: Consider documenting migration path from deprecated methods.The codebase is undergoing significant modernization with:
- Deprecation of specific confirm methods in favor of a unified
Confirm- Introduction of quote-based bridge calls
Consider adding migration guides or examples in the proto comments to help users transition from deprecated methods to the new patterns.
x/crosschain/keeper/abci.go (1)
178-181: Consider adding logging for bridge call resendThe resend logic is correct, but adding debug logs would help track these events during operations.
} else { // 1. resend bridge call + k.Logger(ctx).Debug("resending bridge call", "nonce", data.Nonce, "quoteId", quoteId) if err = k.ResendBridgeCall(ctx, *data, quoteId); err != nil { return true }solidity/test/bridge_call.ts (2)
151-151: Consider extracting quoteId as a shared test constant.The test correctly implements the quote ID changes. However, since the same quoteId value (1) is used across multiple tests, consider extracting it as a shared constant at the describe block level for better maintainability.
describe("bridge call tests", function () { const QUOTE_ID = 1; // ... existing test setup ...Also applies to: 168-168, 183-183
Line range hint
334-334: Update batch transfer tests to use quote ID.The batch transfer tests still use
value: 0in bridgeCall calls, while other tests have been updated to usequoteId. This inconsistency should be addressed to maintain test suite coherence and properly validate the new parameter.Apply this pattern to all batch transfer tests:
await fxBridge.bridgeCall( fxcoreChainId, user1.address, tokens, amounts, user1.address, "0x", - 0, + QUOTE_ID, "0x" );Also applies to: 366-366, 397-397
tests/crosschain_suite.go (1)
Line range hint
288-296: Add test cases for QuoteId validationWhile the implementation correctly reflects the architectural changes, consider adding test cases to validate:
- Non-zero quote IDs
- Invalid quote IDs (negative values)
- Edge cases around quote ID handling
This will ensure robust testing of the new quote ID functionality.
// Example test cases to add: func (suite *CrosschainTestSuite) TestBridgeCallClaimWithNonZeroQuoteId() { // Test with valid non-zero quote ID suite.BridgeCallClaim(to, tokens, amounts, sdkmath.NewInt(123)) } func (suite *CrosschainTestSuite) TestBridgeCallClaimWithInvalidQuoteId() { // Test with negative quote ID suite.BridgeCallClaim(to, tokens, amounts, sdkmath.NewInt(-1)) }solidity/contracts/bridge/FxBridgeLogic.sol (2)
Line range hint
299-303: Critical: Fix incorrect destination chain validationThe require statement is checking for an empty destination chain, which contradicts the function's purpose. This would prevent all bridge calls with valid destination chains from succeeding.
Apply this fix:
- require(bytes(_dstChain).length == 0, "Invalid dstChain"); + require(bytes(_dstChain).length > 0, "Invalid dstChain");
Line range hint
313-326: Fix event parameter ordering in bridgeCallThe event emission has incorrect parameter ordering. The
state_lastEventNonceis being emitted where_quoteIdshould be, which could lead to data corruption.Apply this fix:
emit BridgeCallEvent( _msgSender(), _refund, _to, tx.origin, - state_lastEventNonce, + _quoteId, + state_lastEventNonce, _dstChain, _tokens, _amounts, _data, - _quoteId, _memo );contract/icrosschain.sol.go (1)
Line range hint
1-2: Reminder: This is an auto-generated file.The changes look good, but remember that this file is auto-generated. Any manual modifications will be lost when the bindings are regenerated. Ensure that the source Solidity contract has been properly updated.
x/crosschain/types/msgs_test.go (1)
Line range hint
1615-1671: Add test cases for QuoteId validation.While the current test cases cover basic validation scenarios, consider adding the following test cases to improve coverage:
- Test case for zero QuoteId value
- Test case for negative QuoteId value
Here's a suggested test case to add:
{ name: "error - zero quote id", msg: &types.MsgBridgeCallClaim{ ChainName: moduleName, BridgerAddress: helpers.GenAccAddress().String(), EventNonce: 1, BlockHeight: 100, Sender: helpers.GenExternalAddr(moduleName), Refund: helpers.GenExternalAddr(moduleName), TokenContracts: []string{helpers.GenExternalAddr(moduleName)}, Amounts: []sdkmath.Int{sdkmath.NewInt(100)}, To: helpers.GenExternalAddr(moduleName), Data: "", QuoteId: sdkmath.ZeroInt(), // Zero quote ID Memo: "", TxOrigin: helpers.GenExternalAddr(moduleName), }, expectedError: "invalid quote id: invalid request", }, { name: "error - negative quote id", msg: &types.MsgBridgeCallClaim{ ChainName: moduleName, BridgerAddress: helpers.GenAccAddress().String(), EventNonce: 1, BlockHeight: 100, Sender: helpers.GenExternalAddr(moduleName), Refund: helpers.GenExternalAddr(moduleName), TokenContracts: []string{helpers.GenExternalAddr(moduleName)}, Amounts: []sdkmath.Int{sdkmath.NewInt(100)}, To: helpers.GenExternalAddr(moduleName), Data: "", QuoteId: sdkmath.NewInt(-1), // Negative quote ID Memo: "", TxOrigin: helpers.GenExternalAddr(moduleName), }, expectedError: "invalid quote id: invalid request", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/crosschain/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (37)
api/fx/gravity/crosschain/v1/tx.pulsar.go(17 hunks)app/keepers/keepers.go(8 hunks)contract/crosschain.go(1 hunks)contract/crosschain_precompile.go(1 hunks)contract/icrosschain.sol.go(5 hunks)contract/ifx_bridge_logic.sol.go(2 hunks)contract/quote.go(1 hunks)docs/swagger-ui/swagger.yaml(3 hunks)proto/fx/gravity/crosschain/v1/tx.proto(1 hunks)solidity/contracts/bridge/FxBridgeLogic.sol(3 hunks)solidity/contracts/bridge/FxBridgeLogicETH.sol(3 hunks)solidity/contracts/bridge/IBridgeCall.sol(1 hunks)solidity/contracts/bridge/ICrosschain.sol(1 hunks)solidity/contracts/test/CrosschainTest.sol(2 hunks)solidity/test/bridge_call.ts(9 hunks)tests/contract/crosschain_test.sol.go(1 hunks)tests/crosschain_suite.go(1 hunks)types/constant.go(1 hunks)x/crosschain/keeper/abci.go(1 hunks)x/crosschain/keeper/bridge_call_in.go(4 hunks)x/crosschain/keeper/bridge_call_in_test.go(1 hunks)x/crosschain/keeper/bridge_call_out.go(5 hunks)x/crosschain/keeper/keeper.go(4 hunks)x/crosschain/keeper/keeper_test.go(1 hunks)x/crosschain/keeper/outgoing_tx.go(1 hunks)x/crosschain/keeper/pending_execute_claim_test.go(1 hunks)x/crosschain/mock/expected_keepers_mocks.go(3 hunks)x/crosschain/precompile/bridge_call.go(3 hunks)x/crosschain/precompile/bridge_call_test.go(6 hunks)x/crosschain/precompile/expected_keepers.go(2 hunks)x/crosschain/types/events.go(1 hunks)x/crosschain/types/expected_keepers.go(2 hunks)x/crosschain/types/key.go(2 hunks)x/crosschain/types/msgs.go(2 hunks)x/crosschain/types/msgs_test.go(4 hunks)x/erc20/keeper/cache.go(2 hunks)x/erc20/types/token.go(1 hunks)
🔥 Files not summarized due to errors (1)
- api/fx/gravity/crosschain/v1/tx.pulsar.go: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (1)
- tests/contract/crosschain_test.sol.go
🔇 Additional comments (63)
docs/swagger-ui/swagger.yaml (1)
2026-2029: LGTM! Consistent field definition across all sections.
The new header_hash_num field is consistently defined across all relevant sections with proper OpenAPI specification formatting.
Also applies to: 40887-40890, 41087-41090
solidity/contracts/bridge/IBridgeCall.sol (1)
14-14: LGTM! Note: Breaking change in interface.
The parameter rename from _value to _quoteId improves clarity by better expressing the parameter's purpose. However, this is a breaking change that requires updates to all implementations.
Let's verify that all implementations have been updated:
✅ Verification successful
Based on the search results, I can now generate the final response:
Parameter rename to _quoteId has been consistently implemented
The verification shows that:
- All implementations use the new
_quoteIdparameter name - No instances of the old
_valueparameter name remain in the codebase - The change has been properly propagated to generated Go bindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old bridgeCall signature with _value parameter
ast-grep --pattern 'function bridgeCall($$$, uint256 _value, $$$)'
# Search for implementations of IBridgeCall interface
rg -l "contract.*implements.*IBridgeCall"
Length of output: 112
Script:
#!/bin/bash
# Let's try different approaches to find implementations and usages
# Search for contract files that import IBridgeCall
rg -l "import.*IBridgeCall"
# Search for contract files that mention bridgeCall function
rg -l "function bridgeCall"
# Search for actual function implementations with context
rg "function bridgeCall" -A 3
# Search for interface usages
rg "IBridgeCall"
Length of output: 23664
Script:
#!/bin/bash
# Let's search for the actual bridgeCall implementations to verify the parameter name
rg "function bridgeCall.*\(.*_quoteId" -A 1
# Also search for any remaining instances of the old parameter name
rg "function bridgeCall.*\(.*_value.*\)"
Length of output: 3191
x/erc20/types/token.go (1)
36-38: LGTM! Clean and consistent implementation.
The new GetContractAddress() method follows the same pattern as the existing GetERC20Contract() method, providing a standardized way to convert the contract address string to common.Address. This addition maintains consistency in the codebase and supports the broader ERC20 token management functionality.
x/erc20/keeper/cache.go (2)
6-7: LGTM: Appropriate imports added for the new functionality.
The new imports from Cosmos SDK are correctly added and utilized in the new method.
27-38: Verify the usage context of cache reassignment.
Let's verify how this cache reassignment is used in the bridge call context, particularly with quote IDs.
✅ Verification successful
Let me analyze the usage context further by searching for the actual implementation of the cache operations.
Let me search for more context about how this cache is used in the crosschain functionality.
Let me search for one final piece of context about how this cache is used in the bridge call flow.
Based on the gathered context, I can now provide a final response about the cache reassignment verification:
Cache reassignment is correctly implemented for bridge call token tracking
The ReSetCache method is properly used in the crosschain bridge context to maintain token tracking across bridge calls. Specifically:
- It's used by the crosschain keeper to track origin tokens via
NewOriginTokenKey - The cache is implemented as a
collections.Map[string, sdkmath.Int]for storing token amounts - The error handling follows the expected pattern for cache operations
- The method is correctly called during bridge call resending to maintain token state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ReSetCache to understand the context
rg -l "ReSetCache" --type go
# Search for related bridge call and quote ID handling
rg -A 5 "BridgeCallArgs|QuoteId" --type go
Length of output: 24662
Script:
#!/bin/bash
# Search for cache implementation and usage
rg -A 5 "type Cache interface" --type go
rg -A 5 "Cache\s+collections\." --type go
Length of output: 436
Script:
#!/bin/bash
# Search for cache usage in crosschain context
rg -A 10 "Cache.*Get|Cache.*Set" --type go
# Search for the actual implementation of ReSetCache
ast-grep --pattern 'func $_ ReSetCache($$$) {
$$$
}'
Length of output: 7079
Script:
#!/bin/bash
# Search for bridge call related cache operations
rg -A 10 "bridge.*cache|cache.*bridge" --type go -i
# Look for the actual cache implementation
rg -A 5 "collections\.Map\[string, sdkmath\.Int\]" --type go
Length of output: 3175
x/crosschain/types/events.go (2)
34-35: LGTM! Clear and descriptive event naming.
The replacement of the previous refund event with a more explicit "failed" event type improves clarity and better represents the actual state.
37-39: LGTM! Well-structured resend event tracking.
The new resend event type with both old and new nonce tracking provides excellent auditability for debugging bridge call retries.
x/crosschain/keeper/pending_execute_claim_test.go (1)
28-28: LGTM! Field renaming is consistent with codebase changes.
The replacement of Value with QuoteId aligns with the broader refactoring across the codebase.
x/crosschain/precompile/expected_keepers.go (2)
5-5: LGTM!
The addition of the "math/big" import is necessary to support the new QuoteId parameter type.
26-26: Verify implementations of CrosschainKeeper interface.
The interface change looks good and aligns with the PR objectives. The parameter ordering follows Go conventions with context first, and the new QuoteId parameter is appropriately typed as *big.Int.
Let's verify that all implementations of CrosschainKeeper have been updated:
✅ Verification successful
Interface implementation is correctly updated
The verification shows that the BridgeCallBaseCoin method is properly implemented in the Keeper struct (x/crosschain/keeper/bridge_call_out.go) with the exact same signature as defined in the interface, including all new parameters:
- The interface definition in
expected_keepers.go - The implementation in
keeper/bridge_call_out.go - The usage in
precompile/bridge_call.go
All instances are aligned with the updated signature including the new quoteId parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of CrosschainKeeper interface and verify they implement the updated BridgeCallBaseCoin method signature
# Search for BridgeCallBaseCoin implementations
ast-grep --pattern 'func $_($_ sdk.Context, $_ common.Address, $_ common.Address, $_ common.Address, $_ sdk.Coins, $_ []byte, $_ []byte, $_ *big.Int, $_ *crosschaintypes.FxTarget, $_ sdkmath.Int) (uint64, error)'
Length of output: 214
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find all files that might contain BridgeCallBaseCoin implementations
rg -l "BridgeCallBaseCoin" --type go
# Then look at the actual implementations with context
rg "BridgeCallBaseCoin.*\(" -A 5 --type go
Length of output: 2170
x/crosschain/keeper/bridge_call_in_test.go (1)
Line range hint 15-16: Please provide timeline for re-enabling the skipped test.
The test is currently skipped with a TODO comment. Since we're adding new functionality (QuoteId), it's crucial to have active tests verifying this behavior.
Would you like help in addressing the TODO and re-enabling this test?
contract/crosschain_precompile.go (2)
59-59: LGTM! The QuoteId parameter enhances security.
The replacement of Value with QuoteId suggests a move towards using pre-approved quotes for bridge transactions, which is a more secure approach as it prevents value manipulation during the bridge operation.
Line range hint 58-65: Verify integration with dependent systems.
This is a breaking change in the bridge call interface. Let's verify that all dependent systems are updated to use QuoteId instead of Value.
✅ Verification successful
Let me try a different search strategy to verify this change.
Based on the search results, I can see that the change from Value to QuoteId in the bridge call interface is a planned upgrade that has been properly implemented across the codebase. The evidence shows:
-
The change is documented in CHANGELOG.md as a new feature for bridge calls with automatic refund mechanisms
-
The interface change is consistently implemented in:
- Contract interfaces (solidity/contracts/bridge/*)
- Precompile implementations (x/crosschain/precompile/*)
- Tests have been updated to use QuoteId (solidity/test/bridge_*.ts)
- All dependent systems like BridgeCallFee and BridgeCallback have been updated
-
No remaining references to the old "Value" parameter were found in bridge call implementations
The interface change appears to be properly integrated across all dependent systems. The codebase shows a coordinated update where the Value parameter has been replaced with QuoteId throughout the bridge call functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old Value parameter in bridge calls
# This helps identify any places that need updates
echo "Searching for potential bridge call implementations that might need updates..."
rg -l "bridgeCall.*Value"
echo "Searching for tests that might need updates..."
rg -l "bridge.*test.*Value"
# Look for any bridge-related interfaces or ABIs that might need updates
echo "Checking for bridge-related interface definitions..."
ast-grep --pattern 'interface $_ {
$$$
bridgeCall($$$)
$$$
}'
Length of output: 512
Script:
#!/bin/bash
# Search for any bridgeCall usages with broader context
echo "Searching for bridgeCall usages..."
rg -A 5 -B 5 "bridgeCall"
# Search for any bridge-related contract ABIs or interfaces
echo "Searching for bridge contract definitions..."
fd -e abi -e json . | xargs cat
# Look for any test files related to bridge functionality
echo "Searching for bridge-related test files..."
fd -e test -e spec . | xargs rg "bridge"
Length of output: 504330
contract/crosschain.go (2)
55-55: LGTM: QuoteId field addition
The new QuoteId field is properly defined with the correct type and ABI tag.
55-55: Verify impact of Value to QuoteId migration
This is a breaking change that replaces the Value field with QuoteId. Please ensure:
- All callers of BridgeCallArgs are updated
- A migration path exists for any persisted BridgeCallArgs data
Let's verify the usage:
✅ Verification successful
Migration from Value to QuoteId field appears safe
Based on the verification results:
- The git history shows a clean replacement of
ValuewithQuoteIdinBridgeCallArgs - All references to
Valuein other files are unrelated - they belong to different structs/contracts (ERC20, WFX, etc.) - All code using
BridgeCallArgs(in tests and implementation) is already using the newQuoteIdfield - The change appears to be part of a coordinated update as all callers are aligned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old Value field
rg -i "bridgecallargs.*value"
# Search for places where BridgeCallArgs is constructed to ensure they're updated
ast-grep --pattern 'BridgeCallArgs{$$$}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a broader search to find BridgeCallArgs usage and construction
rg "BridgeCallArgs" -A 5 -B 5
# Also search for struct definition to understand the full context
rg "type BridgeCallArgs struct" -A 10
Length of output: 6747
Script:
#!/bin/bash
# Let's search for any historical references to Value field in git history
git log -p | rg -B 5 -A 5 "Value.*\*big\.Int.*abi:\".*\""
# Let's also check for any test files that might be using the old field
rg "Value.*big\.Int" --type go
Length of output: 1770
x/crosschain/keeper/keeper.go (2)
32-32: LGTM: ERC20TokenKeeper field addition
The new field follows Cosmos SDK keeper patterns and maintains proper interface segregation.
Line range hint 32-79: Verify keeper initialization in tests
Let's ensure proper test coverage for the new ERC20TokenKeeper integration.
solidity/contracts/test/CrosschainTest.sol (1)
78-78: Parameter change from _value to _quoteId looks good.
The change aligns with the PR objective to add quote ID support in bridgeCall functionality.
Let's verify the consistency of this change across related contracts:
Also applies to: 89-89
✅ Verification successful
The parameter change from _value to _quoteId is consistent across the codebase
The verification shows that:
- The
bridgeCallfunction signature in the interface (ICrosschain) and implementations (FxBridgeLogic,FxBridgeLogicETH,CrosschainTest) consistently use_quoteIdparameter - All generated bindings in Go code also reflect this parameter name
- Test files show proper usage of the
bridgeCallfunction with the expected parameter order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all bridgeCall implementations use quoteId consistently
# Expected: All bridgeCall functions should use quoteId parameter instead of value
# Search for bridgeCall function definitions
ast-grep --pattern 'function bridgeCall($$$) {
$$$
}'
# Search for bridgeCall function calls to ensure all callers pass quoteId
rg -A 5 'bridgeCall\('
Length of output: 10833
x/crosschain/keeper/keeper_test.go (1)
100-100: 🛠️ Refactor suggestion
Consider mocking the ERC20TokenKeeper for consistency.
While the nil parameter might work for testing, other keepers in this test suite are properly mocked using gomock. For consistency and better test isolation, consider creating a mock for ERC20TokenKeeper similar to other keepers.
Let's verify if there are any existing tests that might be affected by this change:
x/crosschain/keeper/bridge_call_in.go (3)
14-14: LGTM: Import addition aligns with new refund handling.
The added import is necessary for the new origin denomination check in the refund logic.
79-81: LGTM: Event emission changes improve clarity.
The change from BridgeCallRefundOut to BridgeCallFailed better represents the actual scenario, and including the refund address in the event attributes enhances observability.
65-76:
Consider transaction atomicity in refund handling.
The current implementation has a potential atomicity issue. If the bank transfer succeeds but the ERC20 conversion fails, we end up in an inconsistent state where the base coins are transferred but the ERC20 tokens are not converted. Consider:
- Using a cache context for the entire refund operation
- Committing only if both operations succeed
Here's a suggested implementation:
- if err = k.bankKeeper.SendCoins(ctx, receiverAddr.Bytes(), msg.GetRefundAddr().Bytes(), baseCoins); err != nil {
- return err
- }
-
- for _, coin := range baseCoins {
- if fxtypes.IsOriginDenom(coin.Denom) {
- continue
- }
- if _, err = k.erc20Keeper.BaseCoinToEvm(ctx, msg.GetRefundAddr(), coin); err != nil {
- return err
- }
- }
+ refundCtx, commit := ctx.CacheContext()
+ if err = k.bankKeeper.SendCoins(refundCtx, receiverAddr.Bytes(), msg.GetRefundAddr().Bytes(), baseCoins); err != nil {
+ return err
+ }
+
+ for _, coin := range baseCoins {
+ if fxtypes.IsOriginDenom(coin.Denom) {
+ continue
+ }
+ if _, err = k.erc20Keeper.BaseCoinToEvm(refundCtx, msg.GetRefundAddr(), coin); err != nil {
+ return err
+ }
+ }
+ commit()x/crosschain/precompile/bridge_call.go (3)
86-86: LGTM: QuoteId parameter correctly added to BridgeCallBaseCoin
The addition of args.QuoteId parameter aligns with the PR objective and maintains the logical flow of the bridge call operation.
132-132: Verify ABI compatibility with smart contracts
The input parameter packing has been updated to use QuoteId instead of Value. This change must align with the smart contract ABI definition.
#!/bin/bash
# Search for contract ABI definitions to verify compatibility
echo "=== Searching for ABI definitions ==="
rg -A 10 'bridgeCall.*\bfunction\b' --type solidity
echo "=== Searching for interface definitions ==="
rg -A 5 'interface.*CrossChain' --type solidity116-116: Verify event structure compatibility
The event structure has been updated to replace Value with QuoteId. While the change is correct, we should verify that all event consumers are updated to handle this new structure.
✅ Verification successful
Based on the search results, I can now provide a final response. The code shows that the event structure has been properly updated in both the contract interfaces (ICrosschain and IFxBridgeLogic) and the event emission code. The event consumers are also properly handling the new structure.
Event structure changes are compatible
The event structure has been correctly updated across all implementations:
- The Solidity contract interface shows
QuoteIdparameter in the event definition - The Go bindings (ICrosschainBridgeCallEvent and IFxBridgeLogicBridgeCallEvent) reflect this change
- Event emission in the keeper layer is compatible with the new structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BridgeCallEvent consumers to verify compatibility
rg -l "BridgeCallEvent" | while read -r file; do
echo "=== $file ==="
rg -A 5 "BridgeCallEvent" "$file"
done
Length of output: 351
Script:
#!/bin/bash
# Search for BridgeCallEvent references in Go files
rg -A 5 "BridgeCallEvent" --type go
# Search for event topic definitions and usages
rg -A 5 "PackTopicData.*BridgeCall" --type go
# Search for any files containing "bridge" and "event" patterns
rg -l -e "bridge.*event" -e "event.*bridge" --type go | while read -r file; do
echo "=== $file ==="
cat "$file"
done
# Look for test files that might verify event handling
fd -e go -E vendor | xargs rg -l "TestBridgeCall"
Length of output: 231613
Script:
#!/bin/bash
# Search for BridgeCallEvent event structure and its consumers
rg -A 5 "BridgeCallEvent.*struct" --type go
# Search for event emission calls
rg -A 5 "EmitEvent.*BridgeCallEvent" --type go
# Look for event handling/processing code
rg -A 5 "HandleBridgeCallEvent|ProcessBridgeCallEvent" --type go
# Search for event tests
rg -A 10 "TestBridgeCall.*Event" --type go
Length of output: 20767
x/crosschain/types/expected_keepers.go (1)
93-94: Verify quote ID handling across the codebase.
Let's ensure that the quote ID integration is properly handled across the codebase.
✅ Verification successful
Let me gather more specific information about the quote ID implementation and usage.
Let me check one more aspect to verify the complete integration of quote ID handling.
Based on the comprehensive verification results, I can now provide the final response:
Quote ID integration is properly implemented and verified
The quote ID functionality is well-integrated across the codebase with:
- Complete implementation of
GetQuoteByIdin theBridgeFeeQuoteKeeperinterface - Proper state management through
SetOutgoingBridgeCallQuoteId,GetOutgoingBridgeCallQuoteId, andDeleteOutgoingBridgeCallQuoteId - Quote validation and timeout checks in bridge call processing
- Correct handling in message structures with appropriate validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for quote ID usage patterns to verify integration
echo "Checking quote ID usage patterns..."
rg -l "GetQuoteById|QuoteId|quote.*[iI][dD]" --type go
# Look for potential bridge call implementations
echo "Checking bridge call implementations..."
ast-grep --pattern 'func $$$bridgeCall$$$($$$) $$$'
Length of output: 1021
Script:
#!/bin/bash
# Check the actual implementation of GetQuoteById
echo "Checking GetQuoteById implementations..."
rg "GetQuoteById" -B 2 -A 5
# Check bridge call related code that uses quote ID
echo "Checking quote ID usage in bridge calls..."
rg "quoteId|quote.*ID" -B 2 -A 5 "x/crosschain/keeper/bridge_call"
# Check message handling for quote IDs
echo "Checking message structures with quote IDs..."
rg "QuoteId|quote.*ID" "x/crosschain/types/msgs.go" -B 2 -A 5
Length of output: 7919
Script:
#!/bin/bash
# Check bridge call out implementation and state handling
echo "Checking bridge call state handling..."
rg "OutgoingBridgeCallQuoteId" -B 2 -A 5
# Check tests related to quote ID functionality
echo "Checking quote ID related tests..."
rg "quoteId|quote.*ID" "x/crosschain/keeper/bridge_call_.*test.go" -B 2 -A 5
Length of output: 4642
x/crosschain/precompile/bridge_call_test.go (4)
38-38: LGTM: Args struct updated correctly with QuoteId field
The replacement of Value with QuoteId is implemented correctly, maintaining proper type safety with *big.Int.
Also applies to: 51-54
63-63: LGTM: Input packing updated correctly
The packing order correctly reflects the updated ABI signature with QuoteId.
119-121: LGTM: Event data packing updated correctly
The event data structure and packing logic have been properly updated to include QuoteId.
Also applies to: 130-131
159-161: LGTM: BridgeCallArgs test data updated appropriately
The test case correctly includes QuoteId with appropriate test values.
x/crosschain/types/key.go (2)
116-117: LGTM: Key constant follows module conventions.
The new BridgeCallQuoteKey constant with value 0x55 follows the module's byte prefix pattern and maintains a clear separation from existing keys.
209-211: LGTM: Key generation function is well-implemented.
The GetBridgeCallQuoteKey function follows the established pattern of other key generation functions in the module, correctly using sdk.Uint64ToBigEndian for nonce conversion.
proto/fx/gravity/crosschain/v1/tx.proto (1)
Line range hint 176-180: Field renaming implementation looks good, but verify compatibility impact.
The change from value to quote_id is well-implemented with proper type annotations and maintains the field number for wire format compatibility.
Let's verify the impact of this change:
✅ Verification successful
Let me gather more information to verify the impact of this change across the codebase.
Let me search for any remaining references to the old field name in the codebase to ensure we haven't missed anything.
Let me check one more thing to verify the ClaimHash implementation and any potential impacts.
Based on the comprehensive analysis of the codebase through the executed scripts, I can now provide the final response:
Field renaming from value to quote_id is properly implemented with no compatibility issues.
The change is well-implemented because:
- The field number (11) is preserved, maintaining wire format compatibility
- The type annotations (cosmos.Int, nullable=false) are correctly specified
- The ClaimHash implementation has been updated to use QuoteId
- No legacy references to the old 'value' field were found in the codebase
- The generated protobuf code correctly handles the new field name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old field name in proto files
rg -t proto "value.*=.*11" --glob "proto/**"
# Search for any direct uses of MsgBridgeCallClaim in Go code
rg "MsgBridgeCallClaim.*value" --type go
Length of output: 1922
Script:
#!/bin/bash
# Search for any references to the old 'value' field in proto files
rg -t proto "value.*=.*11" proto/
# Look for any references to MsgBridgeCallClaim in Go code that might need updates
rg "MsgBridgeCallClaim" --type go -A 5 -B 5
# Search for any tests or handlers that might be affected
rg "BridgeCallClaim.*value" --type go
Length of output: 95576
Script:
#!/bin/bash
# Search for any references to 'value' field in MsgBridgeCallClaim
rg -t proto "value.*=.*11" proto/
rg "value.*=.*11" --type go
# Search for any references to the old field in tests or handler code
rg "MsgBridgeCallClaim.*value" --type go -A 2 -B 2
# Look for any references in ClaimHash implementation
rg "ClaimHash.*value" --type go
Length of output: 6710
Script:
#!/bin/bash
# Search for ClaimHash implementation in MsgBridgeCallClaim
rg "func.*ClaimHash.*MsgBridgeCallClaim" --type go -A 10
# Look for any references to the old value field in ClaimHash
rg "value.*String\(\)" --type go -B 5
# Check for any migration code that might need updates
rg "MsgBridgeCallClaim.*migrate" --type go
Length of output: 12927
x/crosschain/keeper/abci.go (2)
171-181: Verify the cleanup of associated quote ID data
The function deletes the bridge call record but should also clean up any associated quote ID data to prevent orphaned storage.
#!/bin/bash
# Search for quote ID cleanup in DeleteOutgoingBridgeCallRecord
rg -A 5 "DeleteOutgoingBridgeCallRecord"171-177: Verify error handling for GetOutgoingBridgeCallQuoteId
The quote ID retrieval and refund logic looks correct. However, we should verify that GetOutgoingBridgeCallQuoteId properly handles any potential storage errors.
solidity/test/bridge_call.ts (2)
114-114: LGTM! Test correctly implements quote ID support.
The test case properly replaces the value parameter with quoteId and correctly verifies the emitted event arguments.
Also applies to: 127-127, 142-142
192-192: LGTM! Test properly handles quote ID with memo.
The test case correctly implements the quote ID parameter while maintaining the memo functionality testing.
Also applies to: 215-215, 230-230
app/keepers/keepers.go (2)
Line range hint 407-519: Architecture enhancement for bridge call quotes with ERC20 support.
The addition of ERC20TokenKeeper alongside the existing BridgeFeeQuoteKeeper creates a robust architecture for handling bridge calls with quote IDs. This design:
- Separates concerns between fee quotes and token management
- Maintains consistency across all supported chains
- Provides a foundation for quote-based bridge operations with ERC20 tokens
Let's verify the bridge call integration:
407-407: LGTM! Consistent integration of ERC20 token management.
The addition of contract.NewERC20TokenKeeper(appKeepers.EvmKeeper) is consistently implemented across all cross-chain keepers (BSC, Polygon, Avalanche, Eth, Arbitrum, Optimism, Layer2, and Tron), which maintains code consistency and ensures uniform ERC20 token handling capabilities.
Let's verify the ERC20TokenKeeper integration:
Also applies to: 423-423, 439-439, 455-455, 471-471, 487-487, 503-503, 519-519
✅ Verification successful
ERC20TokenKeeper integration is properly implemented and consistent
The verification confirms that:
- The
ERC20TokenKeeperinterface is well-defined in bothx/crosschain/typesandx/erc20/typeswith expected methods - The concrete implementation in
contract/erc20_token.goprovides all required functionality - The integration in
app/keepers/keepers.gois consistent across all cross-chain keepers, correctly usingcontract.NewERC20TokenKeeper(appKeepers.EvmKeeper) - The keeper is properly used throughout the codebase in tests and other modules
The implementation maintains consistency and provides the necessary ERC20 token management capabilities across all chains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that ERC20TokenKeeper is properly defined and has the expected interface
ast-grep --pattern 'type ERC20TokenKeeper interface {
$$$
}'
# Verify that the NewERC20TokenKeeper function exists with correct signature
ast-grep --pattern 'func NewERC20TokenKeeper($_) $_'
Length of output: 440
Script:
#!/bin/bash
# Let's try a different approach to verify the ERC20TokenKeeper implementation
# Search for ERC20TokenKeeper interface or struct definition
rg -A 5 "type ERC20TokenKeeper"
# Search for NewERC20TokenKeeper function implementation
rg -A 5 "func NewERC20TokenKeeper"
# Search for all usages of ERC20TokenKeeper to understand the integration
rg "ERC20TokenKeeper"
Length of output: 9850
solidity/contracts/bridge/FxBridgeLogic.sol (1)
823-825: LGTM: Event declaration updated correctly
The BridgeCallEvent declaration has been properly updated to use _quoteId instead of _value.
solidity/contracts/bridge/FxBridgeLogicETH.sol (3)
Line range hint 343-362: LGTM! Event emission is consistent with the parameter change
The event emission correctly includes the new _quoteId parameter, maintaining consistency with the function signature change and providing necessary data for cross-chain tracking.
863-865: LGTM! Event definition follows best practices
The event definition is well-structured with:
- Appropriate indexed parameters for efficient filtering
- Consistent parameter naming with the function signature
- Clear parameter ordering that matches the event emission
335-338: Add validation for _quoteId
Consider adding validation to ensure _quoteId is valid before proceeding with the bridge call. An invalid quote ID could lead to failed transactions and poor user experience.
Let's check if there are any quote validation patterns in the codebase:
✅ Verification successful
Based on the search results, I can see that quote validation is indeed implemented in the codebase, specifically in x/crosschain/keeper/bridge_call_out.go:
func (k Keeper) handleBridgeCallQuote(ctx sdk.Context, from common.Address, bridgeCallNonce uint64, quoteId *big.Int) error {
if quoteId == nil || quoteId.Sign() <= 0 {
return nil
}
quoteInfo, err := k.brideFeeQuoteKeeper.GetQuoteById(ctx, quoteId)
if err != nil {
return err
}
if quoteInfo.IsTimeout(ctx.BlockTime()) {
return types.ErrInvalid.Wrapf("quote is timeout")
}Additionally, in contract/crosschain.go, there's already validation for negative quote IDs:
if args.QuoteId.Sign() < 0 {
return errors.New("quoteId cannot be negative")
}The validation is handled at multiple layers:
- Contract layer validates non-negative values
- Keeper layer validates quote existence and timeout
- Quote validation is part of the bridge call flow
No additional validation needed in the contract
The quote validation is already properly implemented in the system through multiple layers. The contract doesn't need additional validation as it's handled by the keeper layer which checks for quote existence, validity, and timeout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for quote validation patterns
rg -A 5 "quoteId|quote.*valid"
Length of output: 47823
x/crosschain/mock/expected_keepers_mocks.go (3)
497-509: LGTM: ReSetCache mock implementation
The implementation correctly follows the mock pattern and provides the necessary functionality for cache management.
794-807: LGTM: GetQuoteById mock implementation
The implementation properly supports the quote ID functionality required for bridge calls and follows the standard mock pattern.
824-860: LGTM: MockERC20TokenKeeper implementation
The new mock interface correctly implements the ERC20 token transfer functionality and follows the established mock patterns.
contract/icrosschain.sol.go (4)
34-34: LGTM: ABI updated correctly for quoteId.
The ABI string has been properly updated to include the new _quoteId parameter in the BridgeCallEvent signature.
278-294: LGTM: BridgeCall method signatures consistently updated.
All three variants of the BridgeCall method (Transactor, Session, TransactorSession) have been properly updated to use _quoteId *big.Int parameter.
417-421: LGTM: Event struct properly updated.
The ICrosschainBridgeCallEvent struct has been correctly updated to include the QuoteId field of type *big.Int.
Line range hint 422-499: Verify the event signature hash consistency.
The event filtering methods have been updated with the new signature. Let's verify that the event signature hash (0x96da1d63...) is correctly calculated for the new event signature.
✅ Verification successful
Event signature hash is correctly implemented
The event signature hash (0x96da1d63...) in the generated Go code matches the Solidity event definition in ICrosschain.sol. The test file validates that:
- The event signature is
BridgeCallEvent(address,address,address,address,uint256,string,address[],uint256[],bytes,uint256,bytes) - The event has 11 parameters with the first 3 being indexed
- The computed hash matches 0x96da1d63da4d424eb5848fc98c0361f8e970b1934ddc9c5529ba684171283a53
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the event signature hash matches the updated event definition
# Expected: The calculated hash should match 0x96da1d63da4d424eb5848fc98c0361f8e970b1934ddc9c5529ba684171283a53
# Search for other occurrences of this event signature hash to ensure consistency
rg -l "96da1d63da4d424eb5848fc98c0361f8e970b1934ddc9c5529ba684171283a53"
Length of output: 144
Script:
#!/bin/bash
# Let's examine the event signature in both files to verify consistency
rg -A 5 "96da1d63da4d424eb5848fc98c0361f8e970b1934ddc9c5529ba684171283a53" x/crosschain/precompile/bridge_call_test.go
echo "---"
# Also check the event definition in the test file
ast-grep --pattern 'event BridgeCallEvent($$$)'
Length of output: 898
Script:
#!/bin/bash
# Let's check the test file more thoroughly to see the event definition and validation
rg -B 5 -A 10 "bridgeCallABI" x/crosschain/precompile/bridge_call_test.go
echo "---"
# Also check for any Solidity contract files that might contain the original event definition
fd -e sol .
Length of output: 6163
Script:
#!/bin/bash
# Let's check the original event definition in the Solidity contract
cat solidity/contracts/bridge/ICrosschain.sol
echo "---"
# Also check if there are any other implementations of this interface
rg -l "interface ICrosschain"
Length of output: 1914
contract/ifx_bridge_logic.sol.go (2)
Line range hint 1-1: Note: This is an auto-generated file
This file is automatically generated from the Solidity contract ABI. Manual changes should not be made directly to this file.
749-751: LGTM: BridgeCall function signature updates are consistent
The bridgeCall function signatures have been consistently updated across all three implementations (Transactor, Session, and TransactorSession) to replace _value with _quoteId. This change aligns with the PR objectives to add quote ID support in bridge calls.
Let's verify that all references to the bridgeCall function use the new parameter name:
Also applies to: 756-758, 763-765
✅ Verification successful
Based on the extensive search results, I can now verify the review comment. The codebase shows consistent usage of the bridgeCall function across multiple implementations including:
- The contract interfaces (IBridgeCall.sol, ICrosschain.sol) show the function signature matches
- The implementations in FxBridgeLogic.sol and FxBridgeLogicETH.sol have consistent parameter names
- The generated Go bindings (ifx_bridge_logic.sol.go) reflect the same parameter names
- The precompile and test files use consistent parameter naming
The search results confirm that:
- The
_valueparameter has been replaced with_quoteIdacross all implementations - The function signatures are consistent in all three implementations (Transactor, Session, TransactorSession)
- The parameter names and types match exactly as mentioned in the review comment
The bridgeCall function signatures have been correctly and consistently updated across the codebase
The review comment is accurate in its assessment that the changes are consistent and properly aligned with the PR objectives to add quote ID support in bridge calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to '_value' in bridgeCall functions
rg -A 2 'bridgeCall.*_value'
Length of output: 30
Script:
#!/bin/bash
# Let's search for all bridgeCall function implementations and references
rg -A 5 "bridgeCall|BridgeCall" --type go
Length of output: 504330
api/fx/gravity/crosschain/v1/tx.pulsar.go (5)
7969-7969: Field descriptor for quote_id properly defined.
The field descriptor and initialization for quote_id follow the standard protobuf field naming conventions and initialization patterns.
Also applies to: 7987-7987
8117-8119: Field handling logic properly implemented.
The quote_id field handling implementation follows protobuf's standard patterns for string fields, including:
- Proper empty string checks
- Consistent get/set operations
- Appropriate error handling
Also applies to: 8170-8171, 8212-8213, 8270-8271, 8323-8324, 8377-8378
17371-17371: Message struct properly updated with quote_id field.
The QuoteId field is correctly added to the MsgBridgeCallClaim struct with:
- Proper protobuf field numbering (11)
- Correct json field naming (quote_id)
- Standard Go getter method implementation
Also applies to: 17466-17468
18659-18673: Service definitions and metadata properly maintained.
The protobuf service definitions and package metadata are correctly maintained with the addition of the quote_id field, ensuring backward compatibility.
Line range hint 7969-18673: Verify complete quote_id integration.
The implementation of quote_id support appears complete and consistent. Let's verify there are no missing references.
✅ Verification successful
Based on the search results, I can see that the quote_id integration is thoroughly implemented across the codebase:
- Protocol buffers and generated code:
- Properly defined in message types
- Included in all relevant message structures
- Generated code handles serialization/deserialization
- Contract interfaces:
- Present in bridge call function signatures
- Included in event definitions
- Properly typed as uint256
- Implementation:
- Handled in bridge call flows
- Included in checkpoints and verification
- Stored and retrieved appropriately
- No TODO comments or missing implementations found
The implementation appears complete and consistent across all layers - protocol definitions, generated code, contract interfaces, and core logic.
The quote_id field is properly integrated and verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify quote_id field integration is complete
# Test: Search for any potential missing quote_id references
echo "Checking for potential missing quote_id references..."
rg -i "quote.?id" --type go
echo "Checking for any TODO comments related to quote handling..."
rg -i "todo.*quote" --type go
Length of output: 35057
x/crosschain/keeper/bridge_call_out.go (6)
361-392: Ensure error handling for invalid or timed-out quotes.
The new handleBridgeCallQuote method appropriately checks for invalid or timed-out quotes and returns an error in those cases. This helps prevent processing bridge calls with invalid quotes.
379-388: Verify the transfer of quote fees based on the token type.
The method correctly distinguishes between origin and ERC20 tokens when transferring the quote fee to the module account. For origin tokens, it uses the bankKeeper to send coins, while for ERC20 tokens, it uses the erc20TokenKeeper to transfer tokens.
394-423: Ensure the quote fee is transferred to the relayer after processing.
The new TransferQuoteFeeToRelayer method is called after processing the bridge call result. It retrieves the stored quoteId, deletes it to prevent reuse, and transfers the fee to the relayer based on the token type. This ensures that the relayer receives the fee as expected.
425-439: LGTM: Quote ID management for outgoing bridge calls.
The new methods SetOutgoingBridgeCallQuoteId, GetOutgoingBridgeCallQuoteId, and DeleteOutgoingBridgeCallQuoteId handle the storage and retrieval of quote IDs for outgoing bridge calls. The implementation looks correct and follows the expected logic.
441-461: Ensure the ResendBridgeCall method updates the quote ID correctly.
The ResendBridgeCall method allows resending a bridge call with a new nonce. It appropriately deletes the old quote ID, sets the new quote ID, and emits an event with the old and new nonce. This ensures that the quote ID is updated correctly when resending a bridge call.
Line range hint 268-298: Verify the handling of the new quoteId parameter in the codebase.
The BridgeCallBaseCoin method now includes a new quoteId parameter and calls the handleBridgeCallQuote method to process the quote. This change looks good, but it's important to ensure that all callers of this method have been updated to provide the quoteId argument.
Run the following script to verify the usage of BridgeCallBaseCoin:
✅ Verification successful
Let me analyze the search results to verify the handling of the quoteId parameter.
The search results show:
- The interface definition in
expected_keepers.goincludes thequoteIdparameter - The implementation in
bridge_call_out.gomatches the interface - The only caller of this method is in
precompile/bridge_call.gowhich correctly provides thequoteIdargument throughargs.QuoteId
Let me verify if there are any other potential callers using a different search pattern.
Based on the previous and current search results, I can generate the final response:
✅ The quoteId parameter is properly handled throughout the codebase
The verification confirms that:
- The only caller of
BridgeCallBaseCoinis inx/crosschain/precompile/bridge_call.go - This caller correctly provides the
quoteIdparameter viaargs.QuoteId - No other callers were found in the codebase, including test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to `BridgeCallBaseCoin` provide the `quoteId` argument.
# Test: Search for calls to `BridgeCallBaseCoin`. Expect: Only calls with `quoteId` argument.
rg -A 5 $'BridgeCallBaseCoin'
Length of output: 1995
Script:
#!/bin/bash
# Search for any other potential callers using ast-grep
ast-grep --pattern 'BridgeCallBaseCoin($$$)'
# Also search for test files that might call this method
rg -g '**/*_test.go' 'BridgeCallBaseCoin'
Length of output: 88
Co-authored-by: fx0x55 <80245546+fx0x55@users.noreply.github.com>
Summary by CodeRabbit
New Features
QuoteIdfor bridge call claims, enhancing data structure and validation.Bug Fixes
Documentation
Tests
QuoteIdstructure and validation logic.