-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add query interface BridgeCallQuoteByNonce and BridgeCallsByFeeReceiver #818
Conversation
WalkthroughThe pull request introduces significant updates to the API definitions in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
docs/swagger-ui/swagger.yaml (2)
42573-42659
: Enhance response type documentationWhile the response types are well-structured, consider adding more descriptive documentation for the following fields:
QuoteInfo.fee
: Specify the fee format and unitsQuoteInfo.gas_limit
: Explain how this value is determinedQuoteInfo.expiry
: Document when and how the expiry is calculatedAlso applies to: 43494-43511
Line range hint
6529-9446
: Consider adding filtering capabilities to bridge call endpointsThe current API design could be enhanced by adding filtering options to improve query flexibility. Consider adding the following query parameters:
status
: Filter bridge calls by their current status (e.g., pending, completed, failed)time_range
: Allow filtering by creation time or block height rangetoken_type
: Filter by specific token typesThis would improve the API's usability for clients needing to fetch specific subsets of bridge calls.
x/crosschain/keeper/grpc_query_test.go (1)
46-87
: Enhance test coverageWhile the basic test cases are implemented, consider adding:
- Tests for invalid fee receiver address
- Validation of the actual returned bridge call data
- Test cases for empty results
- Error cases (e.g., invalid chain name)
x/crosschain/keeper/grpc_query.go (1)
457-486
: Consider optimizing the pagination filterThe current implementation might be inefficient when many records don't match the fee receiver criteria, as it processes and skips them during pagination. Consider:
- Using a dedicated index for fee receiver lookups
- Or storing bridge calls in a separate collection indexed by fee receiver
Would you like me to propose a specific implementation for either approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/fx/gravity/crosschain/v1/query_grpc.pb.go
is excluded by!**/*.pb.go
x/crosschain/types/query.pb.go
is excluded by!**/*.pb.go
x/crosschain/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
📒 Files selected for processing (6)
docs/swagger-ui/swagger.yaml
(13 hunks)docs/swagger_test.go
(1 hunks)proto/fx/gravity/crosschain/v1/query.proto
(2 hunks)x/crosschain/keeper/grpc_query.go
(2 hunks)x/crosschain/keeper/grpc_query_router.go
(1 hunks)x/crosschain/keeper/grpc_query_test.go
(1 hunks)
🔇 Additional comments (7)
docs/swagger-ui/swagger.yaml (1)
7095-7170
: LGTM! Well-structured endpoint with proper pagination
The endpoint is well-designed with:
- Comprehensive bridge call response structure
- Standard pagination implementation
- Clear parameter definitions
x/crosschain/keeper/grpc_query_router.go (1)
270-276
: Implementation follows established patterns
The new query methods are well-implemented and consistent with the existing codebase:
- Proper error handling through
getQueryServerByChainName
- Clean forwarding to chain-specific implementations
- Consistent method signatures
Also applies to: 278-284
docs/swagger_test.go (1)
61-61
: LGTM: Handler count updated correctly
The assertion update correctly reflects the addition of two new GET endpoints for bridge call queries.
proto/fx/gravity/crosschain/v1/query.proto (3)
147-156
: LGTM: New RPC methods are well-defined
The new RPC methods follow the existing patterns and conventions, with appropriate HTTP endpoint mappings.
365-370
: LGTM: Message definitions are clear and focused
The request and response messages for BridgeCallQuoteByNonce are well-structured with appropriate field types.
372-382
: LGTM: Paginated query messages are well-defined
The request and response messages for BridgeCallsByFeeReceiver properly implement pagination following Cosmos SDK patterns.
x/crosschain/keeper/grpc_query.go (1)
446-455
: LGTM: Implementation handles errors appropriately
The BridgeCallQuoteByNonce implementation follows best practices:
- Validates input
- Uses appropriate gRPC status codes
- Properly wraps/unwraps SDK context
/fx/crosschain/v1/bridge_call_quote_by_nonce: | ||
get: | ||
operationId: BridgeCalls | ||
operationId: BridgeCallQuoteByNonce | ||
responses: | ||
'200': | ||
description: A successful response. | ||
schema: | ||
type: object | ||
properties: | ||
bridge_calls: | ||
type: array | ||
items: | ||
type: object | ||
properties: | ||
sender: | ||
type: string | ||
refund: | ||
type: string | ||
tokens: | ||
type: array | ||
items: | ||
type: object | ||
properties: | ||
contract: | ||
type: string | ||
amount: | ||
type: string | ||
description: >- | ||
ERC20Token unique identifier for an Ethereum ERC20 | ||
token. | ||
to: | ||
type: string | ||
data: | ||
type: string | ||
memo: | ||
type: string | ||
nonce: | ||
type: string | ||
format: uint64 | ||
timeout: | ||
type: string | ||
format: uint64 | ||
block_height: | ||
type: string | ||
format: uint64 | ||
event_nonce: | ||
type: string | ||
format: uint64 | ||
pagination: | ||
quote: | ||
type: object | ||
properties: | ||
next_key: | ||
id: | ||
type: string | ||
format: byte | ||
description: |- | ||
next_key is the key to be passed to PageRequest.key to | ||
query the next page most efficiently. It will be empty if | ||
there are no more results. | ||
total: | ||
format: uint64 | ||
token: | ||
type: string | ||
fee: | ||
type: string | ||
oracle: | ||
type: string | ||
gas_limit: | ||
type: string | ||
format: uint64 | ||
expiry: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add missing nonce parameter to the endpoint
The endpoint /bridge_call_quote_by_nonce
is missing the required nonce
query parameter, which is necessary to fetch the quote for a specific bridge call.
Add the following parameter definition:
/fx/crosschain/v1/bridge_call_quote_by_nonce:
get:
operationId: BridgeCallQuoteByNonce
+ parameters:
+ - name: nonce
+ in: query
+ required: true
+ type: string
+ format: uint64
+ description: The nonce of the bridge call to fetch the quote for
responses:
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
/fx/crosschain/v1/bridge_call_quote_by_nonce: | |
get: | |
operationId: BridgeCalls | |
operationId: BridgeCallQuoteByNonce | |
responses: | |
'200': | |
description: A successful response. | |
schema: | |
type: object | |
properties: | |
bridge_calls: | |
type: array | |
items: | |
type: object | |
properties: | |
sender: | |
type: string | |
refund: | |
type: string | |
tokens: | |
type: array | |
items: | |
type: object | |
properties: | |
contract: | |
type: string | |
amount: | |
type: string | |
description: >- | |
ERC20Token unique identifier for an Ethereum ERC20 | |
token. | |
to: | |
type: string | |
data: | |
type: string | |
memo: | |
type: string | |
nonce: | |
type: string | |
format: uint64 | |
timeout: | |
type: string | |
format: uint64 | |
block_height: | |
type: string | |
format: uint64 | |
event_nonce: | |
type: string | |
format: uint64 | |
pagination: | |
quote: | |
type: object | |
properties: | |
next_key: | |
id: | |
type: string | |
format: byte | |
description: |- | |
next_key is the key to be passed to PageRequest.key to | |
query the next page most efficiently. It will be empty if | |
there are no more results. | |
total: | |
format: uint64 | |
token: | |
type: string | |
fee: | |
type: string | |
oracle: | |
type: string | |
gas_limit: | |
type: string | |
format: uint64 | |
expiry: | |
/fx/crosschain/v1/bridge_call_quote_by_nonce: | |
get: | |
operationId: BridgeCallQuoteByNonce | |
parameters: | |
- name: nonce | |
in: query | |
required: true | |
type: string | |
format: uint64 | |
description: The nonce of the bridge call to fetch the quote for | |
responses: |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation