feat(eibc): Expand list-demand-orders query with more parameters #851
Conversation
zale144
force-pushed
the
zale144/849-list-demand-orders-filters
branch
from
43fc95c
to
1e4569d
Compare
zale144
changed the base branch from
main
to
zale144/850-query-delayedack-by-type
zale144
changed the title
omritoptix
changed the title
| if err != nil { | ||
| return fmt.Errorf("limit must be an integer: %s", args[3]) | ||
| } | ||
| request.Limit = int32(limit) |
There was a problem hiding this comment.
Any reason why we use limit instead of Pagination in cosmos-sdk?
There was a problem hiding this comment.
well, the intent of this added filtering is just for debugging, it's not supposed to be for end users
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
| @@ -26,6 +26,7 @@ func PendingByRollappIDByMaxHeight( | |||
| End: commontypes.RollappPacketByStatusByRollappIDByProofHeightPrefix(rollappID, status, maxProofHeight+1), // inclusive end | |||
| }, | |||
| }, | |||
| FilterFunc: func(packet commontypes.RollappPacket) bool { return true }, | |||
There was a problem hiding this comment.
should define it (func(packet commontypes.RollappPacket) bool { return true } )and reuse for readability
…nd-orders-filters
…list-demand-orders-filters
…list-demand-orders-filters
There was a problem hiding this comment.
state breaking? will require migration
| @@ -177,6 +178,11 @@ func (k Keeper) ListDemandOrdersByStatus(ctx sdk.Context, status commontypes.Sta | |||
| for ; iterator.Valid(); iterator.Next() { | |||
| var val types.DemandOrder | |||
| k.cdc.MustUnmarshal(iterator.Value(), &val) | |||
| for _, opt := range opts { | |||
There was a problem hiding this comment.
the limit(limit int) option is bit miss the point here no?
u'll still process and unmarshal the entire query
I think better design for limit should break this iterator
…nd-orders-filters
WalkthroughThe changes introduce enhanced filtering capabilities for demand orders by adding support for filtering by type, rollapp ID, and limit. This includes modifications across various files to implement the new filters, update function signatures, and adjust tests accordingly. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Outside diff range and nitpick comments (5)
proto/dymension/eibc/demand_order.proto (1)
Line range hint
1-1: State migration may be required due to structural changes in stored data.Please ensure a migration strategy is in place for these changes.
x/eibc/keeper/demand_order_test.go (1)
Line range hint
31-44: Consider adding more comprehensive tests to cover all new scenarios introduced by the additional parameters.Would you like assistance in drafting these additional tests?
x/eibc/client/cli/query_command_orders.go (1)
52-52: The use of a simple limit for debugging purposes, as explained, is acceptable. However, consider documenting this clearly to avoid confusion about its intended use.x/eibc/keeper/keeper.go (1)
Line range hint
160-188: Optimize the demand order listing with limit and filter options.The current implementation processes all entries before applying the limit, which could be inefficient for large datasets. Consider applying the limit and filters directly in the database query if possible, or restructuring the loop to break early once the limit is reached and all filters are applied.
- for ; iterator.Valid(); iterator.Next() { - if limit > 0 && len(list) >= limit { - break - } - var val types.DemandOrder - k.cdc.MustUnmarshal(iterator.Value(), &val) - for _, opt := range opts { - if !opt(val) { - continue - } - } - list = append(list, &val) - } + for ; iterator.Valid() && (limit <= 0 || len(list) < limit); iterator.Next() { + var val types.DemandOrder + k.cdc.MustUnmarshal(iterator.Value(), &val) + if !opts.All(val) { + continue + } + list = append(list, &val) + }ibctesting/eibc_test.go (1)
Line range hint
234-330: Review the logic for handling IBC packet transfers and demand order fulfillment.The test logic for handling IBC packet transfers and demand order fulfillment appears complex and could potentially be simplified or broken down into smaller, more manageable functions. Additionally, ensure that all edge cases are covered, such as handling of timeouts and error acknowledgements effectively.
- // complex nested logic here + // proposed refactoring or simplification here
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
x/eibc/types/demand_order.pb.gois excluded by!**/*.pb.gox/eibc/types/query.pb.gois excluded by!**/*.pb.gox/eibc/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- ibctesting/eibc_test.go (4 hunks)
- proto/dymension/eibc/demand_order.proto (2 hunks)
- proto/dymension/eibc/query.proto (2 hunks)
- x/eibc/client/cli/query_command_orders.go (1 hunks)
- x/eibc/keeper/demand_order_test.go (3 hunks)
- x/eibc/keeper/grpc_query.go (2 hunks)
- x/eibc/keeper/grpc_query_test.go (2 hunks)
- x/eibc/keeper/invariants.go (2 hunks)
- x/eibc/keeper/keeper.go (3 hunks)
- x/eibc/types/demand_order.go (3 hunks)
- x/eibc/types/types.go (1 hunks)
Additional context used
LanguageTool
CHANGELOG.md
[uncategorized] ~85-~85: Do not mix variants of the same word (‘acknowledgement’ and ‘acknowledgment’) within a single text. (EN_WORD_COHERENCY)
Context: ....com//issues/822) Acknowledgement not written in case of ackerr - (rollap...
Additional comments not posted (10)
x/eibc/types/types.go (1)
11-13: The addition of new attribute keys aligns well with the expanded functionality of demand orders.proto/dymension/eibc/demand_order.proto (2)
6-6: Import ofrollapp_packet.protois necessary for the new fields and is correctly placed.
31-33: Addition ofrollapp_idandtypefields to theDemandOrdermessage supports the new filtering capabilities as described in the PR.x/eibc/keeper/demand_order_test.go (1)
31-31: The tests have been updated to include the newlimitparameter, ensuring the new functionality is covered.Also applies to: 41-41, 44-44
x/eibc/client/cli/query_command_orders.go (1)
18-53: The CLI command has been correctly updated to handle the new optional parameters for querying demand orders. The parsing and error handling for these parameters are well-implemented.proto/dymension/eibc/query.proto (2)
9-10: Ensure the newly added imports are utilized effectively in the file.
48-54: The addition ofstatus,type,rollapp_id, andlimitfields inQueryDemandOrdersByStatusRequestaligns with the PR objectives to enhance the query's flexibility.x/eibc/keeper/invariants.go (1)
33-33: The addition of a0as a parameter in calls toListDemandOrdersByStatusseems to be a placeholder forlimit. Confirm that this is the intended usage and that it aligns with the new function signature.Also applies to: 38-38, 43-43
Verification successful
The usage of the
0parameter in theListDemandOrdersByStatusfunction calls is consistent with the new function signature and is intended as a placeholder for thelimitparameter.
x/eibc/keeper/invariants.go: Lines 33, 38, 43x/eibc/keeper/grpc_query.go: Line withListDemandOrdersByStatuscallx/eibc/keeper/demand_order_test.go: Lines withListDemandOrdersByStatuscallsibctesting/eibc_test.go: Line withListDemandOrdersByStatuscallScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `0` parameter in `ListDemandOrdersByStatus` calls. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'ListDemandOrdersByStatus'Length of output: 1557
x/eibc/keeper/grpc_query.go (1)
58-64: The implementation of filtering inDemandOrdersByStatususingfilterOptsis a robust enhancement that aligns with the PR's objectives to add more granular control over the query results.x/eibc/types/demand_order.go (1)
75-77: The inclusion ofRollappIdandTypein theGetEventsmethod ensures that these attributes are available for event handling and logging, which is crucial for traceability.
| func filterOpts(req *types.QueryDemandOrdersByStatusRequest) []filterOption { | ||
| var opts []filterOption | ||
| if req.RollappId != "" { | ||
| opts = append(opts, isRollappId(req.RollappId)) | ||
| } | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| if req.Type != commontypes.RollappPacket_UNDEFINED { | ||
| opts = append(opts, isOrderType(req.Type)) | ||
| } | ||
| return opts | ||
| } | ||
|
|
||
| // Get the demand orders by status | ||
| demandOrders, err := q.ListDemandOrdersByStatus(ctx, statusValue) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| type filterOption func(order types.DemandOrder) bool | ||
|
|
||
| func isRollappId(rollappId string) filterOption { | ||
| return func(order types.DemandOrder) bool { | ||
| return order.RollappId == rollappId | ||
| } | ||
| } | ||
|
|
||
| // Construct the response | ||
| return &types.QueryDemandOrdersByStatusResponse{DemandOrders: demandOrders}, nil | ||
| func isOrderType(orderType ...commontypes.RollappPacket_Type) filterOption { | ||
| return func(order types.DemandOrder) bool { | ||
| for _, ot := range orderType { | ||
| if order.Type == ot { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The helper functions filterOpts, isRollappId, and isOrderType are well-implemented to support the new filtering functionality. Ensure that these functions are covered by unit tests to verify their behavior.
Would you like me to help in writing these unit tests?
| RollappId: rollappPacket.RollappId, | ||
| Type: rollappPacket.Type, |
There was a problem hiding this comment.
Tip
Codebase Verification
The ValidateBasic method in demand_order.go does not include validation for the newly added fields RollappId and Type. This oversight needs to be addressed to ensure the integrity of the data.
RollappIdshould be validated to ensure it is not empty and follows any specific format or constraints required.Typeshould be validated to ensure it is within the expected set of values.
Please add the necessary validation logic for these fields in the ValidateBasic method.
- [x/eibc/types/demand_order.go:34-59]
Analysis chain
The addition of RollappId and Type fields in the NewDemandOrder function is correctly implemented. Ensure these fields are validated appropriately in the ValidateBasic method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the validation logic for `RollappId` and `Type`.
# Test: Search for validation logic in the `ValidateBasic` method.
rg --type go $'ValidateBasic'
Length of output: 4426
Script:
#!/bin/bash
# Extract the ValidateBasic method from demand_order.go to inspect its content.
ast-grep --lang go --pattern $'func (m *DemandOrder) ValidateBasic() error {
$$$
}' x/eibc/types/demand_order.go
Length of output: 1572
| @@ -76,15 +77,15 @@ func (suite *KeeperTestSuite) TestQueryDemandOrdersByStatus() { | |||
| suite.Require().NoError(err) | |||
|
|
|||
| // Query demand orders by status | |||
| res, err := suite.queryClient.DemandOrdersByStatus(sdk.WrapSDKContext(suite.Ctx), &types.QueryDemandOrdersByStatusRequest{Status: status.String()}) | |||
| res, err := suite.queryClient.DemandOrdersByStatus(sdk.WrapSDKContext(suite.Ctx), &types.QueryDemandOrdersByStatusRequest{Status: status}) | |||
There was a problem hiding this comment.
Ensure proper error handling for invalid status codes.
The function DemandOrdersByStatus should handle invalid status codes gracefully by returning an appropriate error message. This ensures that the system behaves predictably even with unexpected inputs.
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(res.DemandOrders) | ||
| suite.Require().Len(res.DemandOrders, 1, fmt.Sprintf("Expected 1 demand order for status %s, but got %d", status, len(res.DemandOrders))) | ||
| suite.Require().Equal(demandOrder.Id, res.DemandOrders[0].Id) | ||
| } | ||
|
|
||
| // Query with invalid status should return an error | ||
| res, err := suite.queryClient.DemandOrdersByStatus(sdk.WrapSDKContext(suite.Ctx), &types.QueryDemandOrdersByStatusRequest{Status: "INVALID"}) | ||
| res, err := suite.queryClient.DemandOrdersByStatus(sdk.WrapSDKContext(suite.Ctx), &types.QueryDemandOrdersByStatusRequest{Status: -1}) |
There was a problem hiding this comment.
Add test case for handling unexpected status values.
It's good to see error handling for an invalid status code. Consider adding more test cases for other potential invalid inputs to ensure robust error handling across the board.
| @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| ### Features | |||
|
|
|||
| - (delayedack) [#849](https://github.com/dymensionxyz/dymension/issues/849) Add demand order filters: type, rollapp id and limit | |||
There was a problem hiding this comment.
Ensure consistency in terminology within documentation.
The term 'acknowledgement' is used here, but elsewhere 'acknowledgment' might be used. It's best to stick to one spelling to maintain consistency across the documentation.
- Acknowledgement not written in case of ackerr
+ Acknowledgment not written in case of ackerrCommittable suggestion was skipped due to low confidence.
Description
Closes #849
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores