Skip to content
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

Stage clob pair creation side effect #2443

Closed
wants to merge 0 commits into from
Closed

Stage clob pair creation side effect #2443

wants to merge 0 commits into from

Conversation

teddyding
Copy link
Contributor

@teddyding teddyding commented Oct 2, 2024

Changelist

[Describe or list the changes made in this PR]

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • New Features

    • Introduced the EventStager type for improved event management during the FinalizeBlock process.
    • Added Protocol Buffers definition for ClobStagedFinalizeBlockEvent to handle CLOB events during block finalization.
  • Bug Fixes

    • Removed the non-functional SendFinalizedSubaccountUpdates method across multiple components, streamlining the update process.
  • Refactor

    • Enhanced the FullNodeStreamingManagerImpl to utilize the new finalizeBlockStager for event staging, improving clarity and maintainability.
    • Streamlined the CreateClobPair function by integrating it with CreatePerpetualClobPair, reducing complexity.
  • Documentation

    • Added comments for better understanding of the event handling capabilities in the streaming manager.

Copy link
Contributor

coderabbitai bot commented Oct 2, 2024

Walkthrough

The changes introduce a new EventStager type in event_stager.go, designed for managing events during the FinalizeBlock process in a blockchain system. This type includes methods for staging and retrieving events, which are integrated into the FullNodeStreamingManagerImpl class. Several methods related to subaccount updates and event staging have been removed or refactored across multiple files, streamlining the event handling process. The modifications also include the removal of unused methods and updates to interfaces to reflect these changes.

Changes

File Path Change Summary
protocol/finalizeblock/event_stager.go Added EventStager type with methods for staging and retrieving events during FinalizeBlock.
protocol/streaming/full_node_streaming_manager.go Introduced finalizeBlockStager field, refactored event staging methods to use EventStager.
protocol/streaming/noop_streaming_manager.go Removed SendFinalizedSubaccountUpdates method from NoopGrpcStreamingManager.
protocol/streaming/types/interface.go Removed SendFinalizedSubaccountUpdates method from FullNodeStreamingManager interface.
protocol/x/clob/types/expected_keepers.go Removed SendFinalizedSubaccountUpdates method from SubaccountsKeeper interface.
protocol/x/subaccounts/keeper/subaccount.go Removed SendFinalizedSubaccountUpdates method from Keeper struct.
protocol/x/subaccounts/types/types.go Removed SendFinalizedSubaccountUpdates method from SubaccountsKeeper interface.
proto/dydxprotocol/clob/finalize_block.proto Added Protocol Buffers definition for ClobStagedFinalizeBlockEvent message.
protocol/x/clob/abci.go Added call to keeper.ProcessStagedFinalizeBlockEvents(ctx) in Precommit function.
protocol/x/clob/keeper/clob_pair.go Updated CreatePerpetualClobPair and added StageNewClobPairSideEffects method for event staging.
protocol/x/clob/keeper/keeper.go Added finalizeBlockEventStager field and ProcessStagedFinalizeBlockEvents method.
protocol/x/listing/keeper/listing.go Simplified CreateClobPair function by replacing manual construction with CreatePerpetualClobPair.
protocol/x/listing/types/expected_keepers.go Removed CreateClobPairStructures method and added StageNewClobPairSideEffects method.

Possibly related PRs

Suggested labels

pml

Suggested reviewers

  • jayy04
  • vincentwschau

🐇 In the land of code, where changes abound,
An EventStager hops, with events all around.
With methods to stage and to fetch with delight,
The streaming manager shines, oh what a sight!
No more empty calls, just clarity and cheer,
In the blockchain's flow, the future is near! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@teddyding teddyding marked this pull request as ready for review October 2, 2024 23:32
@teddyding teddyding requested a review from a team as a code owner October 2, 2024 23:32
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9fecfc5 and ff9eb5f.

📒 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 (9)
protocol/finalizeblock/event_stager.go (3)

43-44: Verify the use of an infinite gas meter in context.

