[CORE-700] - P1 clob queries for existing StatefulOrder keeper methods #676
Conversation
…odegen via typescript due to nested fields in request message.
WalkthroughThe changes introduce new functionality for querying stateful orders in the application. This includes new request and response types, commands for listing all stateful orders and getting the count of stateful orders for a specific subaccount, and corresponding tests. The command signature for showing a specific subaccount has also been updated to be more descriptive. Changes
TipsChat with CodeRabbit Bot (
|
| } | ||
|
|
||
| // Queries the count of all stateful orders. | ||
| rpc StatefulOrderCount(QueryStatefulOrderCountRequest) |
There was a problem hiding this comment.
no http transcoding here because the request is uniquely identified by the subaccount. when I added an http path that selected on the subaccount message, go proto-gen compiled without issues, but telescope barfed.
There was a problem hiding this comment.
for my understanding - what will this endpoint be used for?
this is not something that is in the consensus state
There was a problem hiding this comment.
See Jonathan's comment here
|
|
||
| // Expect no stateful orders. | ||
| require.Equal(t, types.QueryAllStatefulOrdersResponse{ | ||
| StatefulOrders: []types.Order{}, |
There was a problem hiding this comment.
Asking about how to populate clob with stateful orders via a network test here.
![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: 4
Files ignored due to filter (2)
- protocol/x/clob/types/query.pb.go
- protocol/x/clob/types/query.pb.gw.go
Files selected for processing (12)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (3 hunks)
- proto/dydxprotocol/clob/query.proto (3 hunks)
- protocol/mocks/QueryClient.go (2 hunks)
- protocol/x/clob/client/cli/query.go (1 hunks)
- protocol/x/clob/client/cli/query_stateful_orders.go (1 hunks)
- protocol/x/clob/client/cli/query_stateful_orders_test.go (1 hunks)
- protocol/x/clob/keeper/grpc_query_stateful_orders.go (1 hunks)
- protocol/x/clob/keeper/grpc_query_stateful_orders_test.go (1 hunks)
- protocol/x/clob/module_test.go (1 hunks)
- protocol/x/subaccounts/client/cli/query_subaccount.go (1 hunks)
Files not summarized due to errors (7)
- protocol/x/clob/keeper/grpc_query_stateful_orders_test.go: Error: Server error
- protocol/x/clob/client/cli/query.go: Error: Server error
- protocol/mocks/QueryClient.go: Error: Server error
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.lcd.ts: Error: Server error
- proto/dydxprotocol/clob/query.proto: Error: Server error
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.rpc.Query.ts: Error: Server error
- protocol/x/clob/client/cli/query_stateful_orders_test.go: Error: Server error
Files skipped from review due to trivial changes (1)
- protocol/x/clob/keeper/grpc_query_stateful_orders.go
Additional comments (Suppressed): 25
protocol/x/clob/client/cli/query.go (1)
- 26-27: The addition of
CmdListStatefulOrders()andCmdGetStatefulOrderCount()commands to thecmdobject is a good way to extend the functionality of the CLI. Ensure that these new commands are properly documented and tested. Also, verify that these commands are working as expected in the CLI after the changes.protocol/x/clob/client/cli/query_stateful_orders_test.go (2)
16-35: The test
TestCmdListStatefulOrdersis well written and covers the basic functionality of theCmdListStatefulOrderscommand. It checks that the command executes without errors and that it returns an empty list of stateful orders, which is the expected behavior in this test setup.37-58: The test
TestCmdGetStatefulOrderCountis also well written and covers the basic functionality of theCmdGetStatefulOrderCountcommand. It checks that the command executes without errors and that it returns a count of 0 for stateful orders, which is the expected behavior in this test setup.protocol/x/clob/keeper/grpc_query_stateful_orders_test.go (5)
16-43: The
populateStatefulOrdersfunction is a helper function that sets up the test environment by adding stateful orders to the keeper. It adds a long-term order, a triggered conditional order, and an untriggered conditional order. This function is used in bothTestAllStatefulOrdersandTestStatefulOrderCountto ensure a consistent test environment.45-82: The
TestAllStatefulOrdersfunction tests theAllStatefulOrdersfunction of theClobKeeper. It checks for two scenarios: when the request is nil and when the request is successful. In the nil request scenario, it expects an error. In the successful request scenario, it expects a response containing all the stateful orders that were added in thepopulateStatefulOrdersfunction.84-118: The
TestStatefulOrderCountfunction tests theStatefulOrderCountfunction of theClobKeeper. It checks for two scenarios: when the request is nil and when the request is successful. In the nil request scenario, it expects an error. In the successful request scenario, it expects a response containing the count of stateful orders for a specific subaccount.57-70: The test case "Nil request returns an error" checks if the function correctly handles a nil request and returns an error. The test case "Success" checks if the function correctly returns all stateful orders when the request is valid. The expected orders are the ones added in the
populateStatefulOrdersfunction.96-106: The test case "Nil request returns an error" checks if the function correctly handles a nil request and returns an error. The test case "Success" checks if the function correctly returns the count of stateful orders for a specific subaccount when the request is valid. The expected count is 3, which is the number of orders added in the
populateStatefulOrdersfunction.protocol/x/clob/client/cli/query_stateful_orders.go (1)
- 13-44: The
CmdListStatefulOrdersfunction looks good. It correctly sets up a new cobra command, reads the page request, makes a query to theAllStatefulOrdersfunction, and handles errors appropriately. The pagination flags are also correctly added to the command.protocol/x/subaccounts/client/cli/query_subaccount.go (1)
- 48-48: The command usage has been updated from
show-subaccount [index]toshow-subaccount [owner] [account-number]. Ensure that all calls to this command throughout the codebase have been updated to match the new usage.protocol/x/clob/module_test.go (1)
- 274-281: The test has been updated to reflect the addition of two new commands:
list-stateful-ordersandshow-stateful-order-count. Ensure that these commands are implemented correctly and that they return the expected results when invoked.proto/dydxprotocol/clob/query.proto (3)
7-13: The import statement for "dydxprotocol/clob/order.proto" has been added. Ensure that the file "order.proto" exists in the specified path and that it contains the necessary definitions for handling stateful orders.
42-54: Two new RPC methods,
AllStatefulOrdersandStatefulOrderCount, have been added to theQueryservice. These methods are used to query all stateful orders and the count of all stateful orders respectively. The HTTP GET paths for these methods are "/dydxprotocol/clob/orders/stateful" and "/dydxprotocol/clob/orders/stateful/count" respectively. Ensure that these paths are correctly configured in the server routing and that they do not conflict with any existing paths.127-150: New message definitions have been added for the request and response types of the new RPC methods.
QueryAllStatefulOrdersRequestandQueryAllStatefulOrderCountRequestare the request messages forAllStatefulOrdersandStatefulOrderCountrespectively.QueryAllStatefulOrdersResponseandQueryStatefulOrderCountResponseare the corresponding response messages. Ensure that these message definitions match the expected request and response formats for the new RPC methods.protocol/mocks/QueryClient.go (2)
119-147: The new mock function
AllStatefulOrdershas been added correctly. It follows the same pattern as the other mock functions in the file, taking in a context, a request object, and a variable number of gRPC call options. It then uses theCalledmethod from themockpackage to simulate a function call and return the expected results. This function will be useful for testing theAllStatefulOrdersgRPC endpoint.389-417: The new mock function
StatefulOrderCounthas been added correctly. It follows the same pattern as the other mock functions in the file, taking in a context, a request object, and a variable number of gRPC call options. It then uses theCalledmethod from themockpackage to simulate a function call and return the expected results. This function will be useful for testing theStatefulOrderCountgRPC endpoint.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.rpc.Query.ts (5)
4-4: The import statement has been updated to include the new request and response types for the stateful orders queries. This is necessary for the new query methods that have been added to the
Queryinterface and theQueryClientImplclass.19-28: Two new query methods have been added to the
Queryinterface:allStatefulOrdersandstatefulOrderCount. These methods return promises that resolve to the responses of the corresponding gRPC queries. TheallStatefulOrdersmethod takes an optionalQueryAllStatefulOrdersRequestparameter, while thestatefulOrderCountmethod requires aQueryStatefulOrderCountRequestparameter.36-40: The
allStatefulOrdersandstatefulOrderCountmethods have been bound to theQueryClientImplinstance in the constructor. This ensures that the methods can be called with the correct context.75-87: The
allStatefulOrdersandstatefulOrderCountmethods have been implemented in theQueryClientImplclass. These methods encode the request parameters, make the gRPC request, and decode the response data. TheallStatefulOrdersmethod provides a default value for the request parameter, settingpaginationtoundefinedif no request object is provided.109-120: The
allStatefulOrdersandstatefulOrderCountmethods have been added to theQueryextension returned by thecreateRpcQueryExtensionfunction. This allows the methods to be called on the extendedQueryClientinstance.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.lcd.ts (3)
1-6: The import statement on line 3 has been extended to include
QueryAllStatefulOrdersRequestandQueryAllStatefulOrdersResponseSDKType. This is in line with the PR summary which mentions the addition of new query commands for handling stateful orders. The changes look good and are in line with the intended functionality.13-16: The
allStatefulOrdersmethod is now bound to theLCDQueryClientclass instance. This is a necessary step to ensure that the method can be called correctly on an instance of the class.46-65: A new method
allStatefulOrdershas been added to theLCDQueryClientclass. This method takes an optionalQueryAllStatefulOrdersRequestobject as a parameter, which may contain apaginationproperty. Ifpaginationis provided, it is used to set pagination parameters for the API request. The method constructs the API endpoint for querying all stateful orders and makes a GET request to that endpoint using thereqobject. It returns a promise that resolves to aQueryAllStatefulOrdersResponseSDKTypeobject. This is in line with the PR summary which mentions the addition of new query commands for handling stateful orders. The changes look good and are in line with the intended functionality.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (1)
- 188-252: The new hunk introduces interfaces and functions for querying all stateful orders and getting the count of stateful orders for a specific subaccount. Ensure that these new features are properly integrated with the rest of the system and that they do not introduce any breaking changes. Also, make sure that the new features are covered by tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- protocol/x/clob/client/cli/query_stateful_orders.go (1 hunks)
- protocol/x/clob/client/cli/query_stateful_orders_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/client/cli/query_stateful_orders_test.go
Additional comments (Suppressed): 1
protocol/x/clob/client/cli/query_stateful_orders.go (1)
- 1-48: The
CmdListStatefulOrdersfunction looks good. It correctly sets up a new cobra command to list all stateful orders. It also handles pagination and error checking properly.
clemire
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- proto/dydxprotocol/clob/query.proto (3 hunks)
Additional comments (Suppressed): 3
proto/dydxprotocol/clob/query.proto (3)
10-10: The import statement for "order.proto" is new. Ensure that the file exists and is accessible from the current path. Also, verify that the definitions in "order.proto" are used correctly in the new RPC methods and message types.
42-54: Two new RPC methods,
AllStatefulOrdersandStatefulOrderCount, have been added. Ensure that these methods are implemented on the server side and that they return the expected results. Also, verify that the HTTP GET paths are correctly configured and do not conflict with existing paths.127-150: New message types
QueryAllStatefulOrdersRequest,QueryAllStatefulOrdersResponse,QueryStatefulOrderCountRequest, andQueryStatefulOrderCountResponsehave been added. Ensure that these message types are used correctly in the new RPC methods. Also, verify that the fields in these message types are appropriate for their intended use. For example,QueryAllStatefulOrdersRequestincludes apaginationfield for paginated results, andQueryStatefulOrderCountRequestincludes asubaccount_idfield to specify a subaccount.
… think this is necessary, it doesn't save much and this does not cause an error.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- protocol/x/clob/client/cli/query_stateful_orders.go (1 hunks)
- protocol/x/clob/client/cli/query_stateful_orders_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/client/cli/query_stateful_orders_test.go
Additional comments (Suppressed): 2
protocol/x/clob/client/cli/query_stateful_orders.go (2)
13-44: The
CmdListStatefulOrdersfunction is well implemented. It correctly sets up a new cobra command, reads the pagination flags, makes a query to theAllStatefulOrdersfunction, and handles errors appropriately. The pagination and query flags are also correctly added to the command. No changes are needed here.46-81: The
CmdGetStatefulOrderCountfunction is well implemented. It correctly sets up a new cobra command, reads the arguments, makes a query to theStatefulOrderCountfunction, and handles errors appropriately. The query flags are also correctly added to the command. However, it would be better to validate theownerargument to ensure it's a valid address before making the query. This can help prevent unnecessary calls to the server with invalid data.argOwner := args[0] var argNumber uint32 if argNumber, err = cast.ToUint32E(args[1]); err != nil { return err } + if _, err := sdk.AccAddressFromBech32(argOwner); err != nil { + return fmt.Errorf("invalid owner address %s: %w", argOwner, err) + } params := &types.QueryStatefulOrderCountRequest{
| ctx := sdk.UnwrapSDKContext(c) | ||
|
|
||
| statefulOrders := k.GetAllStatefulOrders(ctx) | ||
|
|
There was a problem hiding this comment.
this query is supposed to be paginated, right?
Changelist
Add cli query commands, grpc endpoints for AllStatefulOrders, and StatefulOrderCount (by subaccount).
Test Plan
normal tests: query client method, gRPC query tests, cli tests.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.