[CT-1178] Only emit finalized subaccount update #2181
Conversation
WalkthroughThe changes involve updates across multiple files, including the addition of new branch triggers in GitHub Actions workflows and modifications to method signatures in various interfaces and implementations. Enhancements have been made to the handling of subaccount updates and snapshots, particularly focusing on ensuring that updates are sent in a finalized state. These updates aim to improve overall functionality and streamline processes within the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Workflow
participant StreamingManager
participant Keeper
User->>Workflow: Trigger on branch td/*
Workflow->>StreamingManager: InitializeNewStreams(subaccountSnapshots)
StreamingManager->>Keeper: GetSubaccountSnapshotsForInitStreams()
Keeper-->>StreamingManager: Return subaccount snapshots
StreamingManager-->>Workflow: Complete initialization
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, codebase verification and nitpick comments (1)
protocol/x/clob/abci.go (1)
Line range hint
127-252: Approved: Changes toPrepareCheckStatefunction.The modifications in the
PrepareCheckStatefunction, including the addition ofsubaccountSnapshotsand its conditional initialization, are aligned with the PR's objectives to handle subaccount updates more effectively. The function's updated signature and the integration of the new variable into the stream initialization process are well-implemented.However, it is crucial to ensure that these changes are covered by unit tests to verify their functionality and integration with existing systems.
Would you like assistance in writing the unit tests for these changes, or should I open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/protocol-build-and-push-snapshot.yml (1 hunks)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/mocks/ClobKeeper.go (2 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
- protocol/streaming/noop_streaming_manager.go (2 hunks)
- protocol/streaming/types/interface.go (2 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/types/types.go (1 hunks)
Additional comments not posted (21)
protocol/streaming/types/interface.go (3)
25-25: Approved change but verify integration.The change from a function to a direct map for subaccount snapshots in
InitializeNewStreamsshould improve efficiency. However, ensure that all parts of the system that interact with this method are updated to handle this new method signature.Run the following script to verify the integration:
Verification successful
Integration verified successfully.
The change to use a direct map for subaccount snapshots in
InitializeNewStreamshas been consistently applied across the codebase. All relevant parts of the system appear to be updated to handle the new method signature.
- Files updated include:
protocol/x/clob/abci.go,protocol/x/clob/keeper/keeper.go,protocol/x/clob/types/clob_keeper.go,protocol/streaming/noop_streaming_manager.go,protocol/streaming/types/interface.go,protocol/mocks/ClobKeeper.go, andprotocol/streaming/full_node_streaming_manager.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all parts of the system that interact with `InitializeNewStreams` are updated. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'InitializeNewStreams'Length of output: 3859
29-31: Approved addition but verify implementation.The addition of
GetSubaccountSnapshotsForInitStreamsis a positive step towards a more structured handling of subaccount data. Ensure that this method is properly implemented across all classes that implement this interface.Run the following script to verify the implementation:
Verification successful
Verification successful: Method implemented across relevant classes.
The method
GetSubaccountSnapshotsForInitStreamsis implemented in multiple classes, ensuring its integration into the system. It is actively used, as evidenced by its invocation inprotocol/x/clob/abci.go. No further action is required.
- Implementations found in:
protocol/x/clob/keeper/keeper.goprotocol/streaming/noop_streaming_manager.goprotocol/streaming/full_node_streaming_manager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `GetSubaccountSnapshotsForInitStreams` is implemented across all classes that use this interface. # Test: Search for the method implementation. Expect: Implementations in all classes that use the interface. rg --type go -A 5 $'GetSubaccountSnapshotsForInitStreams'Length of output: 2838
48-48: Approved renaming but verify usage.Renaming
SendSubaccountUpdatestoSendFinalizedSubaccountUpdatesaligns with the PR's objective to ensure only finalized updates are emitted. Verify that this change is consistently applied across the system.Run the following script to verify the usage:
Verification successful
Renaming consistently applied across the codebase.
The method
SendFinalizedSubaccountUpdateshas been consistently renamed and applied throughout the codebase, aligning with the PR's objective to ensure only finalized updates are emitted. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `SendFinalizedSubaccountUpdates` is used consistently across the system. # Test: Search for the method usage. Expect: Only occurrences of the new name. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
protocol/x/clob/types/expected_keepers.go (1)
88-88: Method Renaming ApprovedThe renaming of
SendSubaccountUpdatestoSendFinalizedSubaccountUpdatesis clear and aligns with the PR's objectives to ensure only finalized updates are emitted. This change should help in reducing unnecessary emissions and improving system efficiency.Run the following script to verify that all references to the old method name are updated:
Verification successful
Method Renaming Verified
The renaming of
SendSubaccountUpdatestoSendFinalizedSubaccountUpdateshas been successfully implemented across the codebase. The new method name is present in multiple files, confirming that all references have been updated. This change aligns with the PR's objectives to ensure only finalized updates are emitted, improving system efficiency.
- Verified occurrences in:
protocol/x/subaccounts/types/types.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/clob/types/expected_keepers.goprotocol/streaming/noop_streaming_manager.goprotocol/streaming/full_node_streaming_manager.goprotocol/streaming/types/interface.goprotocol/lib/metrics/metric_keys.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to `SendSubaccountUpdates` are updated to `SendFinalizedSubaccountUpdates`. # Test: Search for the old function name. Expect: No occurrences. rg --type go -A 5 $'SendSubaccountUpdates'Length of output: 41
Script:
#!/bin/bash # Description: Verify that the new function name `SendFinalizedSubaccountUpdates` is present in the codebase. # Test: Search for the new function name. Expect: At least one occurrence. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
protocol/x/clob/types/clob_keeper.go (1)
140-143: Method Parameter Addition ApprovedThe addition of the
subaccountSnapshotsparameter toInitializeNewStreamsenhances the method's functionality by allowing it to process subaccount snapshots alongside the context. This change is likely necessary for handling more complex operations related to stream initialization.Run the following script to verify that all implementations of the
ClobKeeperinterface are updated to accommodate the new parameter:Verification successful
Verification Successful: Method Parameter Addition
All implementations and interface definitions of the
InitializeNewStreamsmethod have been updated to include thesubaccountSnapshotsparameter, confirming that the change has been consistently applied across the codebase.
protocol/x/clob/abci.go: Method call includes the new parameter.protocol/x/clob/types/clob_keeper.go: Method signature includes the new parameter.protocol/x/clob/keeper/keeper.go: Method implementation includes the new parameter.protocol/mocks/ClobKeeper.go: Mock implementation includes the new parameter.protocol/streaming/noop_streaming_manager.go: Method implementation includes the new parameter.protocol/streaming/full_node_streaming_manager.go: Method implementation includes the new parameter.protocol/streaming/types/interface.go: Interface definition includes the new parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all implementations of the `ClobKeeper` interface accommodate the new parameter in `InitializeNewStreams`. # Test: Search for implementations of the `InitializeNewStreams` method. Expect: All implementations include the new parameter. rg --type go -A 5 $'InitializeNewStreams'Length of output: 3859
protocol/lib/metrics/metric_keys.go (1)
74-74: New Metric Key Addition ApprovedThe addition of the
GrpcSendFinalizedSubaccountUpdatesLatencymetric key is a positive change that enhances the granularity of metrics available for monitoring gRPC interactions, particularly in the context of subaccount updates. This addition aligns with the system's focus on finalized updates.Run the following script to verify that the new metric key is being used in the codebase:
Verification successful
Verification Successful: Metric Key Usage Confirmed
The new metric key
GrpcSendFinalizedSubaccountUpdatesLatencyis being utilized in the codebase, specifically in theprotocol/streaming/full_node_streaming_manager.gofile. This confirms its integration into the system for monitoring purposes.
- File:
protocol/streaming/full_node_streaming_manager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new metric key `GrpcSendFinalizedSubaccountUpdatesLatency` is being used in the codebase. # Test: Search for the new metric key. Expect: At least one occurrence. rg --type go -A 5 $'GrpcSendFinalizedSubaccountUpdatesLatency'Length of output: 1275
.github/workflows/protocol-build-and-push.yml (1)
9-9: Approved: New trigger pattern added.The addition of the
'td/*'trigger pattern is approved as it allows for more flexible workflow execution. However, it's important to monitor the impact this might have on CI resources, considering the potential increase in workflow executions.Consider monitoring the CI system for any significant changes in resource utilization or queuing times.
.github/workflows/protocol-build-and-push-snapshot.yml (1)
9-9: Approved: New trigger pattern added.The addition of the
'td/*'trigger pattern is approved as it allows for more flexible workflow execution. However, it's important to monitor the impact this might have on CI resources, considering the potential increase in workflow executions.Consider monitoring the CI system for any significant changes in resource utilization or queuing times.
protocol/x/clob/keeper/keeper.go (2)
255-272: Well-implemented function for fetching subaccount snapshots.The use of a callback function to fetch subaccount updates from
subaccountsKeeperis a flexible and clean approach, allowing for easy modifications if the fetching logic needs to change in the future.
Line range hint
276-289: Efficient use of pre-fetched subaccount snapshots.Accepting
subaccountSnapshotsas a parameter is a significant improvement that enhances the modularity and performance of stream initialization. This change allows the function to operate without the overhead of fetching snapshots internally, which can be critical in performance-sensitive operations.protocol/streaming/full_node_streaming_manager.go (4)
59-60: Improved initialization logic inOrderbookSubscription.Replacing
sync.Oncewith*atomic.Boolsimplifies the initialization logic, allowing for more straightforward checks of the subscription's state. This change enhances the clarity and efficiency of the subscription initialization process.
79-81: Correct implementation ofIsInitialized.The function correctly checks the initialization state using the new
initializedfield. This method provides a clean and straightforward public interface for checking the subscription's state.
519-535: Enhanced control inSendFinalizedSubaccountUpdates.Renaming the method to
SendFinalizedSubaccountUpdatesand adding a panic condition to ensure it is only called inExecModeFinalizeare significant improvements. These changes enhance the robustness of the method by enforcing stricter control over its execution context, which is crucial for maintaining the integrity of the application's logic.
682-703: Useful addition ofGetSubaccountSnapshotsForInitStreams.The method enhances the functionality of the streaming manager by allowing it to collect snapshots for uninitialized subscriptions. This addition is particularly useful for optimizing the initialization process of streams, ensuring that only necessary data is fetched and processed.
protocol/x/subaccounts/keeper/subaccount.go (1)
828-837: Robust implementation ofSendFinalizedSubaccountUpdates.Renaming the method to
SendFinalizedSubaccountUpdatesand including a check to ensure it is only called inDeliverTxmode are significant improvements. These changes enhance the robustness of the method by enforcing stricter control over its execution context, which is crucial for maintaining the integrity of the application's logic.protocol/x/clob/keeper/process_operations.go (1)
61-61: Verify subaccount update handling after function removal.The removal of
fetchSubaccountIdsInvolvedInOpQueuesimplifies the code but requires verification to ensure that subaccount updates are still correctly handled. Please confirm that all necessary updates are managed by other parts of the system or through new mechanisms introduced in this PR.protocol/mocks/ClobKeeper.go (1)
709-711: Approve the function signature change and verify dependent code.The addition of
subaccountSnapshotstoInitializeNewStreamsenhances its functionality. Please ensure that all dependent code has been updated to accommodate this change.Verification successful
Function signature change verified; ensure all dependent code is updated.
The addition of
subaccountSnapshotstoInitializeNewStreamshas been propagated to several key areas in the codebase. Please manually verify that all dependent code, including tests and indirect usages, has been updated to accommodate this change.
- Instances found in:
protocol/x/clob/abci.goprotocol/x/clob/keeper/keeper.goprotocol/x/clob/types/clob_keeper.goprotocol/streaming/noop_streaming_manager.goprotocol/streaming/types/interface.goprotocol/streaming/full_node_streaming_manager.goprotocol/mocks/ClobKeeper.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all dependent code has been updated to accommodate the new parameter in InitializeNewStreams. # Test: Search for usages of InitializeNewStreams to ensure the new parameter is being passed correctly. rg --type go -A 5 $'InitializeNewStreams'Length of output: 3859
protocol/x/subaccounts/types/types.go (1)
80-80: Method Renaming Approved: Reflects Finalized UpdatesThe renaming of
SendSubaccountUpdatestoSendFinalizedSubaccountUpdatesis approved as it aligns with the PR's objective to emit only finalized updates. This change should help in reducing unnecessary emissions and improving the system's efficiency.Run the following script to verify the impact of this change across the system:
Verification successful
Method Renaming Verified: Successful Integration Across Codebase
The method
SendFinalizedSubaccountUpdateshas been successfully integrated across the codebase, replacing the old method name. This confirms that the renaming was comprehensive and aligns with the intended functionality changes.
- The method is found in multiple files, including
types.go,subaccount.go,expected_keepers.go,metric_keys.go,noop_streaming_manager.go,full_node_streaming_manager.go, andinterface.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of renaming `SendSubaccountUpdates` to `SendFinalizedSubaccountUpdates` across the system. # Test: Search for the old method usage. Expect: No occurrences of the old method name. rg --type go -A 5 $'SendSubaccountUpdates'Length of output: 41
Script:
#!/bin/bash # Description: Verify the integration of the new method name `SendFinalizedSubaccountUpdates` across the system. # Test: Search for the new method usage. Expect: Occurrences of the new method name. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
protocol/streaming/noop_streaming_manager.go (3)
65-69: New Method Added: GetSubaccountSnapshotsForInitStreamsThe addition of the
GetSubaccountSnapshotsForInitStreamsmethod is approved as it enhances the functionality of theNoopGrpcStreamingManagerby providing a mechanism to obtain snapshots for initializing streams. This should be crucial for managing state in streaming operations.
73-73: Signature Change Approved: InitializeNewStreamsThe signature change in the
InitializeNewStreamsmethod, where the parametergetSubaccountSnapshothas been replaced withsubaccountSnapshots, is approved. This change indicates a shift from a function-based approach to a more direct data structure approach for handling subaccount snapshots, potentially improving efficiency and clarity.
54-54: Method Renaming Approved: Reflects Finalized UpdatesThe renaming of
SendSubaccountUpdatestoSendFinalizedSubaccountUpdatesis approved as it aligns with the PR's objective to emit only finalized updates. This change should help in reducing unnecessary emissions and improving the system's efficiency.Run the following script to verify the impact of this change across the system:
Verification successful
Renaming and Integration Verified: Method Transition Successful
The method
SendFinalizedSubaccountUpdatesis correctly integrated across the codebase, replacing the old method name. The renaming aligns with the intended functionality and is consistently applied in various contexts, confirming the successful transition. No further action is required.
- Files where the new method is used:
protocol/x/subaccounts/types/types.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/clob/types/expected_keepers.goprotocol/lib/metrics/metric_keys.goprotocol/streaming/full_node_streaming_manager.goprotocol/streaming/noop_streaming_manager.goprotocol/streaming/types/interface.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of renaming `SendSubaccountUpdates` to `SendFinalizedSubaccountUpdates` across the system. # Test: Search for the old method usage. Expect: No occurrences of the old method name. rg --type go -A 5 $'SendSubaccountUpdates'Length of output: 41
Script:
#!/bin/bash # Description: Verify the usage of the new method `SendFinalizedSubaccountUpdates` across the system. # Test: Search for the new method usage. Expect: Occurrences of the new method name. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
teddyding
force-pushed
the
td/fns-subaccounts-finalize-only
branch
2 times, most recently
from
92726f3
to
a3401b8
Compare
teddyding
force-pushed
the
td/fns-subaccounts-finalize-only
branch
from
a3401b8
to
fb9309d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- protocol/mocks/ClobKeeper.go (2 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
- protocol/streaming/noop_streaming_manager.go (2 hunks)
- protocol/streaming/types/interface.go (2 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- protocol/mocks/ClobKeeper.go
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/types/interface.go
- protocol/x/clob/abci.go
- protocol/x/clob/keeper/keeper.go
- protocol/x/clob/types/clob_keeper.go
Additional comments not posted (7)
protocol/streaming/full_node_streaming_manager.go (7)
Line range hint
1-6: Approved import changes.The addition of
sync/atomicaligns with the changes to useatomic.Boolfor managing initialization state inOrderbookSubscription.
59-60: Structural change approved:OrderbookSubscriptioninitialization.Replacing
sync.Oncewith*atomic.Boolfor theinitializedfield simplifies the initialization check and ensures thread safety without the overhead of a function call each time the state is checked.
79-81: Method implementation approved:IsInitialized.The method correctly uses
atomic.Bool.Load()to check the initialization state, ensuring thread safety and efficiency.
167-167: Initialization logic approved inSubscribe.Setting
initializedto&atomic.Bool{}ensures that each new subscription starts in an uninitialized state, aligning with the new management strategy.
519-534: Method changes approved:SendFinalizedSubaccountUpdates.Renaming the method and adding a panic condition to ensure it is only called in
ExecModeFinalizeare crucial for enforcing the intended behavior of only emitting finalized subaccount updates.
682-704: New method approved:GetSubaccountSnapshotsForInitStreams.This method efficiently handles the retrieval of subaccount snapshots for subscriptions that are not yet initialized, facilitating the initial setup of streams and ensuring that all necessary data is available before they become active.
722-756: Initialization logic approved inInitializeNewStreams.Using
initialized.Swap(true)to set the state simplifies the initialization logic and provides a clear, thread-safe method to manage the subscription's state. This approach is more straightforward and efficient compared to the previoussync.Oncemethod.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
protocol/streaming/full_node_streaming_manager.go (1)
682-704: New Method:GetSubaccountSnapshotsForInitStreamsThis new method supports the initialization of streams with subaccount snapshots, enhancing the functionality of the streaming manager. It iterates through subscriptions and collects snapshots for those not yet initialized, which is crucial for the PR's focus on finalized updates.
Ensure that the method handles potential race conditions or inconsistencies during snapshot collection, possibly by locking mechanisms or revisiting the logic for concurrent access.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/mocks/ClobKeeper.go (2 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
- protocol/streaming/noop_streaming_manager.go (2 hunks)
- protocol/streaming/types/interface.go (2 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/types/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- protocol/lib/metrics/metric_keys.go
- protocol/mocks/ClobKeeper.go
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/types/interface.go
- protocol/x/clob/abci.go
- protocol/x/clob/keeper/keeper.go
- protocol/x/clob/types/expected_keepers.go
- protocol/x/subaccounts/types/types.go
Additional comments not posted (6)
protocol/x/clob/types/clob_keeper.go (1)
140-143: Method Signature Change:InitializeNewStreamsThe method
InitializeNewStreamsnow includes an additional parameter to handle subaccount snapshots. This change is aligned with the PR's objectives to ensure that only finalized updates are processed. Ensure that all implementations of this interface are updated to accommodate this new parameter.Run the following script to verify the method usage:
Verification successful
Verification Successful: Method Signature Updated
All implementations of the
InitializeNewStreamsmethod have been updated to include the new parametersubaccountSnapshots, as required. The changes align with the PR's objectives to process only finalized updates.
protocol/mocks/ClobKeeper.goat line 710protocol/x/clob/keeper/keeper.goat line 276Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `InitializeNewStreams` are updated. # Test: Search for the method implementation. Expect: Only occurrences of the new signature. ast-grep --lang go --pattern $'func ($_ $_) InitializeNewStreams($_, $_) { $$$ }'Length of output: 1534
protocol/streaming/full_node_streaming_manager.go (1)
519-535: Method Renaming and Enhancement:SendFinalizedSubaccountUpdatesThe method
SendSubaccountUpdateshas been renamed toSendFinalizedSubaccountUpdatesto better reflect its purpose of handling only finalized subaccount updates. This change includes a panic condition to ensure it is only called inExecModeFinalize, which enforces stricter control over its execution context.Run the following script to verify the method usage:
Verification successful
Verification Successful:
SendFinalizedSubaccountUpdatesUsageThe method
SendFinalizedSubaccountUpdatesis correctly used withExecModeFinalize. The codebase includes checks to ensure that the execution mode issdk.ExecModeFinalizewhen this method is called, aligning with the intended functionality.
- The panic condition in the method enforces this requirement.
- The
ctx.ExecMode()method is used appropriately to pass the execution mode.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `SendFinalizedSubaccountUpdates` are in the correct execution mode. # Test: Search for the method usage. Expect: Only occurrences in `ExecModeFinalize`. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
Script:
#!/bin/bash # Verify the definition and usage of `ctx.ExecMode()` to ensure it returns `ExecModeFinalize`. # Search for the definition of `ExecMode` in the sdk package. rg --type go 'ExecMode' -A 3 # Search for the implementation of `ExecMode()` method in the sdk.Context. rg --type go 'ExecMode()' -A 3Length of output: 31841
protocol/x/subaccounts/keeper/subaccount.go (1)
828-837: Method Renaming and Logic Enhancement:SendFinalizedSubaccountUpdatesThe method
SendSubaccountUpdateshas been renamed toSendFinalizedSubaccountUpdates, and now includes a check to assertDeliverTxmode before proceeding. This change ensures that updates are only emitted when they are finalized and the system is in the correct state, aligning with the PR's objectives.Run the following script to verify the method usage:
Verification successful
Verification Successful:
SendFinalizedSubaccountUpdatesMethod UsageThe method
SendFinalizedSubaccountUpdatesis correctly used with aDeliverTxmode check in its direct call. Other occurrences in the codebase are not direct calls and do not require this check. The review comment's concern is addressed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `SendFinalizedSubaccountUpdates` check for `DeliverTx` mode. # Test: Search for the method usage. Expect: Only occurrences with `DeliverTx` mode check. rg --type go -A 5 $'SendFinalizedSubaccountUpdates'Length of output: 5341
protocol/x/clob/keeper/process_operations.go (3)
Line range hint
31-115: Review ofProcessProposerOperations: Simplification and Focus on Finalized Updates.The changes in
ProcessProposerOperationsreflect the PR's objective to only emit finalized subaccount updates. The removal of thefetchSubaccountIdsInvolvedInOpQueuefunction simplifies the handling of these updates, potentially reducing complexity and improving performance. This aligns well with the PR's goals of enhancing efficiency and accuracy by focusing on finalized states.
Line range hint
117-248: Review ofProcessInternalOperations: Comprehensive Handling of Operations.
ProcessInternalOperationseffectively handles various types of operations, ensuring that each is processed correctly based on its type. The function's implementation appears robust, with appropriate error handling and state updates. This is crucial for maintaining the integrity of the system's state.
61-61: Clarification needed on the comment regarding optimistic orderbook updates.The comment at line 61 mentions reverting optimistic orderbook updates during the
CheckTxphase. It would be beneficial to clarify whether this change is directly related to the removal of thefetchSubaccountIdsInvolvedInOpQueuefunction and how it impacts the overall system behavior.
| // Whether the subscription is initialized with snapshot. | ||
| initialized *atomic.Bool |
There was a problem hiding this comment.
Refactor: Initialization Tracking in OrderbookSubscription
The OrderbookSubscription struct now uses *atomic.Bool for the initialized field, replacing the previous *sync.Once. This change simplifies the initialization logic and is more appropriate for the use case where the initialization state needs to be checked and updated multiple times.
Consider adding more detailed comments explaining why atomic.Bool is preferred over sync.Once in this context, especially for future maintainers.
Also applies to: 79-81
| // Initialize the subscription with orderbook snapshots. | ||
| initialize *sync.Once | ||
| // Whether the subscription is initialized with snapshot. | ||
| initialized *atomic.Bool |
There was a problem hiding this comment.
Switched to atomic.Bool since I need to query it here
| time.Now(), | ||
| ) | ||
|
|
||
| if execMode != sdk.ExecModeFinalize { | ||
| panic("SendFinalizedSubaccountUpdates should only be called in ExecModeFinalize") |
There was a problem hiding this comment.
nit: should panic be before defer?
| // 2. A new subaccount is subscribed to by a new subscription. | ||
| // 3. InitializeNewStreams is called. | ||
| // Then the new subaccount would not be included in the snapshot. | ||
| // We are okay with this behavior. |
There was a problem hiding this comment.
In this case, the client would be notified of the subaccount's position on the next update since it won't be sent out in the snapshot right?
Changelist
Context
Test Plan
Ran client against dev5
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
Summary by CodeRabbit
New Features
'td/*'pattern.Improvements
Bug Fixes
Documentation