-
Notifications
You must be signed in to change notification settings - Fork 114
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
[OTE-221] Add query for PendingSendPacket #1176
Conversation
WalkthroughThe update enhances the rate limit module by introducing features to manage pending send packets efficiently. It includes new RPC methods, CLI commands, keeper functions, and types to facilitate the retrieval and processing of pending send packets within the Dydx Protocol ecosystem. Changes
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 (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/x/ratelimit/types/pending_send_packet.pb.go
is excluded by:!**/*.pb.go
protocol/x/ratelimit/types/query.pb.go
is excluded by:!**/*.pb.go
protocol/x/ratelimit/types/query.pb.gw.go
is excluded by:!**/*.pb.gw.go
Files selected for processing (8)
- proto/dydxprotocol/ratelimit/pending_send_packet.proto (1 hunks)
- proto/dydxprotocol/ratelimit/query.proto (3 hunks)
- protocol/x/ratelimit/client/cli/pending_send_packets.go (1 hunks)
- protocol/x/ratelimit/client/cli/pending_send_packets_test.go (1 hunks)
- protocol/x/ratelimit/client/cli/query.go (1 hunks)
- protocol/x/ratelimit/keeper/grpc_query.go (2 hunks)
- protocol/x/ratelimit/keeper/grpc_query_test.go (1 hunks)
- protocol/x/ratelimit/types/keys.go (2 hunks)
Additional comments: 4
proto/dydxprotocol/ratelimit/pending_send_packet.proto (1)
- 1-12: The protobuf definition for
PendingSendPacket
is well-structured and clearly documented. The use ofstring
forchannel_id
anduint64
forsequence
is appropriate for their intended purposes. The overall structure adheres to best practices for protobuf definitions.protocol/x/ratelimit/client/cli/query.go (1)
- 25-25: The addition of
CmdPendingSendPackets()
to theGetQueryCmd
function is correctly implemented and follows the established pattern for adding new CLI commands. This enhances the module's query capabilities by providing a direct way to query pending send packets.protocol/x/ratelimit/client/cli/pending_send_packets_test.go (1)
- 17-30: The test case for
CmdPendingSendPackets
is correctly implemented and verifies the command's ability to return an empty list of pending send packets. To ensure comprehensive coverage, consider adding more test scenarios, particularly for cases where pending send packets exist in the system.protocol/x/ratelimit/client/cli/pending_send_packets.go (1)
- 10-34: The implementation of the
CmdPendingSendPackets
CLI command is well-structured and adheres to Cosmos SDK standards. The command setup and execution logic are correctly implemented, providing a straightforward way for users to query all pending send packets.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/go.mod
is excluded by:!**/*.mod
Files selected for processing (2)
- proto/dydxprotocol/ratelimit/pending_send_packet.proto (1 hunks)
- proto/dydxprotocol/ratelimit/query.proto (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- proto/dydxprotocol/ratelimit/pending_send_packet.proto
- proto/dydxprotocol/ratelimit/query.proto
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/ratelimit/types/pending_send_packet.pb.go
is excluded by:!**/*.pb.go
protocol/x/ratelimit/types/query.pb.go
is excluded by:!**/*.pb.go
Files selected for processing (3)
- proto/dydxprotocol/ratelimit/pending_send_packet.proto (1 hunks)
- proto/dydxprotocol/ratelimit/query.proto (3 hunks)
- protocol/x/ratelimit/module_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- proto/dydxprotocol/ratelimit/pending_send_packet.proto
- proto/dydxprotocol/ratelimit/query.proto
Additional comments: 1
protocol/x/ratelimit/module_test.go (1)
- 135-138: The update to
TestAppModuleBasic_GetQueryCmd
correctly reflects the addition of the "pending-send-packets" command, aligning with the PR's objective to enhance the ratelimit module with a new query endpoint. It's crucial to keep tests updated to cover new functionalities, ensuring the reliability of the module.
protocol/x/ratelimit/types/keys.go
Outdated
func SplitPendingSendPacketKey(key []byte) (channelId string, sequenceNumber uint64) { | ||
parts := bytes.Split(key, []byte("_")) | ||
if len(parts) != 2 { | ||
panic(fmt.Sprintf("unexpected key format: %s", key)) |
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.
What does user see if we panic here? I assume nothing or some internal error?
Let's:
- Log an error here so we could see this on server side
- Also return this error so the user can see this error as well (easier for troubleshooting)
The error string could also be more helpful (e.g. "unexpected PendingSendPacket key format...")
Co-authored-by: Teddy Ding <teddy@dydx.exchange>
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/ratelimit/types/keys.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/ratelimit/types/keys.go
4dba64a
to
adaa83b
Compare
adaa83b
to
34c1fc9
Compare
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/ratelimit/types/query.pb.go
is excluded by:!**/*.pb.go
protocol/x/ratelimit/types/query.pb.gw.go
is excluded by:!**/*.pb.gw.go
Files selected for processing (7)
- proto/dydxprotocol/ratelimit/query.proto (3 hunks)
- protocol/x/ratelimit/client/cli/pending_send_packets.go (1 hunks)
- protocol/x/ratelimit/client/cli/pending_send_packets_test.go (1 hunks)
- protocol/x/ratelimit/keeper/grpc_query.go (2 hunks)
- protocol/x/ratelimit/keeper/grpc_query_test.go (1 hunks)
- protocol/x/ratelimit/types/keys.go (2 hunks)
- protocol/x/ratelimit/types/keys_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- proto/dydxprotocol/ratelimit/query.proto
- protocol/x/ratelimit/client/cli/pending_send_packets.go
- protocol/x/ratelimit/client/cli/pending_send_packets_test.go
- protocol/x/ratelimit/keeper/grpc_query.go
- protocol/x/ratelimit/keeper/grpc_query_test.go
- protocol/x/ratelimit/types/keys.go
- protocol/x/ratelimit/types/keys_test.go
033b243
to
9f76806
Compare
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/pending_send_packet.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
Additional comments: 9
indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1)
- 1-2: The update from
_101
to_102
in the import statement seems to be a straightforward change, likely due to a version update or reorganization of the source modules. Ensure that all necessary exports from_102
are being used as expected.indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
- 1-14: The reordering and addition of imports, along with the adjustments to the
indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.lcd.ts (2)
- 14-14: Binding the new
allPendingSendPackets
function in the constructor is a good practice for maintaining the correctthis
context. Ensure that the endpoint used in the function is correctly implemented and tested.- 41-43: The implementation of the
allPendingSendPackets
function appears to correctly use the endpoint for fetching data. Ensure that the functionality has been thoroughly tested, especially the handling of different response scenarios.indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/pending_send_packet.ts (1)
- 8-37: The
PendingSendPacket
interfaces and their associated encode/decode functions are well-structured and follow the protobuf specification. Ensure that the field types and encode/decode logic have been correctly implemented and tested.indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.rpc.Query.ts (2)
- 13-15: Adding the
allPendingSendPackets
function to theQuery
interface and its implementation inQueryClientImpl
enhances the functionality. Ensure that the RPC request handling in the implementation is correctly done and thoroughly tested.- 39-42: The implementation of the
allPendingSendPackets
function inQueryClientImpl
appears to correctly handle the RPC request. Ensure that the functionality has been thoroughly tested, especially the handling of different response scenarios.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
- 66-173: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [69-322]
The reordering and renaming of imports in the
dydxprotocol
namespace appear to be well-structured and necessary for the functionality. Ensure that all references to the renamed and reordered imports are valid and that the changes have been thoroughly tested.indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.ts (1)
- 255-337: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [62-335]
The addition of
QueryAllPendingSendPacketsRequest
andQueryAllPendingSendPacketsResponse
interfaces, along with their encode/decode functions, enhances the functionality for querying all pending send packets. Ensure that the field types and encode/decode logic have been correctly implemented and thoroughly tested.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/go.mod
is excluded by:!**/*.mod
Files selected for processing (7)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/pending_send_packet.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/pending_send_packet.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.lcd.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.rpc.Query.ts
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
- indexer/packages/v4-protos/src/codegen/google/bundle.ts
Additional comments: 4
indexer/packages/v4-protos/src/codegen/dydxprotocol/ratelimit/query.ts (4)
- 67-73: The addition of
QueryAllPendingSendPacketsRequest
andQueryAllPendingSendPacketsRequestSDKType
interfaces is clear and follows the established pattern for request types in this codebase. This aligns with the PR's objective to introduce a query endpoint for pending send packets.- 79-87: The
QueryAllPendingSendPacketsResponse
andQueryAllPendingSendPacketsResponseSDKType
interfaces are well-defined, with a clear focus on encapsulating the pending send packets data. This structure is essential for the functionality introduced by the PR, ensuring that the response data is correctly typed and accessible.- 264-291: The implementation of the
encode
,decode
, andfromPartial
functions forQueryAllPendingSendPacketsRequest
follows the established conventions in this codebase. These functions are crucial for the serialization and deserialization of the request data, ensuring compatibility with the underlying protobufjs library.- 300-335: The
encode
,decode
, andfromPartial
functions forQueryAllPendingSendPacketsResponse
are implemented correctly, ensuring that the response data can be serialized and deserialized effectively. This is crucial for the integration of the new query endpoint with the rest of the system, facilitating the retrieval and processing of pending send packets data.
cdc0d7f
to
d76e666
Compare
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/ratelimit/types/keys.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/ratelimit/types/keys.go
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/ratelimit/types/keys.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/ratelimit/types/keys.go
* Add query for PendingSendPacket * update go.mod * lint fixes * newline * protos * proto stuff * update tests * Update protocol/x/ratelimit/types/keys.go Co-authored-by: Teddy Ding <teddy@dydx.exchange> * address comments * add error handling * indexer protos * nit --------- Co-authored-by: Teddy Ding <teddy@dydx.exchange> (cherry picked from commit e545bbf) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts # indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts # indexer/packages/v4-protos/src/codegen/google/bundle.ts # protocol/go.mod
--------- Co-authored-by: Teddy Ding <teddy@dydx.exchange> (cherry picked from commit e545bbf) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts # indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts # indexer/packages/v4-protos/src/codegen/google/bundle.ts # protocol/go.mod * fix protos * update go.mod --------- Co-authored-by: Mohammed Affan <affanmd@nyu.edu> Co-authored-by: affan <affan@dydx.exchange>
* [OTE-221] Add query for PendingSendPacket (backport #1176) (#1221) --------- Co-authored-by: Teddy Ding <teddy@dydx.exchange> (cherry picked from commit e545bbf) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts # indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts # indexer/packages/v4-protos/src/codegen/google/bundle.ts # protocol/go.mod * fix protos * update go.mod --------- Co-authored-by: Mohammed Affan <affanmd@nyu.edu> Co-authored-by: affan <affan@dydx.exchange> * [Backport v4.x] backport full node streaming to v4.x branch (#1270) * [CT-645] Move off chain updates and v1 to a different package (#1131) * [CT-645] Add protos for orderbook stream query service * move removal reasons to a separate package * [CT-645] Add protos for orderbook stream query service (#1133) * [CT-645] Add protos for orderbook stream query service * make update not nullable * fix build * [CT-644] instantiate grpc stream manager (#1134) * [CT-644] instantiate grpc stream manager * update type * update channel type * [CT-646] stream offchain updates through stream manager (#1138) * [CT-646] stream offchain updates through stream manager * comments * fix lint * get rid of finished * comments * comments * [CT-652] add command line flag for full node streaming (#1145) * [CT-647] construct the initial orderbook snapshot (#1147) * [CT-647] construct the initial orderbook snapshot * [CT-647] initialize new streams and send orderbook snapshot (#1152) * [CT-647] initialize new streams and send orderbook snapshot * use sync once * comments * [CT-700] separate indexer and grpc streaming events (#1209) * [CT-700] separate indexer and grpc streaming events * fix tests * comments * update * [CT-700] only send response when there is at least one update (#1216) * [CT-712] send order update when short term order state fill amounts are pruned (#1241) * [CT-712] send fill amount updates for reverted operations (#1240) * [CT-723] add block number + stage to grpc updates (#1252) * [CT-723] add block number + stage to grpc updates * add indexer changes * [CT-727] avoid state reads when sending updates (#1261) * [CT-712] send updates for both normal order matches and liquidation (#1280) (#1281) * Fix lib.ErrorLogWithError: properly pass in args (#1306) (#1310) (cherry picked from commit a91c1ca) Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> * fix broken tests (#1312) (#1316) (cherry picked from commit 5ec37d2) Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Mohammed Affan <affanmd@nyu.edu> Co-authored-by: affan <affan@dydx.exchange> Co-authored-by: jayy04 <103467857+jayy04@users.noreply.github.com> Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com>
Changelist
Test Plan
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.