The use of ante_types.NewFreeInfiniteGasMeter() to create a context with an infinite gas meter may have unintended side effects or security implications. Please ensure that this approach is safe and does not introduce vulnerabilities or performance issues in the execution environment.


61-66: Ensure countsBytes has the correct length before processing.

When using binary.BigEndian.Uint32(countsBytes), the countsBytes slice must be at least 4 bytes long. If it's shorter, this will cause a panic. Verify that countsBytes has the expected length before attempting to convert it.

Apply this diff to add a length check:

 countsBytes := store.Get([]byte(s.stagedEventCountKey))
 if countsBytes == nil {
     return 0
 }
+if len(countsBytes) < 4 {
+    // Handle the unexpected length appropriately
+    return 0 // or return an error if necessary
+}
 return binary.BigEndian.Uint32(countsBytes)

73-74: Review the use of an infinite gas meter when staging events.

Similar to the retrieval method, using ante_types.NewFreeInfiniteGasMeter() in StageFinalizeBlockEvent may have unintended consequences. Please confirm that this is safe within the application's context and does not expose any risks.

protocol/streaming/full_node_streaming_manager.go (6)

23-24: Importing the finalizeblock package

The addition of the finalizeblock import is appropriate for utilizing the EventStager functionality introduced in this PR.


66-66: Adding finalizeBlockStager to the struct

The finalizeBlockStager field is correctly added to the FullNodeStreamingManagerImpl struct to manage event staging.


124-129: Initializing finalizeBlockStager correctly

The finalizeBlockStager is properly initialized using finalizeblock.NewEventStager with the correct parameters. This ensures that staged events are managed consistently.


408-410: Staging subaccount updates

The SendSubaccountUpdate method correctly stages subaccount update events using finalizeBlockStager.StageFinalizeBlockEvent. This aligns with the new event staging mechanism.


556-558: Staging orderbook updates

The SendOrderbookUpdates method correctly stages orderbook update events using finalizeBlockStager.StageFinalizeBlockEvent. This modification aligns with the refactored event handling.


631-633: Staging orderbook fill events

The SendOrderbookFillUpdate method appropriately stages orderbook fill events with the finalizeBlockStager. This ensures that orderbook fills are handled in the finalized event flow.


// Store events keyed by index.
store = prefix.NewStore(store, []byte(s.stagedEventKeyPrefix))
store.Set(lib.Uint32ToKey(count), s.cdc.MustMarshal(stagedEvent))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use Marshal instead of MustMarshal to handle errors gracefully.

Using s.cdc.MustMarshal(stagedEvent) will panic if marshaling fails. To improve error handling, consider using s.cdc.Marshal and managing any potential errors without causing a panic.

Apply this diff to handle marshaling errors:

-store.Set(lib.Uint32ToKey(count), s.cdc.MustMarshal(stagedEvent))
+bytes, err := s.cdc.Marshal(stagedEvent)
+if err != nil {
+    // Handle the error, such as logging or returning
+    return // or handle according to the application's error management strategy
+}
+store.Set(lib.Uint32ToKey(count), bytes)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
store.Set(lib.Uint32ToKey(count), s.cdc.MustMarshal(stagedEvent))
bytes, err := s.cdc.Marshal(stagedEvent)
if err != nil {
// Handle the error, such as logging or returning
return // or handle according to the application's error management strategy
}
store.Set(lib.Uint32ToKey(count), bytes)

