Add missing queries CLI #1233
Add missing queries CLI #1233
Conversation
WalkthroughThe changes revolve around enhancing the interchain staking functionality by refining and expanding the query capabilities. They include modifications to how withdrawal and redelegation records are retrieved and tested, alongside introducing new CLI commands for a comprehensive range of queries. This update aims to fill the gaps in CLI query functionalities, particularly for unbonding and claims, ensuring users have access to necessary information through command-line interfaces. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1233 +/- ##
==========================================
+ Coverage 63.35% 63.48% +0.13%
==========================================
Files 171 171
Lines 11313 11536 +223
==========================================
+ Hits 7167 7324 +157
- Misses 3420 3469 +49
- Partials 726 743 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…uan/add-query-cli
|
Hi @joe-bowman, I noticed that Given this, I was wondering if you think it might be worth refactoring these functions before I start working on issue #1233. |
…ksilver into tuan/add-query-cli
tuantran1702
requested review from
joe-bowman,
faddat and
minhngoc274
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/interchainstaking/client/cli/query.go (2 hunks)
- x/interchainstaking/keeper/grpc_query.go (3 hunks)
- x/interchainstaking/keeper/redelegation_record.go (1 hunks)
Additional comments: 4
x/interchainstaking/keeper/redelegation_record.go (1)
- 69-69: The modification to the
AllRedelegationRecordsfunction aligns with the PR's objective of removing the zone specification, making the function more generic and potentially simplifying the codebase. This change seems appropriate and does not introduce any apparent issues.x/interchainstaking/keeper/grpc_query.go (2)
- 247-252: The changes to the
ZoneWithdrawalRecordsfunction, which now utilizesAllZoneWithdrawalRecordsfor retrieving withdrawal records, simplifies the codebase and aligns with the PR's objectives. Ensure thatAllZoneWithdrawalRecordscorrectly handles zone specifications internally.- 265-265: The modification to the
WithdrawalRecordsfunction, which now directly utilizesAllWithdrawalRecords, simplifies the retrieval process and aligns with the PR's objectives of codebase simplification.x/interchainstaking/client/cli/query.go (1)
- 33-41: The addition of new query commands for withdrawal records, user withdrawal records, zone withdrawal records, unbonding records, receipts, transaction status, zone redelegation records, zone validators, and zone information enhances the CLI's capabilities and aligns with the PR's objectives. The commands are well-structured and follow the conventions used in the rest of the file.
|
I like the idea of combining the 2 lookalike functions, just please make sure they are exactly the same |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/client/cli/cli_test.go (3 hunks)
- x/interchainstaking/client/cli/query.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/interchainstaking/client/cli/cli_test.go
- x/interchainstaking/client/cli/query.go
| @@ -66,6 +67,8 @@ func (s *IntegrationTestSuite) SetupSuite() { | |||
| MessagesPerTx: 0, | |||
| Is_118: true, | |||
| } | |||
|
|
|||
| // TODO: I think setting validators here isn't enough, we need to set them in the store by using the keeper | |||
There was a problem hiding this comment.
Correct; we used to keep them in the zone struct but it was horribly inefficient loading 500 vals every time we wanted to fetch the denom from a zone for example.
I don't think this is correct. ZoneWithdrawalRecords also scopes by chainId (perhaps ought to be called UserZoneWithdrawlRecords) whereas UserWithdrawalRecords doesn't. Would be good to see where these are used though? |
Yeah but still the name is kind of misleading. These aren't used any where beside grpc tests, so I think it's safe to refactor. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/keeper/grpc_query.go (3 hunks)
- x/interchainstaking/keeper/grpc_query_test.go (1 hunks)
Check Runs (1)
devskim success (4)
- x/interchainstaking/keeper/grpc_query.go: 271-271: Suspicious comment
- x/interchainstaking/keeper/grpc_query.go: 324-324: Suspicious comment
Additional comments: 2
x/interchainstaking/keeper/grpc_query.go (2)
- 247-252: The change from retrieving a specific zone to using
AllZoneWithdrawalRecordsfor the entire zone inZoneWithdrawalRecordsfunction simplifies the logic and potentially improves performance by reducing the need for multiple calls. However, ensure that this change aligns with the intended use cases and that all callers of this function are aware of the change in behavior.- 265-267: Changing the parameter in
AllWithdrawalRecordsfromreq.ChainIdtoctxaligns with the standard practice of passing context objects in Go for maintaining request-scoped values, deadlines, and cancellation signals. This change is appropriate and improves the function's flexibility and adherence to Go's context passing conventions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/grpc_query.go (2 hunks)
Additional comments: 2
x/interchainstaking/keeper/grpc_query.go (2)
- 247-252: The change from retrieving a specific zone to using
AllZoneWithdrawalRecordsinZoneWithdrawalRecordsfunction simplifies the logic and potentially improves performance by avoiding unnecessary checks for zone existence. However, ensure that this change aligns with the intended functionality and that all callers of this function are aware of the change in behavior.- 265-267: The change in
WithdrawalRecordsto useAllWithdrawalRecords(ctx)instead of filtering byreq.ChainIdsimplifies the function and potentially broadens its use case. Ensure that this aligns with the intended use cases and that any necessary filtering by chain ID is handled elsewhere if needed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/keeper/grpc_query.go (2 hunks)
- x/interchainstaking/keeper/grpc_query_test.go (1 hunks)
Check Runs (1)
test quicksilver (amd64, linux) null (5)
- x/interchainstaking/keeper/grpc_query_test.go: [failure] 1456-1456:
cannot use 10000000 (untyped int constant) as "cosmossdk.io/math".Int value in struct literal - x/interchainstaking/keeper/grpc_query_test.go: [failure] 1460-1460:
cannot use 20000000 (untyped int constant) as "cosmossdk.io/math".Int value in struct literal - x/interchainstaking/keeper/grpc_query_test.go: [failure] 1494-1494:
cannot use 10000000 (untyped int constant) as "cosmossdk.io/math".Int value in struct literal - x/interchainstaking/keeper/grpc_query_test.go: [failure] 1498-1498:
cannot use 20000000 (untyped int constant) as "cosmossdk.io/math".Int value in struct literal
Additional comments: 2
x/interchainstaking/keeper/grpc_query.go (2)
- 244-249: The change in
ZoneWithdrawalRecordsto useAllZoneWithdrawalRecordsfor data retrieval simplifies the function and potentially improves maintainability by centralizing the logic for withdrawal record retrieval. However, ensure thatAllZoneWithdrawalRecordsefficiently handles the data retrieval, especially considering large datasets, to avoid performance issues.- 262-265: The modification in
WithdrawalRecordsto useAllWithdrawalRecordsinstead of filtering byreq.ChainIdbroadens the scope of data retrieval. This change should be carefully evaluated to ensure it aligns with the intended use cases and does not inadvertently expose more data than expected to the client.
| distributions := []*types.Distribution{ | ||
| { | ||
| Valoper: icsKeeper.GetValidators(ctx, suite.chainB.ChainID)[0].ValoperAddress, | ||
| Amount: 10000000, |
There was a problem hiding this comment.
The integer constants used for the Amount field in types.Distribution structs need to be converted to math.Int type to match the expected type. Here's how you can fix it:
- Amount: 10000000,
+ Amount: math.NewInt(10000000),And similarly for the other instances.
Also applies to: 1460-1460, 1494-1494, 1498-1498
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.
| Amount: 10000000, | |
| Amount: math.NewInt(10000000), |
There was a problem hiding this comment.
@tropicaldog - this is correct as of main. Amount is Distribution changed to math.Int in #1319
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Ignore this; it's outdated. Appears fixed in the latest commit
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/interchainstaking/client/cli/cli_test.go (6 hunks)
- x/interchainstaking/keeper/grpc_query_test.go (1 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/interchainstaking/keeper/ibc_packet_handlers.go
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/grpc_query_test.go
Additional comments: 2
x/interchainstaking/client/cli/cli_test.go (2)
- 23-23: The constant
testDelegatoris well-defined and follows Go naming conventions.- 230-250: The test case labeled "valid" in
TestGetDelegatorIntentCmdis well-structured and tests the expected behavior. Ensure that the test data reflects realistic scenarios for more comprehensive testing.
1. Summary
Fixes #1145
2.Type of change
3. Implementation details
4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)
Summary by CodeRabbit
New Features
Improvements
Tests