[CT-1320] proto for FNS price updates #2586
Conversation
WalkthroughThe changes in this pull request involve modifications to Protocol Buffers files within the Changes
Possibly related PRs
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 (
|
| // Market ids for price updates. | ||
| repeated uint32 market_ids = 3; |
There was a problem hiding this comment.
technically we can derive these from the clob pair ids, but i think this is slightly more flexible
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
proto/dydxprotocol/prices/streaming.proto (3)
11-12: Enhance market_id field documentationConsider adding more context to the market_id field documentation, such as valid ranges or special values if any.
- // The `Id` of the `Market`. + // The unique identifier of the Market. Must be a valid market ID in the system. + // References a Market in the markets module. uint32 market_id = 1;
14-15: Consider adding validation rules for price fieldThe price field is correctly marked as non-nullable, but consider adding validation rules or constraints if applicable.
Would you like assistance in defining validation rules for the price field using the validate.rules option?
17-18: Enhance snapshot field documentationThe snapshot field could benefit from more detailed documentation explaining when it's true vs false and its implications.
- // Snapshot indicates if the response is from a snapshot of the price. + // Snapshot indicates if this update is part of an initial state snapshot (true) + // or a real-time price update (false). Snapshots are typically sent when + // first subscribing to price updates for a market. bool snapshot = 3;proto/dydxprotocol/clob/query.proto (2)
174-176: Add documentation comment for the market_ids field.While the field addition is correct, consider adding a documentation comment similar to other fields in the message to explain its purpose and usage.
// Subaccount ids to stream subaccount updates for. repeated dydxprotocol.subaccounts.SubaccountId subaccount_ids = 2; + // Market ids to stream price updates for. repeated uint32 market_ids = 3;
174-176: Well-designed integration of price updates into existing streaming infrastructure.The approach of adding price updates to the existing streaming infrastructure is clean and maintainable:
- It reuses the established streaming patterns
- It maintains backward compatibility
- It allows clients to selectively subscribe to price updates for specific markets
Consider documenting the following aspects in the service documentation:
- Expected frequency of price updates
- Ordering guarantees between different update types
- Handling of price update errors
Also applies to: 202-202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/prices/types/streaming.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/dydxprotocol/clob/query.proto(3 hunks)proto/dydxprotocol/prices/streaming.proto(1 hunks)
🧰 Additional context used
🪛 buf
proto/dydxprotocol/prices/streaming.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (4)
proto/dydxprotocol/prices/streaming.proto (2)
9-19: Well-structured message definition for price updates
The StreamPriceUpdate message is well-designed with appropriate field types and clear separation of concerns. The inclusion of the snapshot flag is a good practice for streaming implementations.
4-4: Verify gogoproto dependency availability
The static analysis indicates that "gogoproto/gogo.proto" file is not found. This dependency is required for the nullable option used in the price field.
✅ Verification successful
gogoproto dependency is properly configured and widely used
The verification shows that:
- The project has
github.com/gogo/protobufdependency correctly set up inprotocol/go.mod - The
gogoproto/gogo.protoimport is consistently used across 65+ proto files in the codebase - This is a standard pattern in the project's protobuf definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if gogoproto is properly set up in the project
fd -t f go.mod | xargs grep -l "gogo/protobuf"
# Verify other proto files using gogoproto
rg -t proto "import.*gogoproto/gogo.proto"
Length of output: 137
Script:
#!/bin/bash
# Check the content of go.mod to verify gogo/protobuf dependency
cat protocol/go.mod
# Use find to locate proto files and grep for gogoproto imports
find . -name "*.proto" -type f -exec grep -l "import.*gogoproto/gogo.proto" {} \;
Length of output: 29093
🧰 Tools
🪛 buf
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/dydxprotocol/clob/query.proto (2)
17-17: LGTM: Import statement is correctly placed and necessary.
The import of the prices streaming proto is properly organized with other related imports and is required for the new price update functionality.
202-202: Verify the StreamPriceUpdate message structure.
The addition to the oneof group is correct, but let's verify the structure of the StreamPriceUpdate message to ensure it contains all necessary fields for price updates.
✅ Verification successful
StreamPriceUpdate message structure is well-defined and properly integrated
The StreamPriceUpdate message has a well-defined structure with all necessary fields for price updates:
- Contains market_id (uint32) for market identification
- Uses MarketPrice message (from market_price.proto) which includes id and exponent fields
- Has a snapshot flag to indicate if the response is from a price snapshot
The message structure is complete and properly integrated with the required imports and dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the structure of StreamPriceUpdate message
# Look for the StreamPriceUpdate message definition
rg -A 10 "message StreamPriceUpdate" --type proto
# Verify any existing usage patterns
ast-grep --pattern 'StreamPriceUpdate $var = $_'
Length of output: 132
Script:
#!/bin/bash
# Search for StreamPriceUpdate message definition using ripgrep without file type restriction
rg -A 10 "message StreamPriceUpdate"
# Search for proto files containing StreamPriceUpdate
fd . -e proto -x rg -l "StreamPriceUpdate" {}
Length of output: 851
Script:
#!/bin/bash
# Let's check the MarketPrice message structure since it's used in StreamPriceUpdate
rg -A 10 "message MarketPrice" proto/dydxprotocol/prices/streaming.proto
Length of output: 74
Script:
#!/bin/bash
# Let's check the MarketPrice message structure in any proto file
rg -A 10 "message MarketPrice"
# Also check for any imports in the streaming.proto file to locate MarketPrice
rg "^import" proto/dydxprotocol/prices/streaming.proto
Length of output: 2986
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
8-14: LGTM - Generated code maintains correct structureThe namespace exports maintain the correct sequential structure and properly incorporate the new protobuf/any type. The changes appear to be automatically generated from protocol buffer definitions, maintaining consistency with Google's protobuf standards.
Since this is generated code, ensure that any manual modifications to proto files that led to these changes are properly documented and follow the protobuf best practices.
indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/streaming.ts (3)
4-15: Clean up documentation formatting.Remove unnecessary blank lines in the documentation comments to improve readability.
/** StreamPriceUpdate provides information on a price update. */ - export interface StreamPriceUpdate { /** The `Id` of the `Market`. */ marketId: number; /** The updated price. */ - price?: MarketPrice; /** Snapshot indicates if the response is from a snapshot of the price. */ - snapshot: boolean; }
16-27: Remove redundant interface documentation.The documentation comment for
StreamPriceUpdateSDKTypeis redundant since it's just an SDK type mapping ofStreamPriceUpdate.-/** StreamPriceUpdate provides information on a price update. */ - export interface StreamPriceUpdateSDKType {
38-52: Add null check for message parameter.The encode method should validate the input message parameter to prevent runtime errors.
encode(message: StreamPriceUpdate, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { + if (!message) { + return writer; + } + if (message.marketId !== 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts(4 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts(13 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/streaming.ts(1 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 due to trivial changes (1)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
🔇 Additional comments (21)
indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
1-6: Verify the usage of newly added protobuf/any import
The addition of protobuf/any import suggests new dynamic type handling requirements in the codebase. Let's verify where this is being used.
✅ Verification successful
The protobuf/any import is correctly added and actively used
The protobuf/any import is being used in the delaymsg module for handling delayed messages. Specifically:
- It's imported and used in
dydxprotocol/delaymsg/tx.tsanddydxprotocol/delaymsg/delayed_message.ts - The corresponding proto files (
delaymsg/tx.protoanddelaymsg/delayed_message.proto) explicitly importgoogle/protobuf/any.proto - The Any type is used to wrap arbitrary protocol messages in the delayed message system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of google.protobuf.Any in proto files and generated code
# This will help understand why the Any type was needed
# Search for Any type usage in proto files
echo "Searching for Any type usage in proto files..."
rg -t proto "google\.protobuf\.Any"
# Search for _136 (protobuf/any) usage in generated TypeScript files
echo "Searching for Any type usage in generated TypeScript files..."
rg "_136\." "indexer/packages/v4-protos/src/codegen/"
Length of output: 357
Script:
#!/bin/bash
# Let's try a different approach to find Any type usage
# Search for "Any" in TypeScript files within the v4-protos package
echo "Searching for Any type references..."
rg "Any" "indexer/packages/v4-protos/src/"
# List all .proto files to understand available proto definitions
echo -e "\nListing .proto files..."
fd ".proto$"
# Check if there are any imports or references to "any.proto"
echo -e "\nSearching for any.proto references..."
rg "any\.proto"
Length of output: 11431
indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/streaming.ts (4)
29-35: LGTM!
The base object creation is well-implemented with appropriate default values for each field type.
54-82: LGTM!
The decode implementation correctly handles all message fields and includes proper error handling for unknown fields.
84-91: LGTM!
The fromPartial implementation correctly handles undefined/null cases and provides appropriate default values.
1-92: Verify integration with existing price update handlers.
The StreamPriceUpdate implementation looks solid, but we should verify its integration with existing price update handlers to ensure compatibility.
✅ Verification successful
Let me gather more specific information about the price update implementations to verify compatibility.
Based on the gathered information, I can now provide a final response:
StreamPriceUpdate implementation is correctly integrated with the price update system
The implementation is verified to be correct because:
- The
StreamPriceUpdateinterface correctly uses the existingMarketPricetype which matches the protocol's price representation (with id, exponent, and price fields) - It's properly integrated into the CLOB (Central Limit Order Book) query system as shown by its usage in
dydxprotocol/clob/query.ts - The implementation follows the same protobuf encoding/decoding patterns as other price-related structures in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing price update handlers that might need updates
echo "Searching for existing price handlers..."
rg -t ts "price.*update" --glob '!*.test.ts' -l
# Look for potential message type conflicts
echo "Checking for potential message type conflicts..."
ast-grep --pattern 'interface $name {
$$$
price?: $type
$$$
}'
Length of output: 1203
Script:
#!/bin/bash
# Check the implementation of market price update handler
echo "Checking market price update handler implementation..."
rg -A 10 "class MarketPriceUpdateHandler" indexer/services/ender/src/handlers/markets/market-price-update-handler.ts
# Check the MarketPrice type implementation
echo "Checking MarketPrice type implementation..."
cat indexer/packages/v4-protos/src/codegen/dydxprotocol/prices/market_price.ts
# Check for any direct usage of StreamPriceUpdate
echo "Checking StreamPriceUpdate usage..."
rg -A 5 "StreamPriceUpdate" --type ts
Length of output: 12315
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (4)
10-10: LGTM: Clean import addition for price update types.
The import statement is well-structured and correctly imports both the main and SDK types for price updates.
260-262: LGTM: Consistent implementation of market IDs field.
The marketIds field is properly implemented across the interface definition and encoding/decoding logic, following protobuf standards.
Also applies to: 275-277, 1203-1204, 1222-1228, 1258-1270, 1284-1284
1342-1343: LGTM: Well-structured price update field addition.
The priceUpdate field is properly implemented as an optional field with consistent encoding/decoding logic.
Also applies to: 1373-1376, 1413-1416, 1434-1434
Line range hint 1-1434: Verify consistency with proto definitions.
Let's ensure the TypeScript code matches the underlying proto definitions.
✅ Verification successful
Based on the proto file output, I can verify the TypeScript code matches the proto definitions. Let me generate the final response:
TypeScript code matches proto definitions
The TypeScript code correctly implements the proto definitions:
-
StreamOrderbookUpdatesRequestmatches with correct field numbers:clobPairId(1)subaccountIds(2)marketIds(3)
-
StreamUpdatematches with correct field numbers and oneof:blockHeight(1)execMode(2)- oneof
updateMessage:orderbookUpdate(3)orderFill(4)takerOrder(5)subaccountUpdate(6)priceUpdate(7)
All field types, structures and message definitions are consistent between the TypeScript code and proto definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check proto definitions for field numbers and types
# Find and check the corresponding proto file
fd -e proto -x grep -l "StreamOrderbookUpdatesRequest\|StreamUpdate" {} \; | xargs cat
# Verify field numbers and types
rg "market_ids.*=.*3" --type proto
rg "price_update.*=.*7" --type proto
Length of output: 10377
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (12)
83-88: Addition of new imports for the 'prices' module is appropriate.
The newly added imports for ./prices/streaming and ./prices/tx enhance the functionality of the prices module. These additions are correctly implemented.
332-335: Updates to the 'prices' module exports are correct.
The export for the prices module now appropriately includes the new modules _87, _88, _148, _168, and _187. This ensures that all functionalities are properly exported.
337-345: Inclusion of new modules in 'ratelimit' exports is valid.
The ratelimit export now includes the additional modules, which enhances its capabilities. The updates are implemented correctly.
347-354: Revisions to the 'revshare' module exports are appropriate.
The added modules to the revshare export are correctly integrated, ensuring expanded functionality.
356-363: Enhancements to the 'rewards' module exports are correctly implemented.
The export now includes the necessary modules to support new features in the rewards module.
365-370: 'Sending' module exports updated appropriately.
The inclusion of new modules in the sending export is correct and aligns with the changes made in the imports.
372-379: Updates to the 'stats' module exports are valid.
The stats export now properly includes the additional modules, ensuring comprehensive functionality.
381-388: 'Subaccounts' module exports are correctly updated.
The export modifications for the subaccounts module are appropriate and correctly implemented.
390-395: Enhancements to the 'vault' module exports are appropriate.
The updates ensure that all new functionalities in the vault module are properly exported.
400-407: 'Vest' module exports updated correctly.
The vest export now includes the necessary modules, reflecting the changes in the imports.
408-410: ClientFactory exports are properly updated.
The ClientFactory now includes the new modules _195, _196, and _197, ensuring that the client can utilize the latest functionalities.
126-135: Verify potential missing imports due to numbering gap.
There is a gap in the import numbering from _129 to _137, which could indicate that imports for modules _130 to _136 are missing. Please ensure all necessary modules are included.
Run the following script to check for missing module imports in bundle.ts:
| option go_package = "github.com/dydxprotocol/v4-chain/protocol/x/prices/types"; | ||
|
|
||
| // StreamPriceUpdate provides information on a price update. | ||
| message StreamPriceUpdate { |
There was a problem hiding this comment.
Do we need to add this to StagedFinalizeBlockEvent? We probably want price update streams to go through similar flows (store in transient store, stream in Precommit)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
proto/dydxprotocol/clob/streaming.proto (1)
Line range hint
11-18: Consider the impact on stream consumers and performanceThe addition of price updates to the streaming system has several implications to consider:
- Ensure consumers can handle the increased message frequency from price updates
- Consider implementing rate limiting or batching for high-frequency price updates
- Document the expected update frequency for consumers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/x/clob/types/streaming.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/streaming.ts(5 hunks)proto/dydxprotocol/clob/streaming.proto(2 hunks)
🔇 Additional comments (9)
proto/dydxprotocol/clob/streaming.proto (2)
5-5: LGTM: Import statement follows protobuf conventions
The new import for prices streaming proto is properly placed and follows the package structure conventions.
17-17: LGTM: Field addition follows protobuf best practices
The addition of the price_update field to the oneof event is well-structured and maintains backward compatibility. The field number 4 follows the sequential ordering.
Let's verify the field number uniqueness and message structure:
✅ Verification successful
Let me gather more information about the StreamPriceUpdate message structure since the previous command didn't show its definition.
Let me check one more thing to verify the StreamPriceUpdate message import and definition.
Field number and message structure verified successfully
The verification confirms:
- Field number 4 is unique within the StagedFinalizeBlockEvent message
- StreamPriceUpdate message is properly defined in prices/streaming.proto with a clear structure
- The import statement is correctly added to clob/streaming.proto
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify field number uniqueness and message structure
# Check for any other usage of field number 4 in this message
rg -A 10 "StagedFinalizeBlockEvent" | rg "= 4"
# Verify the structure of StreamPriceUpdate message
ast-grep --pattern 'message StreamPriceUpdate {
$$$
}'
Length of output: 205
Script:
#!/bin/bash
# Search for StreamPriceUpdate message definition in prices proto files
fd "prices" --extension proto --exec cat {} | rg -A 10 "message StreamPriceUpdate"
# Also verify there are no other field number 4 usages in the same message
rg -B 5 -A 5 "StagedFinalizeBlockEvent" proto/dydxprotocol/clob/streaming.proto
Length of output: 681
Script:
#!/bin/bash
# Check if the prices/streaming.proto file exists and show its content
fd "streaming.proto" --full-path | grep "prices" | xargs cat
# Verify the import statement is correctly added
rg "import.*prices/streaming.proto" proto/dydxprotocol/clob/streaming.proto
Length of output: 716
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/streaming.ts (7)
3-3: LGTM: Import statement is correctly structured
The new import for StreamPriceUpdate types follows the existing pattern and is properly organized with other streaming-related imports.
12-12: LGTM: Interface updates maintain consistency
The new price update fields are properly added to both interfaces with:
- Correct naming conventions (camelCase/snake_case)
- Consistent optional field marking
- Appropriate type references
Also applies to: 20-20
27-28: LGTM: Base object initialization is consistent
The initialization of the new priceUpdate field follows the established pattern of setting optional fields to undefined.
46-49: LGTM: Protocol Buffer encoding is correctly implemented
The encoding logic for priceUpdate follows the Protocol Buffers wire format specification:
- Correct field number (4)
- Proper wire type for message (2)
- Consistent with other message field encodings
74-77: LGTM: Protocol Buffer decoding is correctly implemented
The decoding logic for priceUpdate is properly implemented:
- Correct field number in switch case
- Proper use of StreamPriceUpdate.decode
- Maintains consistency with other field decodings
92-92: LGTM: Partial object creation is properly implemented
The fromPartial implementation for priceUpdate correctly handles:
- Null/undefined checks
- Type conversion using StreamPriceUpdate.fromPartial
- Consistent with other field implementations
Line range hint 1-96: Verify consistency with source proto file
Since this is generated code, it's important to ensure that these changes align with the corresponding proto file definitions.
✅ Verification successful
Generated code matches proto definition perfectly
The TypeScript code in indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/streaming.ts is correctly generated from the proto definition in proto/dydxprotocol/clob/streaming.proto. The generated interface matches the proto message structure exactly:
- All fields are present with correct types and numbers (orderFill:1, subaccountUpdate:2, orderbookUpdate:3, priceUpdate:4)
- Imports correctly reflect the cross-package references (subaccounts, prices)
- The oneof event semantics is properly translated to optional fields in TypeScript
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the corresponding proto file definition
rg -A 10 "message StagedFinalizeBlockEvent" --type proto
Length of output: 88
Script:
#!/bin/bash
# Let's find proto files without relying on file type
fd -e proto | xargs rg -A 10 "message StagedFinalizeBlockEvent"
Length of output: 771
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
These updates improve the overall functionality and responsiveness of the service for users tracking market prices.