for i := uint32(0); i < count; i++ {
event := newStagedEvent()
bytes := store.Get(lib.Uint32ToKey(i))
s.cdc.MustUnmarshal(bytes, event)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use Unmarshal instead of MustUnmarshal to handle errors gracefully.

The method s.cdc.MustUnmarshal(bytes, event) will panic if unmarshaling fails. To enhance robustness, it's advisable to handle unmarshaling errors explicitly using s.cdc.Unmarshal, allowing the program to manage errors without panicking.

Apply this diff to handle unmarshaling errors:

-s.cdc.MustUnmarshal(bytes, event)
+if err := s.cdc.Unmarshal(bytes, event); err != nil {
+    // Handle the error appropriately, such as logging it or returning it
+    continue // or break, return, etc., based on desired behavior
+}

Committable suggestion was skipped due to low confidence.

Comment on lines 51 to 53
bytes := store.Get(lib.Uint32ToKey(i))
s.cdc.MustUnmarshal(bytes, event)
events[i] = event
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential nil values when retrieving events from the store.

When retrieving bytes with store.Get(lib.Uint32ToKey(i)), there's a possibility that the returned value is nil. Using s.cdc.MustUnmarshal(bytes, event) without checking may cause a panic if bytes is nil. Consider adding a nil check before unmarshaling to prevent potential runtime errors.

Apply this diff to handle nil values safely:

 bytes := store.Get(lib.Uint32ToKey(i))
+if bytes == nil {
+    continue // or handle the nil case appropriately
+}
 s.cdc.MustUnmarshal(bytes, event)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bytes := store.Get(lib.Uint32ToKey(i))
s.cdc.MustUnmarshal(bytes, event)
events[i] = event
bytes := store.Get(lib.Uint32ToKey(i))
if bytes == nil {
continue // or handle the nil case appropriately
}
s.cdc.MustUnmarshal(bytes, event)
events[i] = event

Comment on lines 78 to 82
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Update the event count after successfully storing the event.

Currently, the event count is incremented before the event is stored. If an error occurs during store.Set for the event, the count would have been incremented inaccurately. Consider updating the event count only after the event has been successfully stored to maintain consistency.

Apply this diff to adjust the order of operations:

 // Store events keyed by index.
 store = prefix.NewStore(store, []byte(s.stagedEventKeyPrefix))
 store.Set(lib.Uint32ToKey(count), s.cdc.MustMarshal(stagedEvent))

-// Increment events count.
-count := s.getStagedEventsCount(store)
-store.Set([]byte(s.stagedEventCountKey), lib.Uint32ToKey(count+1))
+
+// Increment events count after successful storage.
+store = noGasCtx.TransientStore(s.transientStoreKey)
+store.Set([]byte(s.stagedEventCountKey), lib.Uint32ToKey(count+1))

Committable suggestion was skipped due to low confidence.

Comment on lines 418 to 431
events := sm.finalizeBlockStager.GetStagedFinalizeBlockEvents(
ctx,
func() *clobtypes.StagedFinalizeBlockEvent {
return &clobtypes.StagedFinalizeBlockEvent{}
},
)
results := make([]clobtypes.StagedFinalizeBlockEvent, len(events))
for i, event := range events {
if event == nil {
panic("Got nil event from finalizeBlockStager")
}
results[i] = *event
}
return results
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid panicking when encountering nil events

The code currently panics if a nil event is retrieved from finalizeBlockStager, which can cause the node to crash unexpectedly. It's better to handle this scenario gracefully.

Consider logging the error and skipping the nil event:

 for i, event := range events {
     if event == nil {
-        panic("Got nil event from finalizeBlockStager")
+        sm.logger.Error("Received nil event from finalizeBlockStager")
+        continue
     }
     results[i] = *event
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
events := sm.finalizeBlockStager.GetStagedFinalizeBlockEvents(
ctx,
func() *clobtypes.StagedFinalizeBlockEvent {
return &clobtypes.StagedFinalizeBlockEvent{}
},
)
results := make([]clobtypes.StagedFinalizeBlockEvent, len(events))
for i, event := range events {
if event == nil {
panic("Got nil event from finalizeBlockStager")
}
results[i] = *event
}
return results
events := sm.finalizeBlockStager.GetStagedFinalizeBlockEvents(
ctx,
func() *clobtypes.StagedFinalizeBlockEvent {
return &clobtypes.StagedFinalizeBlockEvent{}
},
)
results := make([]clobtypes.StagedFinalizeBlockEvent, len(events))
for i, event := range events {
if event == nil {
sm.logger.Error("Received nil event from finalizeBlockStager")
continue
}
results[i] = *event
}
return results

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
proto/dydxprotocol/clob/finalize_block.proto (1)

8-13: LGTM: Well-structured message definition with room for expansion.

The ClobStagedFinalizeBlockEvent message is well-defined:

  • It uses a oneof field, allowing for future event types to be added easily.
  • The current event type create_clob_pair is clearly named and typed.

Consider expanding the comment to provide more context:

- // ClobStagedFinalizeBlockEvent defines a CLOB event staged during FinalizeBlock.
+ // ClobStagedFinalizeBlockEvent defines a CLOB (Central Limit Order Book) event
+ // staged during the FinalizeBlock step. It can contain various types of events,
+ // currently supporting the creation of a new CLOB pair.
protocol/x/clob/abci.go (1)

Line range hint 1-301: Suggest additional testing and documentation for the new functionality.

While the change to the Precommit function is minimal and focused, it introduces new behavior in the ABCI lifecycle. To ensure robustness and maintainability:

  1. Add unit tests specifically for the ProcessStagedFinalizeBlockEvents functionality in the context of the Precommit function.
  2. Update the module's documentation to reflect this new step in the ABCI lifecycle, explaining when and why staged events are processed.
  3. Consider adding logging or metrics to track the number and types of events processed during this step, which could be valuable for monitoring and debugging.

Would you like assistance in drafting test cases or documentation updates for this change?

protocol/x/clob/keeper/clob_pair.go (1)

207-207: Consider adding explanatory comment for lib.AssertDeliverTxMode(ctx)

While the assertion ensures the context is in DeliverTx mode, adding a comment explaining why this assertion is necessary can improve code readability and maintainability.

Apply this diff to add an explanatory comment:

 func (k Keeper) StageNewClobPairSideEffects(ctx sdk.Context, clobPair types.ClobPair) error {
+    // Ensure that this function is only called during transaction delivery.
     lib.AssertDeliverTxMode(ctx)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ff9eb5f and 3b92b79.

⛔ Files ignored due to path filters (1)
  • protocol/x/clob/types/finalize_block.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (6)
  • proto/dydxprotocol/clob/finalize_block.proto (1 hunks)
  • protocol/x/clob/abci.go (1 hunks)
  • protocol/x/clob/keeper/clob_pair.go (3 hunks)
  • protocol/x/clob/keeper/keeper.go (4 hunks)
  • protocol/x/listing/keeper/listing.go (1 hunks)
  • protocol/x/listing/types/expected_keepers.go (1 hunks)
🧰 Additional context used
🪛 buf
proto/dydxprotocol/clob/finalize_block.proto

7-7: syntax error: unexpected int literal

(COMPILE)

🔇 Additional comments (7)
proto/dydxprotocol/clob/finalize_block.proto (2)

1-7: LGTM: File structure and imports are well-defined.

The file structure follows Protocol Buffers best practices:

  • Proto3 syntax is used.
  • Package is clearly defined.
  • Necessary import for clob_pair.proto is included.
  • The go_package option is correctly specified for Go code generation.

This structure promotes good organization and modularity in the project.

🧰 Tools
🪛 buf

7-7: syntax error: unexpected int literal

(COMPILE)


8-8: ⚠️ Potential issue

Remove unexpected character on line 8.

There's an unexpected "3" on line 8 which doesn't belong in the proto file. This is causing a syntax error detected by the static analysis tool.

Please remove this character:

-3

This will resolve the syntax error and ensure the proto file is correctly formatted.

Likely invalid or redundant comment.

protocol/x/listing/types/expected_keepers.go (1)

38-38: Approve the interface change and verify related updates.

The change from CreateClobPairStructures to StageNewClobPairSideEffects aligns with the PR objectives and suggests a more staged approach to CLOB pair creation. This is a good improvement in terms of separation of concerns.

To ensure this change is consistently applied throughout the codebase:

Please ensure that:

  1. The StageNewClobPairSideEffects method is properly implemented in the concrete ClobKeeper struct.
  2. All calls to the former CreateClobPairStructures method are updated to use StageNewClobPairSideEffects.
  3. Any relevant tests are updated to reflect this change in functionality.
✅ Verification successful

Change successfully applied and verified.

All references to CreateClobPairStructures have been removed and replaced with StageNewClobPairSideEffects. The new method is properly implemented and utilized throughout the codebase, with no remaining references or issues found in test files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new method and check for any remaining references to the old method.

# Test 1: Search for implementations and calls of the new method
echo "Searching for StageNewClobPairSideEffects implementations and calls:"
rg --type go "StageNewClobPairSideEffects"

# Test 2: Search for any remaining references to the old method
echo "Searching for any remaining CreateClobPairStructures references:"
rg --type go "CreateClobPairStructures"

# Test 3: Look for potential missed updates in test files
echo "Searching for potential missed updates in test files:"
rg --type go "CreateClobPairStructures" -g "*_test.go"

Length of output: 1039

protocol/x/clob/abci.go (1)

54-55: LGTM. Verify implications of processing staged events.

The addition of keeper.ProcessStagedFinalizeBlockEvents(ctx) in the Precommit phase is a logical enhancement to ensure staged events are finalized before proceeding to the next block. This change aligns with the PR objective of handling CLOB pair creation side effects.

Consider the following:

  1. Verify that this change doesn't significantly impact the performance of the Precommit phase.
  2. Ensure that the ProcessStagedFinalizeBlockEvents method handles errors internally or consider adding error handling here if necessary.
  3. Update relevant documentation to reflect this change in the ABCI lifecycle.

To verify the impact and usage of this new method, you can run:

protocol/x/clob/keeper/clob_pair.go (3)

72-76: LGTM: Correctly staging side effects in DeliverTx mode

The addition of code to stage side effects when in DeliverTx mode ensures that side effects are only applied upon transaction commitment, aligning with best practices for transactional integrity.


87-106: Event addition for perpetual market creation looks good

The addition of the indexer event for perpetual market creation is correctly implemented, with all parameters appropriately passed to NewPerpetualMarketCreateEvent. This ensures that the indexer is notified of new market creations.


Line range hint 252-271: Removal of unused ctx parameter is appropriate

The ctx sdk.Context parameter has been removed from SetClobPairIdForPerpetual as it was unused. Simplifying the function signature enhances code clarity.

Comment on lines 92 to 100
k.ClobKeeper.CreatePerpetualClobPair(
ctx,
clobPairId,
perpetualId,
satypes.BaseQuantums(types.DefaultStepBaseQuantums),
types.DefaultQuantumConversionExponent,
types.SubticksPerTick_LongTail,
clobtypes.ClobPair_STATUS_ACTIVE,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle possible error from CreatePerpetualClobPair

The call to k.ClobKeeper.CreatePerpetualClobPair(...) does not capture or handle any potential errors that this function might return. If CreatePerpetualClobPair can return an error, it's important to check and handle it to ensure robustness and prevent unexpected behavior.

Apply this diff to handle the potential error:

+	err = k.ClobKeeper.CreatePerpetualClobPair(
+		ctx,
+		clobPairId,
+		perpetualId,
+		satypes.BaseQuantums(types.DefaultStepBaseQuantums),
+		types.DefaultQuantumConversionExponent,
+		types.SubticksPerTick_LongTail,
+		clobtypes.ClobPair_STATUS_ACTIVE,
+	)
+	if err != nil {
+		return 0, err
+	}
-	k.ClobKeeper.CreatePerpetualClobPair(
-		ctx,
-		clobPairId,
-		perpetualId,
-		satypes.BaseQuantums(types.DefaultStepBaseQuantums),
-		types.DefaultQuantumConversionExponent,
-		types.SubticksPerTick_LongTail,
-		clobtypes.ClobPair_STATUS_ACTIVE,
-	)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
k.ClobKeeper.CreatePerpetualClobPair(
ctx,
clobPairId,
perpetualId,
satypes.BaseQuantums(types.DefaultStepBaseQuantums),
types.DefaultQuantumConversionExponent,
types.SubticksPerTick_LongTail,
clobtypes.ClobPair_STATUS_ACTIVE,
)
err = k.ClobKeeper.CreatePerpetualClobPair(
ctx,
clobPairId,
perpetualId,
satypes.BaseQuantums(types.DefaultStepBaseQuantums),
types.DefaultQuantumConversionExponent,
types.SubticksPerTick_LongTail,
clobtypes.ClobPair_STATUS_ACTIVE,
)
if err != nil {
return 0, err
}


switch event := stagedEvent.Event.(type) {
case *types.ClobStagedFinalizeBlockEvent_CreateClobPair:
k.ApplySideEffectsForCNewlobPair(ctx, *event.CreateClobPair)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the function name 'ApplySideEffectsForCNewlobPair'

The function name ApplySideEffectsForCNewlobPair appears to have a typo. It should likely be ApplySideEffectsForNewClobPair or ApplySideEffectsForCreateClobPair.

Please correct the function name to the intended ApplySideEffectsForCreateClobPair to ensure the method is called correctly.

 case *types.ClobStagedFinalizeBlockEvent_CreateClobPair:
-    k.ApplySideEffectsForCNewlobPair(ctx, *event.CreateClobPair)
+    k.ApplySideEffectsForCreateClobPair(ctx, *event.CreateClobPair)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
k.ApplySideEffectsForCNewlobPair(ctx, *event.CreateClobPair)
k.ApplySideEffectsForCreateClobPair(ctx, *event.CreateClobPair)

streamingManager streamingtypes.FullNodeStreamingManager
indexerEventManager indexer_manager.IndexerEventManager
streamingManager streamingtypes.FullNodeStreamingManager
finalizeBlockEventStager finalizeblock.EventStager[*types.ClobStagedFinalizeBlockEvent]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Initialize 'finalizeBlockEventStager' in the 'NewKeeper' function

The field finalizeBlockEventStager added to the Keeper struct is not initialized in the NewKeeper constructor. This could lead to a nil pointer dereference when ProcessStagedFinalizeBlockEvents is called.

To fix this, initialize finalizeBlockEventStager in the NewKeeper function, similar to other fields. For example:

 // In the NewKeeper function initialization
 keeper := &Keeper{
     // ... existing fields ...
+    finalizeBlockEventStager: finalizeblock.NewEventStager[*types.ClobStagedFinalizeBlockEvent](),
 }

Committable suggestion was skipped due to low confidence.

Comment on lines 193 to 196
func (k Keeper) ApplySideEffectsForCNewlobPair(
ctx sdk.Context,
clobPair types.ClobPair,
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Typo in function name ApplySideEffectsForCNewlobPair

The function name ApplySideEffectsForCNewlobPair appears to have a typo. It should likely be ApplySideEffectsForNewClobPair.

Apply this diff to correct the function name:

-func (k Keeper) ApplySideEffectsForCNewlobPair(
+func (k Keeper) ApplySideEffectsForNewClobPair(
     ctx sdk.Context,
     clobPair types.ClobPair,
 ) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (k Keeper) ApplySideEffectsForCNewlobPair(
ctx sdk.Context,
clobPair types.ClobPair,
) {
func (k Keeper) ApplySideEffectsForNewClobPair(
ctx sdk.Context,
clobPair types.ClobPair,
) {

Comment on lines 78 to 81
perpetualId, err := clobPair.GetPerpetualId()
if err != nil {
return clobPair, err
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid panicking; handle errors gracefully

Panicking on error from clobPair.GetPerpetualId() can cause the application to crash unexpectedly. It's recommended to handle the error gracefully and return an appropriate error to the caller.

Apply this diff to handle the error:

 if err != nil {
-    panic(err)
+    return clobPair, err
 }

Committable suggestion was skipped due to low confidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant