refactor(x/crosschain): save relationship between bridgeCall and quoteInfo #812
Conversation
WalkthroughThe changes in this pull request introduce a new message type, Changes
Possibly related PRs
Warning Rate limit exceeded@zakir-code has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
proto/fx/gravity/crosschain/v1/types.proto (1)
209-220: LGTM! Consider adding field documentation.The
QuoteInfomessage structure is well-designed with appropriate field types and protobuf options. Thefeefield correctly usescosmos.Intfor handling large numbers with non-nullable constraint.Consider adding comments to document the purpose and constraints of each field. Example:
message QuoteInfo { + // Unique identifier for the quote uint64 id = 1; + // Token contract address or identifier string token = 2; + // Fee amount in cosmos.Int format string fee = 3 [ (cosmos_proto.scalar) = "cosmos.Int", (gogoproto.customtype) = "cosmossdk.io/math.Int", (gogoproto.nullable) = false ]; + // Oracle address that provided the quote string oracle = 4; + // Maximum gas limit for the bridge call uint64 gas_limit = 5; + // Block height or timestamp when the quote expires uint64 expiry = 6; }x/crosschain/keeper/abci.go (1)
171-182: Consider adding logging for bridge call cleanup actionsThe cleanup logic handles two cases (refund and resend) but doesn't log these important state transitions. Consider adding debug logs to help with monitoring and debugging.
quoteInfo, found := k.GetOutgoingBridgeCallQuoteInfo(ctx, data.Nonce) if !found { + k.Logger(ctx).Debug("cleaning up timed out bridge call - initiating refund", "nonce", data.Nonce) // 1. handler bridge call refund if err = k.RefundOutgoingBridgeCall(ctx, data); err != nil { return true } } else { + k.Logger(ctx).Debug("cleaning up timed out bridge call - resending", "nonce", data.Nonce) // 1. resend bridge call if err = k.ResendBridgeCall(ctx, *data, quoteInfo); err != nil { return true } }x/crosschain/keeper/bridge_call_out.go (2)
422-443: Add input validation and documentationThe QuoteInfo methods would benefit from:
- Input validation for nonce
- Godoc comments explaining the methods' purpose and parameters
Apply this diff:
+// SetOutgoingBridgeCallQuoteInfo stores the quote information for a bridge call +// nonce must be greater than 0 func (k Keeper) SetOutgoingBridgeCallQuoteInfo(ctx sdk.Context, nonce uint64, quoteInfo types.QuoteInfo) { + if nonce == 0 { + panic("nonce cannot be zero") + } store := ctx.KVStore(k.storeKey) store.Set(types.GetBridgeCallQuoteKey(nonce), k.cdc.MustMarshal("eInfo)) } +// GetOutgoingBridgeCallQuoteInfo retrieves the quote information for a bridge call +// returns the quote info and whether it was found func (k Keeper) GetOutgoingBridgeCallQuoteInfo(ctx sdk.Context, nonce uint64) (types.QuoteInfo, bool) { + if nonce == 0 { + return types.QuoteInfo{}, false + } store := ctx.KVStore(k.storeKey) bz := store.Get(types.GetBridgeCallQuoteKey(nonce)) if bz == nil { return types.QuoteInfo{}, false } quoteInfo := types.QuoteInfo{} k.cdc.MustUnmarshal(bz, "eInfo) return quoteInfo, true } +// DeleteOutgoingBridgeCallQuoteInfo removes the quote information for a bridge call func (k Keeper) DeleteOutgoingBridgeCallQuoteInfo(ctx sdk.Context, nonce uint64) { + if nonce == 0 { + return + } store := ctx.KVStore(k.storeKey) store.Delete(types.GetBridgeCallQuoteKey(nonce)) }
Line range hint
445-466: Add validations and handle edge casesThe function needs improvements in several areas:
- Input validation for bridgeCall and quoteInfo
- Proper error handling for timeout calculation
- Transaction safety for quote info transfer
Apply this diff:
func (k Keeper) ResendBridgeCall(ctx sdk.Context, bridgeCall types.OutgoingBridgeCall, quoteInfo types.QuoteInfo) error { + if bridgeCall.Nonce == 0 { + return types.ErrInvalid.Wrap("invalid bridge call nonce") + } + if quoteInfo.Fee.IsNil() { + return types.ErrInvalid.Wrap("invalid quote info") + } bridgeCallTimeout := k.CalExternalTimeoutHeight(ctx, GetBridgeCallTimeout) if bridgeCallTimeout <= 0 { return types.ErrInvalid.Wrapf("bridge call timeout height") } oldBridgeCallNonce := bridgeCall.Nonce - k.DeleteOutgoingBridgeCallQuoteInfo(ctx, oldBridgeCallNonce) newBridgeCallNonce := k.autoIncrementID(ctx, types.KeyLastBridgeCallID) bridgeCall.Nonce = newBridgeCallNonce bridgeCall.Timeout = bridgeCallTimeout + // Set new quote info before deleting old one to maintain data consistency + k.SetOutgoingBridgeCallQuoteInfo(ctx, newBridgeCallNonce, quoteInfo) k.SetOutgoingBridgeCall(ctx, &bridgeCall) - k.SetOutgoingBridgeCallQuoteInfo(ctx, newBridgeCallNonce, quoteInfo) + k.DeleteOutgoingBridgeCallQuoteInfo(ctx, oldBridgeCallNonce) ctx.EventManager().EmitEvent(sdk.NewEvent(types.EventTypeBridgeCallResend, sdk.NewAttribute(types.AttributeKeyBridgeCallResendOldNonce, fmt.Sprintf("%d", oldBridgeCallNonce)), sdk.NewAttribute(types.AttributeKeyBridgeCallResendNewNonce, fmt.Sprintf("%d", newBridgeCallNonce))), ) return k.erc20Keeper.ReSetCache(ctx, types.NewOriginTokenKey(k.moduleName, oldBridgeCallNonce), types.NewOriginTokenKey(k.moduleName, newBridgeCallNonce)) }api/fx/gravity/crosschain/v1/types.pulsar.go (2)
9258-9264: Consider adding field validation and documentation.The
QuoteInfostruct could benefit from:
- Input validation for critical fields:
feeshould be a valid cosmos-sdk Intoracleshould be a valid addressexpiryshould be a future timestamp- Documentation explaining:
- The purpose of each field
- Valid value ranges
- Relationships between fields
9253-9264: Consider enhancing the quote information model.The
QuoteInfomessage provides a good foundation for storing bridge call quotes. Consider these architectural improvements:
- Add version field for future compatibility
- Consider using a more precise type for fees (e.g., fixed-point decimal)
- Add status field to track quote lifecycle
- Consider adding audit fields (created_at, updated_at)
Also applies to: 9511-9523
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/crosschain/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
api/fx/gravity/crosschain/v1/types.pulsar.go(6 hunks)proto/fx/gravity/crosschain/v1/types.proto(1 hunks)x/crosschain/keeper/abci.go(1 hunks)x/crosschain/keeper/bridge_call_out.go(1 hunks)x/crosschain/types/types.go(1 hunks)
🔇 Additional comments (7)
proto/fx/gravity/crosschain/v1/types.proto (1)
Line range hint 182-207: Verify the relationship between OutgoingBridgeCall and QuoteInfo.
The PR objective mentions saving the relationship between bridgeCall and quoteInfo, but there's no direct reference between these messages in the proto definitions.
Let's verify how this relationship is implemented:
Also applies to: 209-220
✅ Verification successful
Let me gather more information about the relationship implementation by checking the key and type definitions.
Based on the gathered information, I can now provide the final response:
The relationship between OutgoingBridgeCall and QuoteInfo is correctly implemented through a KV store mapping.
The relationship is maintained through:
- A one-to-one mapping between bridge call nonce and QuoteInfo in the KV store
- CRUD operations in the keeper:
SetOutgoingBridgeCallQuoteInfo: Creates/updates the mappingGetOutgoingBridgeCallQuoteInfo: Retrieves QuoteInfo by bridge call nonceDeleteOutgoingBridgeCallQuoteInfo: Removes the mapping
- The relationship is properly handled during bridge call lifecycle (creation, resend, refund)
No direct reference in the proto definition is needed as the relationship is managed at the storage level using the bridge call nonce as the key.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how QuoteInfo is referenced from OutgoingBridgeCall in the codebase
# Search for files that might contain the relationship implementation
echo "Files that might contain the relationship implementation:"
fd -e go -e proto | rg -l "OutgoingBridgeCall|QuoteInfo"
# Search for code that links these types
echo -e "\nCode that links OutgoingBridgeCall and QuoteInfo:"
rg -A 5 "OutgoingBridgeCall.*QuoteInfo|QuoteInfo.*OutgoingBridgeCall"
Length of output: 4731
Script:
#!/bin/bash
# Check the key definition and type implementations
echo "Checking key definition for bridge call quote:"
rg "GetBridgeCallQuoteKey|KeyLastBridgeCallID" -A 3
echo -e "\nChecking QuoteInfo type definition:"
rg "type QuoteInfo struct" -A 5
Length of output: 3572
x/crosschain/keeper/abci.go (3)
171-172: Verify error handling for GetOutgoingBridgeCallQuoteInfo
The code correctly uses the tuple return pattern (value, found) for checking quote info existence. However, we should verify that the GetOutgoingBridgeCallQuoteInfo method properly handles all error cases.
#!/bin/bash
# Search for the implementation and error handling in GetOutgoingBridgeCallQuoteInfo
ast-grep --pattern 'func $_ GetOutgoingBridgeCallQuoteInfo($_) ($_, $_) {
$$$
}'Line range hint 171-189: Verify the atomicity of cleanup operations
The code performs multiple state changes (refund/resend followed by deletion). We should ensure that these operations are atomic and that the state remains consistent if any operation fails.
#!/bin/bash
# Search for any existing transaction handling or recovery mechanisms
rg -A 5 'BeginBlock|EndBlock|SavePoint|Commit' 179-179: Verify the updated ResendBridgeCall signature
The ResendBridgeCall method has been updated to accept quoteInfo as a parameter. Let's verify its implementation to ensure proper handling of the new parameter.
✅ Verification successful
ResendBridgeCall implementation correctly matches the updated signature
The method signature has been properly updated to accept three parameters: ctx sdk.Context, bridgeCall types.OutgoingBridgeCall, and quoteInfo types.QuoteInfo. The implementation shows proper handling of bridge call resending with nonce management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the ResendBridgeCall implementation
ast-grep --pattern 'func $_ ResendBridgeCall($_, $_, $_) error {
$$$
}'
Length of output: 249
Script:
#!/bin/bash
# Search for the ResendBridgeCall implementation with rg
rg -U "func.*ResendBridgeCall.*\{" -A 10
Length of output: 979
x/crosschain/types/types.go (1)
522-524: LGTM!
The method safely converts the Oracle hex string to an Ethereum address using the appropriate Ethereum type.
api/fx/gravity/crosschain/v1/types.pulsar.go (2)
7689-8379: LGTM: Generated reflection code is complete and correct.
The protobuf reflection implementation is properly generated with comprehensive error handling and proper edge case management.
9511-9523: Verify protobuf generation is up-to-date.
The protobuf metadata has been updated to include the new QuoteInfo message. Please ensure:
- The protobuf generation is reproducible
- The proto file changes are committed
Also applies to: 9569-9569, 9583-9585
✅ Verification successful
Let me gather more information about the protobuf generation setup to verify if everything is properly configured.
Based on the gathered information, I can now provide the final response:
Protobuf generation setup is correctly configured and up-to-date
The verification shows:
- The
QuoteInfomessage is properly defined inproto/fx/gravity/crosschain/v1/types.proto - The protobuf generation setup is properly configured with
protocgen.shandprotocgen-pulsar.sh - No uncommitted changes exist in proto files
- The generated code matches the proto definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that protobuf generation is up-to-date
# Find and check the proto file
fd -e proto | xargs grep -l "QuoteInfo"
# Verify protobuf generation command in Makefile
fd Makefile | xargs grep -A 5 "proto-gen"
Length of output: 629
Script:
#!/bin/bash
# Check the content of protocgen.sh and proto file
cat scripts/protocgen.sh
cat proto/fx/gravity/crosschain/v1/types.proto
# Check if there are any uncommitted changes in proto files
git status --porcelain "**/*.proto"
Length of output: 8393
Summary by CodeRabbit
New Features
QuoteInfofor enhanced quote handling in cross-chain transactions.QuoteInfoobjects, including converting theOraclefield to an Ethereum address.Bug Fixes
quoteInfobefore processing.Refactor
These changes streamline cross-chain operations and enhance user experience with improved quote management.