Move FinalizeBlock event staging logic into a generic EventStager #2435
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
teddyding
force-pushed
the
td/event-stager
branch
from
579734c to
01d9cc2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- protocol/finalizeblock/event_stager.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (6 hunks)
- protocol/streaming/noop_streaming_manager.go (0 hunks)
- protocol/streaming/types/interface.go (0 hunks)
- protocol/x/clob/types/expected_keepers.go (0 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (0 hunks)
- protocol/x/subaccounts/types/types.go (0 hunks)
💤 Files with no reviewable changes (5)
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/types/interface.go
- protocol/x/clob/types/expected_keepers.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/types/types.go
🔇 Additional comments (6)
protocol/streaming/full_node_streaming_manager.go (6)
23-24: Addition of thefinalizeblockimport is appropriateThe new import of the
finalizeblockpackage is necessary for managing finalize block events. This addition is appropriate and correctly placed.
66-66: Introduction offinalizeBlockStagerfieldAdding the
finalizeBlockStagerfield to theFullNodeStreamingManagerImplstruct enhances the management of finalize block events. This integration is appropriate and aligns with the objectives of the PR.
124-129: Correct initialization offinalizeBlockStagerThe
finalizeBlockStageris properly initialized usingfinalizeblock.NewEventStager. The parameters provided ensure that it functions as intended.
408-408: Staging subaccount update eventsThe use of
sm.finalizeBlockStager.StageFinalizeBlockEventcorrectly stages subaccount update events for processing after block finalization.
558-558: Staging orderbook update eventsThe call to
sm.finalizeBlockStager.StageFinalizeBlockEventappropriately stages orderbook updates for post-finalization processing.
633-633: Staging order fill eventsUsing
sm.finalizeBlockStager.StageFinalizeBlockEventto stage order fill events aligns with the new event staging logic and is correctly implemented.
| @@ -710,32 +691,6 @@ func getStreamUpdatesForSubaccountUpdates( | |||
| return streamUpdates, subaccountIds | |||
| } | |||
|
|
|||
| // SendFinalizedSubaccountUpdates groups subaccount updates by their subaccount ids and | |||
| // sends messages to the subscribers. | |||
| func (sm *FullNodeStreamingManagerImpl) SendFinalizedSubaccountUpdates( | |||
There was a problem hiding this comment.
This method and below interface are unused and should've been deleted (as part of internalize PR)
| sm.finalizeBlockStager.StageFinalizeBlockEvent( | ||
| ctx, | ||
| sm.cdc.MustMarshal(&stagedEvent), | ||
| ) |
There was a problem hiding this comment.
meganit: the Getter returns []T so let's also pass in T instead of []byte for consistency
|
|
||
| // GetStagedFinalizeBlockEventsFromStore retrieves all staged events from the store. | ||
| func (s EventStager[T]) GetStagedFinalizeBlockEventsFromStore( | ||
| store storetypes.KVStore, |
There was a problem hiding this comment.
also meganit: take in ctx and move the free gas meter here for consistency w setter method
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
protocol/finalizeblock/event_stager.go (2)
15-21: LGTM: EventStager struct is well-designed.The use of generics with the
proto.Messageconstraint is a good choice for type-safe event handling. The struct fields provide all necessary components for event staging and retrieval.Consider adding a brief comment for each field to explain their purpose, which would enhance the struct's documentation.
23-36: LGTM: NewEventStager constructor is well-implemented.The constructor correctly initializes all fields of the EventStager struct.
Consider adding validation for input parameters, especially for
transientStoreKeyandcdc, to ensure they are not nil. This would prevent potential runtime panics. For example:func NewEventStager[T proto.Message]( transientStoreKey storetypes.StoreKey, cdc codec.BinaryCodec, stagedEventCountKey string, stagedEventKeyPrefix string, ) EventStager[T] { if transientStoreKey == nil { panic("transientStoreKey cannot be nil") } if cdc == nil { panic("cdc cannot be nil") } return EventStager[T]{ transientStoreKey: transientStoreKey, cdc: cdc, stagedEventCountKey: stagedEventCountKey, stagedEventKeyPrefix: stagedEventKeyPrefix, } }protocol/streaming/full_node_streaming_manager.go (4)
124-129: LGTM: Proper initialization of finalizeBlockStagerThe initialization of
finalizeBlockStagerin theNewFullNodeStreamingManagerfunction is correct and ensures that the EventStager is ready for use. This aligns well with the PR's objective of implementing a generic EventStager for FinalizeBlock events.Consider extracting the
NewEventStagerparameters into named constants at the package level for improved readability and maintainability. For example:const ( StagedEventsCountKey = "StagedEventsCount" StagedEventsKeyPrefix = "StagedEvents/" )Then use these constants in the
NewEventStagercall.
408-411: LGTM: Proper use of finalizeBlockStager for subaccount updatesThe modification to use
finalizeBlockStager.StageFinalizeBlockEventfor staging subaccount updates is correct and aligns with the new generic EventStager approach. The DeliverTx mode check ensures that only finalized updates are staged.Consider adding error handling for the
StageFinalizeBlockEventcall. While it may not return an error in the current implementation, it's a good practice to handle potential future changes. For example:if err := sm.finalizeBlockStager.StageFinalizeBlockEvent(ctx, &stagedEvent); err != nil { sm.logger.Error("Failed to stage finalize block event", "error", err) // Decide if you want to return the error or handle it differently }
556-559: LGTM: Consistent use of finalizeBlockStager for orderbook updatesThe modification to use
finalizeBlockStager.StageFinalizeBlockEventfor staging orderbook updates is correct and consistent with the changes made in other methods. The DeliverTx mode check ensures that only finalized updates are staged.For consistency with the suggested improvement in the SendSubaccountUpdate method, consider adding error handling here as well:
if err := sm.finalizeBlockStager.StageFinalizeBlockEvent(ctx, &stagedEvent); err != nil { sm.logger.Error("Failed to stage finalize block event for orderbook update", "error", err) // Decide if you want to return the error or handle it differently }This maintains consistency across similar operations and improves error handling throughout the codebase.
631-634: LGTM: Consistent use of finalizeBlockStager for orderbook fill updatesThe modification to use
finalizeBlockStager.StageFinalizeBlockEventfor staging orderbook fill updates is correct and consistent with the changes made in other methods. The DeliverTx mode check ensures that only finalized updates are staged.For consistency with the suggested improvements in other methods, consider adding error handling here as well:
if err := sm.finalizeBlockStager.StageFinalizeBlockEvent(ctx, &stagedEvent); err != nil { sm.logger.Error("Failed to stage finalize block event for orderbook fill update", "error", err) // Decide if you want to return the error or handle it differently }This maintains consistency across similar operations and improves error handling throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- protocol/finalizeblock/event_stager.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (6 hunks)
🔇 Additional comments (6)
protocol/finalizeblock/event_stager.go (3)
1-13: LGTM: Package declaration and imports are appropriate.The package name
finalizeblockand the imported packages align well with the file's purpose of managing events during the FinalizeBlock process.
38-56:⚠️ Potential issueConsider improving error handling and gas usage in GetStagedFinalizeBlockEvents.
While the method effectively retrieves staged events, there are a few areas for improvement:
The use of an infinite gas meter (
NewFreeInfiniteGasMeter()) bypasses gas consumption checks. This could potentially lead to security vulnerabilities or unintended behaviors in gas accounting.The use of
MustUnmarshalcan cause panics if the data is invalid or corrupted. This could lead to stability issues in production.To address these concerns:
Consider using a regular context with gas metering, or document why infinite gas is necessary here.
Replace
MustUnmarshalwith regularUnmarshaland handle potential errors gracefully. For example:if err := s.cdc.Unmarshal(bytes, event); err != nil { // Handle the error appropriately, such as logging or returning it // For example: return nil, fmt.Errorf("failed to unmarshal event: %w", err) }These changes will improve the robustness and safety of the code.
1-83: Overall assessment: Good implementation with room for improvement.The
EventStagerprovides a solid foundation for managing events during the FinalizeBlock process. The use of generics and the overall structure are commendable. However, there are several areas where the code could be improved:
- Error handling: Replace
MustUnmarshalandMustMarshalwith their non-panicking counterparts and handle errors explicitly.- Gas usage: Reconsider the use of infinite gas meters or provide clear documentation justifying their necessity.
- Input validation: Add checks for nil inputs in the constructor and methods.
- Documentation: Enhance inline comments, especially for struct fields.
Addressing these points will significantly improve the robustness and maintainability of the code.
To ensure that the
EventStageris being used correctly throughout the codebase, please run the following verification script:This script will help identify any potential misuse or inconsistencies in how the
EventStageris being utilized across the project.protocol/streaming/full_node_streaming_manager.go (3)
23-24: LGTM: New import and struct field align with PR objectivesThe addition of the
finalizeblockpackage import and the newfinalizeBlockStagerfield in theFullNodeStreamingManagerImplstruct are well-aligned with the PR's objective of creating a genericEventStager. This change should enhance the modularity and reusability of the event handling logic across all modules.Also applies to: 65-66
Line range hint
1-1000: Overall LGTM: Successful implementation of generic EventStagerThe changes in this file successfully implement the generic EventStager for managing FinalizeBlock events across multiple methods. This aligns well with the PR objectives of enhancing modularity and reusability of event handling logic.
Key improvements:
- Consistent use of
finalizeBlockStagerfor staging events in DeliverTx mode.- Proper initialization of the EventStager in the constructor.
- Consistent checks for DeliverTx mode before staging events.
Suggestions for further improvement:
- Implement consistent error handling across all methods using the EventStager.
- Consider extracting some constants for improved readability.
- Replace the panic in
GetStagedFinalizeBlockEventswith more graceful error handling.These changes significantly enhance the structure and maintainability of the event handling system in the FullNodeStreamingManager.
418-431:⚠️ Potential issueAvoid using
panic; handle nil events gracefullyWhile the use of
finalizeBlockStager.GetStagedFinalizeBlockEventsis correct, the panic on encountering a nil event could lead to application instability. It's better to handle this scenario more gracefully to maintain application robustness.Consider modifying the code to log the error and continue processing:
results := make([]clobtypes.StagedFinalizeBlockEvent, len(events)) for i, event := range events { if event == nil { - panic("Got nil event from finalizeBlockStager") + sm.logger.Error("Received nil event from finalizeBlockStager", "index", i) + continue } results[i] = *event } return resultsThis approach logs the error and skips the nil event, allowing the function to return the successfully retrieved events without crashing the application.
| // StageFinalizeBlockEvent stages an event in the transient store. | ||
| func (s EventStager[T]) StageFinalizeBlockEvent( | ||
| ctx sdk.Context, | ||
| stagedEvent T, | ||
| ) { | ||
| noGasCtx := ctx.WithGasMeter(ante_types.NewFreeInfiniteGasMeter()) | ||
| store := noGasCtx.TransientStore(s.transientStoreKey) | ||
|
|
||
| // Increment events count. | ||
| count := s.getStagedEventsCount(store) | ||
| store.Set([]byte(s.stagedEventCountKey), lib.Uint32ToKey(count+1)) | ||
|
|
||
| // Store events keyed by index. | ||
| store = prefix.NewStore(store, []byte(s.stagedEventKeyPrefix)) | ||
| store.Set(lib.Uint32ToKey(count), s.cdc.MustMarshal(stagedEvent)) | ||
| } |
There was a problem hiding this comment.
Improve error handling and consider gas usage in StageFinalizeBlockEvent.
The method effectively stages events, but there are areas for improvement:
-
The use of an infinite gas meter (
NewFreeInfiniteGasMeter()) bypasses gas consumption checks. This could potentially lead to security vulnerabilities or unintended behaviors in gas accounting. -
The use of
MustMarshalcan cause panics if marshaling fails. This could lead to stability issues in production. -
There's no validation of the input event.
To address these concerns:
-
Consider using a regular context with gas metering, or document why infinite gas is necessary here.
-
Replace
MustMarshalwith regularMarshaland handle potential errors gracefully. For example:
bytes, err := s.cdc.Marshal(stagedEvent)
if err != nil {
// Handle the error appropriately, such as logging or returning it
return fmt.Errorf("failed to marshal event: %w", err)
}
store.Set(lib.Uint32ToKey(count), bytes)- Add validation for the input event to ensure it's not nil:
if stagedEvent == nil {
return fmt.Errorf("staged event cannot be nil")
}These changes will improve the robustness and safety of the code.
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
) (cherry picked from commit 53b298b)
…ckport #2435) (#2436) Co-authored-by: Teddy Ding <[email protected]>
Changelist
Create a new
EventStagerstruct that manages the staging and retrieval ofFinalizeBlockevent, so that it can be used by all modules.Test Plan
Tested FNS via localnet.
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
New Features
EventStagerfor efficient staging and retrieval of events during the FinalizeBlock process.FullNodeStreamingManagerwith a new dependency on thefinalizeblockpackage.Bug Fixes
Documentation
SendFinalizedSubaccountUpdatesmethod across various components.