Full node streaming -- perp position to signed int #2544
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
proto/dydxprotocol/subaccounts/streaming.proto (1)
31-32: Consider version strategy for breaking change.Changing the field type from
uint64toint64is a breaking change that affects wire format compatibility. Consider:
- Implementing a migration strategy
- Updating API version
- Coordinating with client implementations
- Adding validation for negative values in relevant handlers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/x/subaccounts/types/streaming.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/streaming.ts (6 hunks)
- proto/dydxprotocol/subaccounts/streaming.proto (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
🔇 Additional comments (8)
proto/dydxprotocol/subaccounts/streaming.proto (1)
31-32: LGTM! Verify client compatibility.The change from
uint64toint64for quantums is well-documented and appropriate for representing both long and short positions.Let's verify other proto definitions for consistency:
✅ Verification successful
Verified: Position representation change is consistent
The change from
uint64toint64for quantums instreaming.protoaligns with the codebase's design. The main perpetual position message inperpetual_position.protoalready uses a custom serializable integer type that can handle signed values, and other quantum-related fields across the codebase are appropriately typed for their specific use cases (e.g. order quantums remain unsigned as they represent absolute sizes).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other quantum definitions in proto files to ensure consistent typing rg -t proto "quantums.*=.*;" --no-headingLength of output: 73
Script:
#!/bin/bash # Search for files with .proto extension first fd -e proto # Then search for quantum definitions in those files rg "quantums.*=.*;" $(fd -e proto)Length of output: 12108
Script:
#!/bin/bash # Let's check the perpetual position related files specifically cat proto/dydxprotocol/subaccounts/perpetual_position.proto cat proto/dydxprotocol/indexer/protocol/v1/perpetual.protoLength of output: 1697
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/streaming.ts (5)
62-62: LGTM! Clear documentation update.The updated documentation clearly indicates that negative values represent short positions, which aligns with the PR objective of supporting signed integers for perpetual positions.
Also applies to: 74-74
181-181: LGTM! Appropriate default value change.The change from
Long.UZEROtoLong.ZEROis correct for supporting signed integers, allowing for both positive and negative position values.
192-192: LGTM! Correct encoding type update.The encoding method now uses
int64instead ofuint64, which is necessary for properly serializing negative position values.
212-212: LGTM! Correct decoding type update.The decoding method now uses
int64instead ofuint64, ensuring proper deserialization of negative position values.
227-227: Verify handling of negative values in API consumers.The change from
Long.UZEROtoLong.ZEROis correct, but we should ensure all consumers of this API are prepared to handle negative position values.Let's verify the API consumers:
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the API consumers. The search results show:
- The
SubaccountPerpetualPositiontype is used in streaming updates and its documentation explicitly states "The size of the position in base quantums. Negative means short."- The consumers of this type primarily handle it for streaming updates and position tracking.
- The code shows no assumptions about non-negative values in the quantums field.
- The change from
UZEROtoZEROaligns with the documented behavior of supporting negative values for short positions.Change from
Long.UZEROtoLong.ZEROis safe and correctly implemented.The codebase already expects and handles negative quantum values for short positions. The documentation and usage patterns confirm this is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of SubaccountPerpetualPosition to verify consumers # are prepared for negative quantums # Look for comparisons or operations with quantums that might assume non-negative values rg -A 3 "quantums\s*(===|!==|<=|>=|>|<|[+\-*/])" --type ts # Look for direct usage of the type rg -A 3 "SubaccountPerpetualPosition" --type tsLength of output: 23392
protocol/x/subaccounts/keeper/subaccount.go (2)
301-301: LGTM! Consistent with the previous change.The change maintains consistency with the earlier modification while preserving
Uint64()for asset positions which don't need to represent short positions.
153-153: LGTM! Verify integration with streaming consumers.The change from
Uint64()toInt64()correctly enables representation of short positions in perpetual markets.Let's verify the streaming consumers are ready for signed integers:
✅ Verification successful
The search results show that the streaming consumers are ready for signed integers. Specifically:
- The protocol defines
SubaccountPerpetualPositionwith signed integer quantums inproto/dydxprotocol/subaccounts/streaming.proto:// The size of the position in base quantums. Negative means short. int64 quantums = 2;
- The indexer code handles signed quantums through the Long type in
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/streaming.ts:/** The size of the position in base quantums. Negative means short. */ quantums: Long;
- The streaming messages are properly generated and handled in both the protocol and indexer codebases.
LGTM! The streaming consumers are ready for signed integers.
The change from
Uint64()toInt64()is safe as the streaming consumers already expect and handle signed integers for perpetual position quantums.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for streaming message handling code to verify int64 compatibility rg -A 5 "SubaccountPerpetualPosition.*quantums.*"Length of output: 51
Script:
#!/bin/bash # Let's try a broader search for perpetual position quantums in proto definitions and handlers rg -l "PerpetualPosition" | xargs rg -A 5 "quantums" # Also search for streaming update handlers rg -l "StreamSubaccountUpdate" | xargs rg -A 5 "quantums"Length of output: 393653
|
@Mergifyio backport release/protocol/v6.x |
✅ Backports have been created
|
(cherry picked from commit 0e185fd)
|
https://github.com/Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 0e185fd)
previously was uint32. change to int64
Summary by CodeRabbit
New Features
quantumsfield in subaccount positions to allow for negative values, indicating short positions.quantumsfield's interpretation.Bug Fixes
quantumsfromuint64toint64across relevant interfaces and methods to reflect the ability to represent negative values.