feat: make PML compatible with OE by staging in-memory CLOB side effects #2447
Conversation
WalkthroughThe changes in this pull request involve significant restructuring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
protocol/lib/tx_mode.go
Outdated
| @@ -12,7 +12,10 @@ func AssertDeliverTxMode(ctx sdk.Context) { | |||
| } | |||
|
|
|||
| func IsDeliverTxMode(ctx sdk.Context) bool { | |||
| return !ctx.IsCheckTx() && !ctx.IsReCheckTx() | |||
| if ctx.IsCheckTx() || ctx.IsReCheckTx() || ctx.ExecMode() == sdk.ExecModeCheck || ctx.ExecMode() == sdk.ExecModeReCheck { | |||
There was a problem hiding this comment.
InitGenesis runs in ExecMode == ModeCheckTx but IsCheckTx == false for some reason.
We don't want to stage events during InitGenesis, because:
- InitGenesis stages the events.
- HydrateClobPair (Preblocker) sees the ClobPair objects and create in memory object
- In Precommit, we attempt to create the
Clobobjects again, but fail because it's already done above.
There was a problem hiding this comment.
Would this condition also avoid simulation?
There was a problem hiding this comment.
I ended up taking a new approach to make changes less disruptive.
Genesis and all test setup calls CreatePerpetualClobPairAndMemStructs, which is essentially the old function that just creates in-memory structs right away. Normal DeliverTx changes should call CreatePerpetualClobPair instead which correctly stage the creation.
teddyding
force-pushed
the
td/pml-stager
branch
from
37a6a8a
to
dc672ad
Compare
protocol/x/listing/keeper/listing.go
Outdated
| return 0, err | ||
| } | ||
| } | ||
| k.ClobKeeper.CreatePerpetualClobPair( |
There was a problem hiding this comment.
Check for error return from this function
teddyding
force-pushed
the
td/pml-stager
branch
from
dc672ad
to
1a9482b
Compare
| @@ -51,6 +51,8 @@ func Precommit( | |||
| ctx sdk.Context, | |||
| keeper keeper.Keeper, | |||
| ) { | |||
| keeper.ProcessStagedFinalizeBlockEvents(ctx) | |||
There was a problem hiding this comment.
Could you add a comment as to why this needs to be here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
proto/dydxprotocol/clob/finalize_block.proto (1)
8-16: LGTM: Well-structured message definition with room for expansion.The
ClobStagedFinalizeBlockEventmessage is well-defined and allows for future expansion of event types. The use ofoneofis appropriate for representing different types of events.Consider enhancing the comment for the
create_clob_pairfield to provide more context:- // create_clob_pair indicates a new CLOB pair creation. + // create_clob_pair indicates a new CLOB (Central Limit Order Book) pair creation event.This change would provide more clarity for developers who might not be familiar with the CLOB acronym.
protocol/x/listing/types/expected_keepers.go (1)
38-40: Approve changes with a suggestion for documentation.The modifications to the
ClobKeeperinterface align well with the PR objectives. The new methodsStageNewClobPairSideEffectsandGetStagedClobFinalizeBlockEventssupport the staging mechanism for ClobPair creation, addressing the issues with in-memory changes during user transactions and optimistic execution.Consider adding brief comments above each new method to explain their purpose and how they relate to the staging mechanism. This would improve the interface's self-documentation. For example:
// StageNewClobPairSideEffects stages the side effects of creating a new ClobPair // without immediately applying them to the in-memory state. StageNewClobPairSideEffects(ctx sdk.Context, clobPair clobtypes.ClobPair) error // GetStagedClobFinalizeBlockEvents returns the staged ClobPair creation events // to be processed during the Precommit phase. GetStagedClobFinalizeBlockEvents(ctx sdk.Context) []*clobtypes.ClobStagedFinalizeBlockEventprotocol/x/clob/types/keys.go (1)
142-147: LGTM! Consider adding a brief comment for clarity.The new constants
StagedEventsCountKeyandStagedEventsKeyPrefixalign well with the PR objectives of staging events for block finalization. They follow the existing naming conventions and are appropriately placed in a new section.Consider adding a brief comment above the new constants to explain their purpose, similar to other sections in this file. For example:
// FinalizeBlock event staging // These constants are used for managing events staged during the block finalization process. const ( StagedEventsCountKey = "StgEvtCnt" StagedEventsKeyPrefix = "StgEvt:" )This addition would improve the overall documentation and make it easier for other developers to understand the purpose of these constants.
protocol/x/clob/keeper/get_price_premium_test.go (1)
191-191: LGTM. Consider updating the test name for clarity.The change from
CreatePerpetualClobPairtoCreatePerpetualClobPairAndMemStructsaligns well with the PR objectives of staging ClobPair creation and handling in-memory structures. This modification enhances the function's description and likely its internal implementation to address the issues mentioned in the PR summary.Consider updating the test function name
TestGetPricePremiumForPerpetualto reflect that it now also tests the creation of memory structures. For example:func TestGetPricePremiumForPerpetualWithMemStructs(t *testing.T) { // ... (existing test code) }This change would make the test name more descriptive of its current functionality.
protocol/testutil/keeper/clob.go (1)
355-355: LGTM. Consider adding a test case for the new behavior.The change from
CreatePerpetualClobPairtoCreatePerpetualClobPairAndMemStructsis consistent with the modification inCreateTestClobPairsand aligns with the PR objectives.Consider adding a test case that verifies the following:
- The ClobPair is correctly created and staged.
- The in-memory structures are properly initialized.
- The changes are only finalized during the
Precommitphase.This will help ensure that the new method behaves as expected in various scenarios, including transaction reversion and optimistic execution.
protocol/x/clob/memclob/memclob_get_impact_price_subticks_test.go (1)
341-341: LGTM. Consider adding a comment explaining the change.The replacement of
CreatePerpetualClobPairwithCreatePerpetualClobPairAndMemStructsaligns with the PR objective of making PML compatible with OE by staging ClobPair creation. This change doesn't affect the test logic or assertions.Consider adding a brief comment explaining why this change was made and how it relates to the PR objectives. This will help future developers understand the reasoning behind this modification.
protocol/mocks/MemClob.go (1)
446-460: LGTM! Consider adding a comment for clarity.The changes to the
MaybeCreateOrderbookmethod are correct and consistent with the updated method signature. The implementation now properly handles the boolean return value and includes appropriate error handling.Consider adding a brief comment explaining the purpose of the boolean return value, for example:
// MaybeCreateOrderbook attempts to create an orderbook for the given ClobPair. // Returns true if the orderbook was created, false if it already existed. func (_m *MemClob) MaybeCreateOrderbook(clobPair clobtypes.ClobPair) bool { // ... (rest of the implementation) }protocol/x/clob/keeper/mev_test.go (1)
Line range hint
1-1433: Summary: Function update enhances CLOB pair creation processThe main changes in this file involve updating the
CreatePerpetualClobPairfunction toCreatePerpetualClobPairAndMemStructs. This modification appears to be part of a larger enhancement to the CLOB system, likely involving improved handling of memory structures during the creation of perpetual CLOB pairs.Key points:
- The function interface remains the same, suggesting backwards compatibility.
- The change is consistent across the file, indicating a systematic update.
- The tests themselves haven't been significantly altered, which implies that the new function's behavior is expected to be similar to the old one.
Consider the following to ensure a smooth transition:
- Update any documentation or comments related to the CLOB pair creation process to reflect the new function and its enhanced capabilities.
- If this change is part of a larger refactoring, ensure that all related components (e.g., other keepers, handlers) are updated consistently.
- Consider adding specific tests for any new functionality introduced by the
CreatePerpetualClobPairAndMemStructsfunction, if not already covered by existing tests.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/finalize_block.ts (1)
5-8: Consider consolidating duplicated documentation commentsThe documentation comments for
ClobStagedFinalizeBlockEventandClobStagedFinalizeBlockEventSDKTypeare identical. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider consolidating the comments or referencing shared documentation.Also applies to: 13-17
protocol/x/clob/keeper/keeper.go (1)
219-246: Enhance error handling inProcessStagedFinalizeBlockEventsWhile the method logs errors when encountering
nilevents or unknown event types, consider implementing additional metrics or alerts to monitor these unexpected occurrences. This can aid in early detection of anomalies in the processing of finalize block events.protocol/x/clob/keeper/clob_pair.go (2)
294-297: Handle potential orderbook creation issues during initializationIn
InitMemClobOrderbooks, the functionMaybeCreateOrderbookis called, but its return value is not checked. If the orderbook creation fails, it could lead to inconsistent state. Consider logging an error or handling the failure case appropriately.func (k Keeper) InitMemClobOrderbooks(ctx sdk.Context) { clobPairs := k.GetAllClobPairs(ctx) for _, clobPair := range clobPairs { // Create the corresponding orderbook in the memclob. - k.MemClob.MaybeCreateOrderbook( - clobPair, - ) + if created := k.MemClob.MaybeCreateOrderbook(clobPair); !created { + dydxlog.ErrorLog( + ctx, + "InitMemClobOrderbooks: Failed to create orderbook for CLOB pair", + "clob_pair", clobPair, + ) + } } }
18-18: Organize imports according to Go conventionsTypically, Go import statements are grouped into standard library imports, third-party imports, and local imports, separated by blank lines. Consider reorganizing the imports for better readability.
protocol/x/clob/keeper/clob_pair_test.go (1)
Line range hint
74-81: Handle the returned error fromCreatePerpetualClobPairAndMemStructsIn this test, the
CreatePerpetualClobPairAndMemStructsfunction returns a result and an error, but the error is not being handled. Although the//nolint: errcheckdirective suppresses the linter warning, it's best practice to handle potential errors to ensure test reliability.Consider capturing and checking the error:
//nolint: errcheck - ks.ClobKeeper.CreatePerpetualClobPairAndMemStructs( + _, err := ks.ClobKeeper.CreatePerpetualClobPairAndMemStructs( ks.Ctx, clobPair.Id, clobtest.MustPerpetualId(clobPair), satypes.BaseQuantums(clobPair.StepBaseQuantums), clobPair.QuantumConversionExponent, clobPair.SubticksPerTick, clobPair.Status, ) + require.NoError(t, err)protocol/x/clob/keeper/deleveraging_test.go (1)
Remaining instances of
CreatePerpetualClobPairfound.The following files still use the old method and need to be updated to
CreatePerpetualClobPairAndMemStructs:
protocol/x/listing/keeper/listing.goprotocol/x/listing/types/expected_keepers.goprotocol/x/clob/types/clob_keeper.goprotocol/x/clob/keeper/clob_pair.goprotocol/x/clob/keeper/msg_server_create_clob_pair.goprotocol/mocks/ClobKeeper.go🔗 Analysis chain
Line range hint
422-429: Verify all instances ofCreatePerpetualClobPairare updated toCreatePerpetualClobPairAndMemStructs.You've updated the method call to
CreatePerpetualClobPairAndMemStructshere, which is correct. To ensure consistency and prevent potential issues, please verify that all other usages ofCreatePerpetualClobPairhave been updated throughout the codebase.Run the following script to check for any remaining usages of the old method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining usages of `CreatePerpetualClobPair`. rg --type go 'CreatePerpetualClobPair\('Length of output: 756
protocol/x/clob/keeper/orders_test.go (1)
Line range hint
1856-1863: Remove or update commented-out test code.The test
TestGetStatePosition_PanicsOnInvalidClobcontains commented-out code that references asset CLOB pairs. Since asset CLOB pairs are not implemented, consider removing this code or updating it once the feature is available.protocol/x/clob/keeper/process_operations_test.go (1)
Line range hint
2464-2469: Consider staging ClobPair creation to reflect the PR's changesIn lines 2464-2469, the test directly calls
ks.ClobKeeper.CreatePerpetualClobPairAndMemStructs(...)to create ClobPairs. Since the PR introduces the use ofFinalizeBlockEventStagerto stage ClobPair creation during theDeliverTxphase, the test should simulate this staging behavior. Updating the test to stage the ClobPair creation events will provide a more accurate verification of the new logic and ensure that in-memory changes are properly applied during thePrecommitphase.protocol/x/clob/keeper/liquidations_test.go (1)
Line range hint
4603-4609: Consider refactoring repeated function calls to reduce duplicationThe calls to
CreatePerpetualClobPairAndMemStructsare repeated multiple times with similar parameters. Consider extracting this logic into a helper function to reduce code duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/x/clob/types/finalize_block.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (27)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/finalize_block.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)
- proto/dydxprotocol/clob/finalize_block.proto (1 hunks)
- protocol/mocks/MemClob.go (1 hunks)
- protocol/testutil/keeper/clob.go (2 hunks)
- protocol/x/clob/abci.go (1 hunks)
- protocol/x/clob/abci_test.go (3 hunks)
- protocol/x/clob/genesis.go (1 hunks)
- protocol/x/clob/keeper/clob_pair.go (7 hunks)
- protocol/x/clob/keeper/clob_pair_test.go (16 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (2 hunks)
- protocol/x/clob/keeper/get_price_premium_test.go (1 hunks)
- protocol/x/clob/keeper/keeper.go (7 hunks)
- protocol/x/clob/keeper/liquidations_test.go (11 hunks)
- protocol/x/clob/keeper/mev_test.go (2 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
- protocol/x/clob/keeper/orders_test.go (9 hunks)
- protocol/x/clob/keeper/process_operations_test.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/memclob/memclob_get_impact_price_subticks_test.go (1 hunks)
- protocol/x/clob/types/keys.go (1 hunks)
- protocol/x/clob/types/memclob.go (1 hunks)
- protocol/x/listing/keeper/listing.go (1 hunks)
- protocol/x/listing/keeper/listing_test.go (4 hunks)
- protocol/x/listing/types/expected_keepers.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
🧰 Additional context used
🪛 buf
proto/dydxprotocol/clob/finalize_block.proto
4-4: import "dydxprotocol/clob/clob_pair.proto": file does not exist
(COMPILE)
🔇 Additional comments (78)
indexer/packages/v4-protos/src/codegen/google/bundle.ts (3)
8-9: LGTM! Verify exported content.The
apiexport has been updated to spread properties from_130and_131, which is consistent with the reordering of imports.To ensure that the exported content remains correct, please run the following script:
#!/bin/bash # Description: Verify the content of the api export # Test: Check the content of the files being spread into the api export. Expect: Correct content from api/annotations and api/http. echo "Content of api/annotations:" cat ./indexer/packages/v4-protos/src/codegen/google/api/annotations.ts echo "\nContent of api/http:" cat ./indexer/packages/v4-protos/src/codegen/google/api/http.ts
11-14: LGTM! Verify exported content, especially the new addition.The
protobufexport has been updated to include_132,_133,_134, and the newly added_135, which corresponds to the new./protobuf/anyimport.To ensure that the exported content is correct, especially the newly added
_135, please run the following script:✅ Verification successful
Failed to locate
any.ts. Generate scripts to find its existence and correct path.
Verified the
protobufexport includes the new_135successfully.The
any.tsfile exists at./indexer/packages/v4-protos/src/codegen/google/protobuf/any.ts, confirming that theprotobufexport is correctly updated with_135.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the content of the protobuf export, focusing on the new addition # Test: Check the content of the files being spread into the protobuf export, with focus on the new any.ts file. Expect: Correct content from all files, including the new any.ts. for file in descriptor duration timestamp any; do echo "Content of protobuf/$file:" cat ./indexer/packages/v4-protos/src/codegen/google/protobuf/$file.ts echo "\n" doneLength of output: 35193
Script:
#!/bin/bash # Description: Search for 'any.ts' file in the codebase to verify its existence and location. # Search for 'any.ts' within the 'google/protobuf' directory and its subdirectories fd 'any.ts' ./indexer/packages/v4-protos/src/codegen/google/protobuf/ # Additionally, search the entire repository in case the file is misplaced fd 'any.ts' .Length of output: 211
1-6: LGTM! Verify usage of the new import.The import statements have been reordered, which improves readability. The addition of
./protobuf/anyimport suggests new functionality or types are being used.Please ensure that the newly imported
anymodule fromprotobufis being used in the codebase. Run the following script to verify its usage:proto/dydxprotocol/clob/finalize_block.proto (2)
2-2: LGTM: Package and option declarations are correct.The package declaration and go_package option are properly set, following the expected conventions for the dydxprotocol project.
Also applies to: 6-6
1-4: Verify the existence of the imported proto file.The import statement for "dydxprotocol/clob/clob_pair.proto" is present, but the static analysis tool couldn't find this file. Please ensure that this file exists in the correct location within your project structure.
Run the following script to check for the existence of the imported file:
✅ Verification successful
Imported proto file confirmed to exist. No further action required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the imported proto file exists # Test: Search for the clob_pair.proto file fd --type file "clob_pair.proto$" # If the file is not found, check for any .proto files in the clob directory if [ $? -ne 0 ]; then echo "clob_pair.proto not found. Checking for any .proto files in clob directory:" fd --type file ".proto$" | grep "/clob/" fiLength of output: 93
🧰 Tools
🪛 buf
4-4: import "dydxprotocol/clob/clob_pair.proto": file does not exist
(COMPILE)
protocol/x/clob/keeper/get_price_premium_test.go (1)
Line range hint
1-235: Overall, the test file is well-structured and comprehensive.The test cases cover various scenarios including success cases, error handling, and edge cases. The mock setup and assertions are thorough, providing good coverage for the
GetPricePremiumForPerpetualfunction.The only change in this file aligns well with the PR objectives, and no further modifications are necessary based on the current PR goals.
protocol/x/clob/abci.go (3)
54-55: LGTM: Change aligns with PR objectivesThe addition of
keeper.ProcessStagedFinalizeBlockEvents(ctx)in thePrecommitfunction aligns well with the PR objectives. This change addresses the issues mentioned in the PR summary by:
- Allowing ClobPair creation events to be staged during the
DeliverTxphase.- Processing these events during the
Precommitphase, ensuring that in-memory side effects are applied at the appropriate time.This implementation should resolve the problems with reverting user transactions and handling optimistically processed blocks.
54-55: Verify integration with ABCI lifecycleWhile the change is well-implemented and isolated to the
Precommitfunction, it's important to ensure that processing staged events at this point in the ABCI lifecycle doesn't have unintended consequences. Please confirm that:
- There are no race conditions or conflicts with operations in
EndBlockerorPrepareCheckState.- The order of operations (staging in
DeliverTxand processing inPrecommit) is correct and doesn't interfere with other module's lifecycle hooks.- Error handling for
ProcessStagedFinalizeBlockEventsis appropriate within thePrecommitcontext.To help verify the integration, you can run the following script to check for any potential conflicts or unexpected interactions:
✅ Verification successful
Verified integration with ABCI lifecycle
ProcessStagedFinalizeBlockEventsis exclusively called within thePrecommitfunction andkeeper.go, ensuring no unintended external interactions.- No modifications to
ClobPairare present in other ABCI functions, confirming the change is isolated.- Error handling for
ProcessStagedFinalizeBlockEventsis appropriately implemented within thePrecommitcontext.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential conflicts or unexpected interactions in ABCI lifecycle # Test 1: Check if ProcessStagedFinalizeBlockEvents is called elsewhere echo "Checking for other calls to ProcessStagedFinalizeBlockEvents:" rg "ProcessStagedFinalizeBlockEvents" --type go # Test 2: Check for other modifications to ClobPair in ABCI functions echo "Checking for ClobPair modifications in other ABCI functions:" rg "ClobPair" --type go | rg "func (PreBlocker|BeginBlocker|EndBlocker|PrepareCheckState)" # Test 3: Check for error handling related to ProcessStagedFinalizeBlockEvents echo "Checking for error handling related to ProcessStagedFinalizeBlockEvents:" rg "ProcessStagedFinalizeBlockEvents" --type go -A 5 -B 5Length of output: 2117
Line range hint
1-255: Summary: Well-implemented change with minimal impactThe modification to the
Precommitfunction in this file is well-focused and addresses the PR objectives effectively. The change is minimal and doesn't affect other ABCI lifecycle functions directly.However, to ensure the robustness of this change in the broader context of the CLOB module, please consider the following:
- Verify that the
ProcessStagedFinalizeBlockEventsmethod in the keeper is implemented correctly and handles all edge cases.- Ensure that this change doesn't introduce any performance bottlenecks in the
Precommitphase.- Update relevant documentation and comments to reflect this new behavior in the ABCI lifecycle.
- Consider adding or updating unit tests to cover this new functionality, especially focusing on the interaction between
DeliverTxandPrecommitphases.To help with a broader verification, you can run the following script:
This broader verification will help ensure that the change is fully integrated and doesn't have any unexpected impacts on the system.
protocol/testutil/keeper/clob.go (2)
Line range hint
1-371: Overall assessment: Changes are consistent and aligned with PR objectives.The modifications in this file are focused and consistent, replacing
CreatePerpetualClobPairwithCreatePerpetualClobPairAndMemStructsin both relevant functions. This change appears to be part of a larger refactoring to address the issues mentioned in the PR description, specifically the staging of ClobPair creation to ensure proper handling of transaction reversion and optimistic execution.To ensure the changes are comprehensive, please run the following command to check for any remaining instances of the old method:
#!/bin/bash # Description: Verify that all instances of CreatePerpetualClobPair have been replaced # Test: Search for any remaining instances of CreatePerpetualClobPair rg --type go $'CreatePerpetualClobPair(?!AndMemStructs)'If this command returns any results, those instances should be reviewed and updated as necessary.
296-296: LGTM. Verify the implementation ofCreatePerpetualClobPairAndMemStructs.The change from
CreatePerpetualClobPairtoCreatePerpetualClobPairAndMemStructsaligns with the PR objectives to stage ClobPair creation. This modification appears to be part of a systematic update to address the issues mentioned in the PR description.To ensure the new method behaves as expected, please run the following verification:
protocol/x/clob/keeper/mev_test.go (2)
1292-1292: LGTM. Verify test coverage.The change from
CreatePerpetualClobPairtoCreatePerpetualClobPairAndMemStructsis consistent with the previous update. This modification enhances the creation process of perpetual CLOB pairs.To ensure that the test coverage remains adequate with the new function, please run the test suite and check the coverage report. You may use a command like:
#!/bin/bash # Run tests and generate coverage report go test -coverprofile=coverage.out ./... go tool cover -html=coverage.out -o coverage.html # Check if the new function is covered grep -n "CreatePerpetualClobPairAndMemStructs" coverage.outReview the coverage report to confirm that the new
CreatePerpetualClobPairAndMemStructsfunction is adequately tested.
907-907: LGTM. Verify new function behavior.The change from
CreatePerpetualClobPairtoCreatePerpetualClobPairAndMemStructslooks good. This update likely includes additional functionality related to memory structures.Please ensure that the new
CreatePerpetualClobPairAndMemStructsfunction behaves as expected and doesn't introduce any unintended side effects. Run the following script to check for any other usages of the old function that might need updating:✅ Verification successful
Verified: No remaining usages of
CreatePerpetualClobPairfound in the production codebase. All necessary updates toCreatePerpetualClobPairAndMemStructshave been successfully applied.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of CreatePerpetualClobPair rg "CreatePerpetualClobPair[^A]" --type goLength of output: 340
Script:
#!/bin/bash # Search for any remaining usage of CreatePerpetualClobPair excluding CreatePerpetualClobPairAndMemStructs rg "CreatePerpetualClobPair(?!AndMemStructs)" --type goLength of output: 324
Script:
#!/bin/bash # Search for any remaining usage of CreatePerpetualClobPair excluding CreatePerpetualClobPairAndMemStructs rg "CreatePerpetualClobPair" --type go | grep -v "CreatePerpetualClobPairAndMemStructs"Length of output: 1557
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/finalize_block.ts (2)
9-12: Property naming conventions are consistentThe property names
createClobPairandcreate_clob_pairin the respective interfaces follow the established naming conventions for Protobuf message types and their SDK representations.Also applies to: 18-21
23-66: Encoding and decoding methods are correctly implementedThe
encode,decode, andfromPartialmethods forClobStagedFinalizeBlockEventare properly implemented and align with Protobuf encoding standards, ensuring accurate serialization and deserialization of the message.protocol/x/listing/keeper/listing.go (3)
96-99: Confirm the suitability of default constants for long-tail markets.The function uses default constants:
types.DefaultStepBaseQuantumstypes.DefaultQuantumConversionExponenttypes.SubticksPerTick_LongTailVerify that these values are appropriate for long-tail or isolated markets as intended and align with the market specifications.
92-101: Proper error handling is implemented.The error returned from
CreatePerpetualClobPairis correctly checked, and the function returns0and the error if an error occurs. This ensures that potential issues are propagated appropriately.
92-101: Verify the necessity of context checks before creating the ClobPair.The updated
CreateClobPairfunction no longer checks if the context is in deliver transaction mode before creating CLOB pair structures. Previously, such checks might have been implemented to prevent in-memory changes from persisting in case of transaction reversion or block discards in an optimistic execution environment. Ensure that this change aligns with the intended behavior and does not introduce side effects when transactions are reverted or blocks are discarded.Run the following script to check for context checks related to
CreateClobPairusage:protocol/x/listing/keeper/listing_test.go (2)
15-15: Import statement is appropriateThe
sdkimport is necessary for usingsdk.ExecModeFinalizein the context configuration.
297-312: Test correctly verifies ClobPair stagingThe test accurately checks that the
ClobPaircreation event is staged whenisDeliverTxistrue, and that no events are staged when it isfalse. This ensures that in-memory state modifications occur only during the appropriate execution mode.protocol/x/clob/keeper/keeper.go (6)
8-8: Importingfinalizeblockpackage approvedThe addition of the
github.com/dydxprotocol/v4-chain/protocol/finalizeblockimport is appropriate for handling finalize block events.
19-19: Aliasdydxlogimport approvedIntroducing the
dydxlogalias improves code readability when logging errors.
108-108: SettingtransientStoreKeyin Keeper struct approvedAssigning
transientStoreKeyto the Keeper struct ensures that transient data is appropriately managed.
137-142: Initialization offinalizeBlockEventStagerwithtransientStoreKeyThe
finalizeBlockEventStageris correctly initialized using thetransientStoreKey. This setup is essential for staging finalize block events during theDeliverTxphase.
211-218: Addition ofGetStagedClobFinalizeBlockEventsmethod approvedThe new method
GetStagedClobFinalizeBlockEventseffectively retrieves staged finalize block events, enhancing the Keeper's event management capabilities.
82-82: Verify replacement ofliquidationsStoreKeywithtransientStoreKeyThe
NewKeeperfunction now acceptstransientStoreKeyinstead ofliquidationsStoreKey. Ensure all instances ofliquidationsStoreKeyhave been updated throughout the codebase to prevent inconsistencies.Run the following script to confirm that
liquidationsStoreKeyis no longer referenced:indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (25)
29-41: Imports for Clob Module Updated CorrectlyThe added imports for the Clob module (lines 29-41) appropriately include new files such as
finalize_block, ensuring the module now covers all necessary components. The module numbering (_33 to _45) is sequential and consistent.
192-194: 'accountplus' Exports Enhanced with New ModulesThe inclusion of
_136,_154, and_175into theaccountplusexport enhances the module by incorporating additional functionalities. This ensures that the new query and transaction RPC services are exposed properly.
200-201: 'affiliates' Exports UpdatedThe addition of
_155and_176to theaffiliatesexport ensures that the query RPC services are correctly integrated.
207-208: 'assets' Module Exports AugmentedIncluding
_137and_156into theassetsexport provides access to the latest LCD and RPC queries, keeping the module up-to-date.
215-217: 'blocktime' Module Enhanced with New ServicesThe
blocktimeexport now includes_138,_157, and_177, adding the latest query and transaction RPC services to the module.
225-227: 'bridge' Exports Extended with Additional RPC ServicesThe addition of
_139,_158, and_178to thebridgeexport incorporates new LCD and RPC services, enhancing the module's capabilities.
244-247: Clob Module Exports Updated with New AdditionsThe inclusion of
_45,_140,_159, and_179into theclobexport ensures that the new transaction services and query endpoints are correctly exposed.
250-254: 'daemons' Namespace StructuringThe
daemonsnamespace now includes:
bridge(line 250)liquidation(line 252)pricefeed(line 254)This structuring enhances modularity and maintainability.
261-264: 'delaymsg' Exports UpdatedThe
delaymsgexport now includes_53,_141,_160, and_180, adding necessary RPC and transaction services.
268-270: 'epochs' Module EnhancementsIncluding
_56,_142, and_161into theepochsexport ensures updated query services are available.
275-276: 'feetiers' Module Updated with New ServicesThe
feetiersexport now includes_60,_143,_162, and_181, providing the latest transaction and query services.
280-285: 'govplus' Module Exports EnhancedThe addition of
_62,_63,_163, and_182to thegovplusexport incorporates new query and transaction services.
287-304: 'indexer' Namespace Modules OrganizedThe
indexernamespace now includes:
events(line 287)indexer_manager(line 289)off_chain_updates(line 291)protocol.v1with submodules (lines 294-297)redis(line 300)shared(line 302)socks(line 304)This reorganization improves the clarity and structure of the
indexernamespace.
310-313: 'listing' Module UpdatedThe
listingexport now includes_77,_144,_164, and_183, ensuring all relevant services are accessible.
319-322: 'perpetuals' Module EnhancedBy adding
_82,_145,_165, and_184to theperpetualsexport, the module now supports the latest transaction and query services.
328-331: 'prices' Module Exports UpdatedIncluding
_87,_146,_166, and_185into thepricesexport ensures up-to-date services are available.
338-341: 'ratelimit' Module AugmentedThe
ratelimitexport now includes_93,_147,_167, and_186, adding essential RPC and transaction functionalities.
347-350: 'revshare' Module Updated with New ServicesAdding
_98,_148,_168, and_187to therevshareexport enhances the module's capabilities.
356-359: 'rewards' Module EnhancementsThe inclusion of
_103,_149,_169, and_188into therewardsexport updates the module with the latest services.
364-366: 'sending' Module UpdatedThe
sendingexport now includes_107,_170, and_189, ensuring all sending-related services are exposed.
372-375: 'stats' Module Exports EnhancedAdding
_112,_150,_171, and_190to thestatsexport updates the module with necessary services.
382-384: 'subaccounts' Module AugmentedThe
subaccountsexport now includes_118,_151, and_172, enhancing its functionality.
391-391: 'vault' Module Exports UpdatedIncluding
_124,_152,_173, and_191into thevaultexport ensures comprehensive service availability.
397-403: 'vest' Module Enhanced with New ExportsThe
vestexport now includes_126,_127,_153,_174, and_192, providing updated transaction and query services.
404-406: 'ClientFactory' Export UpdatedThe
ClientFactorynow includes_193,_194, and_195, ensuring it incorporates all necessary client services.protocol/x/clob/keeper/msg_server_place_order_test.go (2)
Line range hint
189-196: Function call updated correctly to 'CreatePerpetualClobPairAndMemStructs'The function
CreatePerpetualClobPairAndMemStructsis appropriately used here, reflecting the updated implementation that stages ClobPair creation. The parameters passed to the function are correct and align with the new method signature.
Line range hint
340-347: Consistent use of 'CreatePerpetualClobPairAndMemStructs' in test setupThe test case correctly updates the function call to
CreatePerpetualClobPairAndMemStructs, ensuring that in-memory structures are initialized as part of ClobPair creation. This change maintains consistency across your test suite.protocol/x/clob/keeper/clob_pair.go (2)
37-68: Ensure proper usage of the new function in test setups and genesis initializationThe new
CreatePerpetualClobPairAndMemStructsfunction is intended for genesis state initialization and test setups. Verify that it is used exclusively in these contexts to prevent unintended side effects during regular operation.#!/bin/bash # Description: Find all usages of CreatePerpetualClobPairAndMemStructs. rg --type go 'CreatePerpetualClobPairAndMemStructs'Ensure that the function is only called in appropriate places such as genesis initialization and test files.
100-106: Confirm side effects are applied during non-genesis blocksIn
CreatePerpetualClobPair, the conditionif lib.IsDeliverTxMode(ctx)is used to determine whether to stage side effects. Ensure that this condition correctly differentiates between genesis and non-genesis blocks, so that side effects are appropriately staged when required.Run the following script to review the usage of
lib.IsDeliverTxMode:This will help confirm that side effects are consistently applied during transaction delivery and not during genesis initialization.
protocol/x/clob/abci_test.go (3)
Line range hint
533-539: Usage ofCreatePerpetualClobPairAndMemStructsis appropriateThe method
CreatePerpetualClobPairAndMemStructsis correctly used for the BTC CLOB pair, and the arguments passed align with the expected parameters. The error handling withrequire.NoError(t, err)ensures that any issues are properly captured during tests.
Line range hint
566-572: Consistent application ofCreatePerpetualClobPairAndMemStructsThe function
CreatePerpetualClobPairAndMemStructsis appropriately used for the ETH CLOB pair, mirroring its usage with the BTC pair. The consistency in error handling enhances test reliability.
Line range hint
1173-1179: Correct implementation within the loop for CLOB pairsInside the loop over
tc.clobs, the usage ofCreatePerpetualClobPairAndMemStructscorrectly initializes each CLOB pair with the necessary parameters. The error is properly checked usingrequire.NoError(t, err), maintaining consistency across test cases.protocol/x/clob/keeper/deleveraging_test.go (1)
Line range hint
819-826: Duplicate of previous comment regarding method update.This change mirrors the previous update to
CreatePerpetualClobPairAndMemStructs. Ensure consistency across the codebase as mentioned earlier.protocol/x/clob/keeper/orders_test.go (8)
Line range hint
593-601: EnsureCreatePerpetualClobPairAndMemStructsis correctly initializing in-memory structures.The call to
CreatePerpetualClobPairhas been replaced withCreatePerpetualClobPairAndMemStructs. Verify that this new function properly initializes both the on-chain state and the in-memory structures required for the tests to run correctly.
Line range hint
854-862: Check function arguments after replacingCreatePerpetualClobPairwithCreatePerpetualClobPairAndMemStructs.Ensure that all arguments passed to
CreatePerpetualClobPairAndMemStructscorrectly reflect the new function's signature, and that any additional initialization required for the tests is performed.
Line range hint
985-993: Update indexer event expectations due to function change.With the introduction of
CreatePerpetualClobPairAndMemStructs, ensure that the mock expectations forAddTxnEventin the indexer event manager are still accurate, and that all necessary events are being emitted as expected.
Line range hint
1042-1052: Confirm proper context setup after CLOB pair creation.After changing to
CreatePerpetualClobPairAndMemStructs, verify that the context, specifically block height and block time, is appropriately set for the testTestPerformStatefulOrderValidation_PreExistingStatefulOrderto reflect any changes in behavior.
Line range hint
1799-1810: Validate CLOB pair initialization inTestGetStatePosition_Success.Ensure that the new function
CreatePerpetualClobPairAndMemStructsproperly initializes the CLOB pairs for the test cases and that there are no side effects affecting the retrieval of state positions.
Line range hint
2017-2027: Adjust mock expectations forInitStatefulOrderstests.After replacing
CreatePerpetualClobPairwithCreatePerpetualClobPairAndMemStructs, ensure that the mocks for theMemClobandIndexerEventManagerinTestInitStatefulOrdersare correctly set up to reflect any changes in behavior or additional calls made by the new function.
Line range hint
2187-2195: Ensure CLOB pair is properly initialized inTestPlaceStatefulOrdersFromLastBlock.Verify that the CLOB pair is correctly created using
CreatePerpetualClobPairAndMemStructs, and confirm that this change does not introduce any unintended side effects in the test logic.
Line range hint
2317-2325: Verify initialization inTestPlaceConditionalOrdersTriggeredInLastBlock.Check that the use of
CreatePerpetualClobPairAndMemStructsproperly initializes the CLOB pair for conditional order tests and that the test accurately reflects any changes in the initialization process.protocol/x/clob/memclob/memclob.go (1)
161-166: Update toMaybeCreateOrderbookfunction is correct and improves clarityThe
MaybeCreateOrderbookmethod now returns a boolean to indicate whether a new orderbook was created. This change enhances the function's utility by providing immediate feedback on the operation's outcome. The logic correctly returnsfalseif the orderbook already exists andtrueafter creating a new one.protocol/x/clob/keeper/liquidations_test.go (10)
Line range hint
335-341: Function call updated correctlyThe call to
CreatePerpetualClobPairhas been correctly updated toCreatePerpetualClobPairAndMemStructs, and the parameters have been adjusted accordingly.
Line range hint
450-456: Function call updated with test case statusThe function
CreatePerpetualClobPairAndMemStructsis properly called withtc.status, ensuring that the ClobPair is created with the correct status specified in the test case.
Line range hint
1256-1262: Function call updated forconstants.ClobPair_BtcThe function call to
CreatePerpetualClobPairAndMemStructsis correctly updated forconstants.ClobPair_Btc, and parameters are passed appropriately.
Line range hint
1286-1292: Function call updated forconstants.ClobPair_EthThe function call to
CreatePerpetualClobPairAndMemStructsis correctly updated forconstants.ClobPair_Eth.
Line range hint
2196-2202: Function calls updated within loopThe
CreatePerpetualClobPairAndMemStructsfunction is correctly called within the loop to create ClobPairs based on the test data.
Line range hint
2316-2322: Function call updated inTestPlacePerpetualLiquidation_SendOffchainMessagesThe call to
CreatePerpetualClobPairAndMemStructsis correctly updated in theTestPlacePerpetualLiquidation_SendOffchainMessagesfunction.
Line range hint
3855-3861: Function call updated in test setupThe function
CreatePerpetualClobPairAndMemStructsis correctly used when setting up the test conditions.
Line range hint
4839-4845: Function call updated inTestMaybeGetLiquidationOrderThe function call is updated appropriately, and parameters are correctly passed.
Line range hint
5184-5190: Function call updated inTestGetMaxAndMinPositionNotionalLiquidatableThe call to
CreatePerpetualClobPairAndMemStructsis correctly updated in this test function.
Line range hint
5339-5345: Function call updated inTestSortLiquidationOrdersThe function
CreatePerpetualClobPairAndMemStructsis correctly called with appropriate parameters.
| @@ -19,7 +19,7 @@ func InitGenesis(ctx sdk.Context, k *keeper.Keeper, genState types.GenesisState) | |||
| if err != nil { | |||
| panic(errorsmod.Wrap(types.ErrInvalidClobPairParameter, err.Error())) | |||
| } | |||
| _, err = k.CreatePerpetualClobPair( | |||
| _, err = k.CreatePerpetualClobPairAndMemStructs( | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing Implementation of CreateOrderbook Method
The CreatePerpetualClobPairAndMemStructs function in protocol/x/clob/genesis.go calls the CreateOrderbook method of MemClob. However, the CreateOrderbook method was not found in the codebase, indicating it may be missing or not implemented.
- File:
protocol/x/clob/keeper/clob_pair.go - Method Call:
k.MemClob.CreateOrderbook(clobPair)
Please ensure that the CreateOrderbook method is properly implemented within the MemClob struct to handle in-memory order book creation.
🔗 Analysis chain
LGTM. Please verify new method implementation and confirm issue resolution.
The change from CreatePerpetualClobPair to CreatePerpetualClobPairAndMemStructs aligns with the PR objectives of making PML compatible with OE by staging ClobPair creation. This modification appears to address the issues mentioned in the PR description.
To ensure the completeness of this change:
- Please confirm that the
CreatePerpetualClobPairAndMemStructsmethod in the keeper package properly implements the staging of ClobPair creation. - Verify that this change effectively addresses the following issues mentioned in the PR objectives:
- Discarding in-memory changes if the user transaction is reverted.
- Handling the case when the block is processed optimistically and later discarded.
Can you provide more details on how these specific issues are resolved by this change?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the implementation of CreatePerpetualClobPairAndMemStructs
ast-grep --lang go --pattern $'func (k *Keeper) CreatePerpetualClobPairAndMemStructs(
$$$
) ($_, error) {
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Search for the function CreatePerpetualClobPairAndMemStructs across all Go files
rg --type go 'CreatePerpetualClobPairAndMemStructs'
Length of output: 5627
Script:
#!/bin/bash
# Extract the implementation of CreatePerpetualClobPairAndMemStructs function
rg --type go -A 50 'func \(k Keeper\) CreatePerpetualClobPairAndMemStructs' protocol/x/clob/keeper/clob_pair.go
Length of output: 1584
Script:
#!/bin/bash
# Extract the implementation of createPerpetualClobPair function
rg --type go -A 50 'func \(k Keeper\) createPerpetualClobPair' protocol/x/clob/keeper/clob_pair.go
# Extract the implementation of CreateOrderbook method in MemClob
rg --type go -A 50 'func \(m \*MemClob\) CreateOrderbook' protocol/x/clob/keeper/memclob/memclob.go
# Extract the implementation of SetClobPairIdForPerpetual function
rg --type go -A 50 'func \(k Keeper\) SetClobPairIdForPerpetual' protocol/x/clob/keeper/clob_pair.go
Length of output: 3421
Script:
#!/bin/bash
# Search for the CreateOrderbook method across all Go files
rg --type go 'func \(m \*MemClob\) CreateOrderbook'
Length of output: 53
| @@ -23,7 +23,7 @@ type MemClob interface { | |||
| ) | |||
| MaybeCreateOrderbook( | |||
| clobPair ClobPair, | |||
| ) | |||
| ) bool | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Required for Non-Mock Implementations
The MaybeCreateOrderbook method in the MemClob interface has been updated to include a boolean return type. However, no non-mock implementations were found with the updated signature. This indicates that the change is incomplete and may cause inconsistencies or runtime errors.
Action Items:
- Ensure all non-mock implementations of
MemClobupdate theMaybeCreateOrderbookmethod to return a boolean. - Verify that all callers of this method handle the new return value appropriately.
🔗 Analysis chain
Approve change with suggestions for improvement
The addition of a boolean return type to MaybeCreateOrderbook improves the method's expressiveness by allowing it to indicate success or failure. This is a positive change.
However, there are a few points to consider:
-
Please add a comment explaining the meaning of the return value (e.g.,
// Returns true if the orderbook was created, false if it already existed). -
This change modifies the interface contract. Ensure all implementations of
MemClobare updated to return a boolean value. -
Update all callers of this method to handle the new return value appropriately.
To verify the impact of this change, please run the following script:
This script will help identify areas that need to be updated due to the signature change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations and usages of MaybeCreateOrderbook
# Find implementations of MaybeCreateOrderbook
echo "Implementations of MaybeCreateOrderbook:"
rg --type go "func.*MaybeCreateOrderbook.*\(.*\).*bool" -g '!*test*'
# Find calls to MaybeCreateOrderbook
echo "\nCalls to MaybeCreateOrderbook:"
rg --type go "\.MaybeCreateOrderbook\(" -g '!*test*'
Length of output: 567
Script:
#!/bin/bash
# Description: Check for non-mock implementations of MaybeCreateOrderbook and verify their signatures
# Find non-mock implementations of MaybeCreateOrderbook with the boolean return type
echo "Non-mock Implementations of MaybeCreateOrderbook with updated signature:"
rg --type go "func\s+\(?[^ ]+\)?MemClob\.MaybeCreateOrderbook\s*\(.*\)\s*bool" -g '!*mocks*' -g '!*test*'
Length of output: 262
|
|
||
| // FinalizeBlock event staging | ||
| const ( | ||
| StagedEventsCountKey = "StgEvtCnt" | ||
| StagedEventsKeyPrefix = "StgEvt:" | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Constant Definitions Detected
The constants StagedEventsCountKey and StagedEventsKeyPrefix have differing values in protocol/x/clob/types/keys.go and protocol/streaming/constants.go. This inconsistency can lead to unexpected behaviors.
protocol/x/clob/types/keys.go:StagedEventsCountKey = "StgEvtCnt"StagedEventsKeyPrefix = "StgEvt:"
protocol/streaming/constants.go:StagedEventsCountKey = "EvtCnt"StagedEventsKeyPrefix = "Evt:"
Please review and unify the constant definitions to ensure consistency across the codebase.
🔗 Analysis chain
Verify usage of new constants in related files.
The new constants StagedEventsCountKey and StagedEventsKeyPrefix have been added correctly. To ensure they are being used as intended:
Please run the following script to check for the usage of these new constants in other files:
This will help confirm that the new constants are being utilized correctly in the implementation of the event staging functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new constants in related files.
# Test: Search for usage of StagedEventsCountKey
echo "Searching for usage of StagedEventsCountKey:"
rg --type go -A 5 'StagedEventsCountKey'
echo "---"
# Test: Search for usage of StagedEventsKeyPrefix
echo "Searching for usage of StagedEventsKeyPrefix:"
rg --type go -A 5 'StagedEventsKeyPrefix'
Length of output: 2452
| @@ -234,7 +235,7 @@ func TestCreateClobPair(t *testing.T) { | |||
|
|
|||
| // Set deliverTx mode | |||
| if tc.isDeliverTx { | |||
| ctx = ctx.WithIsCheckTx(false).WithIsReCheckTx(false) | |||
| ctx = ctx.WithIsCheckTx(false).WithIsReCheckTx(false).WithExecMode(sdk.ExecModeFinalize) | |||
There was a problem hiding this comment.
Ensure ExecMode is explicitly set when isDeliverTx is false
When isDeliverTx is false, the context only sets IsCheckTx to true. Consider explicitly setting ExecMode to sdk.ExecModeCheck to ensure consistent and predictable execution behavior.
Apply this diff to set the ExecMode explicitly:
if tc.isDeliverTx {
ctx = ctx.WithIsCheckTx(false).WithIsReCheckTx(false).WithExecMode(sdk.ExecModeFinalize)
lib.AssertDeliverTxMode(ctx)
} else {
- ctx = ctx.WithIsCheckTx(true)
+ ctx = ctx.WithIsCheckTx(true).WithExecMode(sdk.ExecModeCheck)
lib.AssertCheckTxMode(ctx)
}Committable suggestion was skipped due to low confidence.
| perpetualId, err := clobPair.GetPerpetualId() | ||
| if err != nil { | ||
| return clobPair, err | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Avoid panicking; return errors instead
In the createPerpetualClobPair function, using panic(err) can cause the application to crash, which is undesirable in production code. It's better to return the error to the caller for proper handling.
Apply this diff to return the error instead of panicking:
func (k Keeper) createPerpetualClobPair(
ctx sdk.Context,
clobPairId uint32,
perpetualId uint32,
stepSizeBaseQuantums satypes.BaseQuantums,
quantumConversionExponent int32,
subticksPerTick uint32,
status types.ClobPair_Status,
) (types.ClobPair, error) {
clobPair := types.ClobPair{
Metadata: &types.ClobPair_PerpetualClobMetadata{
PerpetualClobMetadata: &types.PerpetualClobMetadata{
PerpetualId: perpetualId,
},
},
Id: clobPairId,
StepBaseQuantums: stepSizeBaseQuantums.ToUint64(),
QuantumConversionExponent: quantumConversionExponent,
SubticksPerTick: subticksPerTick,
Status: status,
}
if err := k.ValidateClobPairCreation(ctx, &clobPair); err != nil {
return clobPair, err
}
// Write the `ClobPair` to state.
k.SetClobPair(ctx, clobPair)
perpetualId, err := clobPair.GetPerpetualId()
if err != nil {
- panic(err)
+ return types.ClobPair{}, err
}
perpetual, err := k.perpetualsKeeper.GetPerpetual(ctx, perpetualId)
if err != nil {
return types.ClobPair{}, err
}📝 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.
| perpetualId, err := clobPair.GetPerpetualId() | |
| if err != nil { | |
| return clobPair, err | |
| panic(err) | |
| } | |
| perpetualId, err := clobPair.GetPerpetualId() | |
| if err != nil { | |
| return types.ClobPair{}, err | |
| } |
| if created := k.MemClob.MaybeCreateOrderbook(clobPair); !created { | ||
| dydxlog.ErrorLog( | ||
| ctx, | ||
| "ApplySideEffectsForNewClobPair: Orderbook already exists for CLOB pair", | ||
| "clob_pair", clobPair, | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Return error when orderbook creation fails
In ApplySideEffectsForNewClobPair, if the orderbook already exists, the function logs an error but does not notify the caller. To ensure that such issues are properly handled, consider returning an error.
Apply this diff to return an error:
func (k Keeper) ApplySideEffectsForNewClobPair(
ctx sdk.Context,
clobPair types.ClobPair,
-) {
+) error {
if created := k.MemClob.MaybeCreateOrderbook(clobPair); !created {
dydxlog.ErrorLog(
ctx,
"ApplySideEffectsForNewClobPair: Orderbook already exists for CLOB pair",
"clob_pair", clobPair,
)
- return
+ return fmt.Errorf("orderbook already exists for CLOB pair: %v", clobPair.Id)
}
k.SetClobPairIdForPerpetual(clobPair)
+ return nil
}Remember to update any callers of this function to handle the returned error appropriately.
Committable suggestion was skipped due to low confidence.
| @@ -158,11 +158,12 @@ func (m *MemClobPriceTimePriority) CancelOrder( | |||
| // MaybeCreateOrderbook is used for updating memclob internal data structures to mark an orderbook as created. | |||
| func (m *MemClobPriceTimePriority) MaybeCreateOrderbook( | |||
| clobPair types.ClobPair, | |||
| ) { | |||
| ) (created bool) { | |||
There was a problem hiding this comment.
Handle potential overflow when converting maxMatchSize to uint64
In the resizeReduceOnlyMatchIfNecessary function, the conversion of maxMatchSize (a *big.Int) to uint64 using maxMatchSize.Uint64() might result in incorrect values if maxMatchSize exceeds the maximum value of a uint64. This could lead to unexpected behavior or data loss due to overflow.
Consider adding a check to ensure that maxMatchSize fits within the uint64 range before conversion. If it doesn't, handle the overflow appropriately, such as returning an error or capping the value.
Apply this diff to address the issue:
+ if !maxMatchSize.IsUint64() {
+ // Handle the overflow case appropriately
+ return satypes.BaseQuantums(math.MaxUint64)
+ }
return satypes.BaseQuantums(maxMatchSize.Uint64())Committable suggestion was skipped due to low confidence.
teddyding
changed the title
teddyding
changed the title
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
…cts (backport #2447) (#2459) Co-authored-by: Teddy Ding <[email protected]>
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
Changelist
PML supports user transaction that creates new ClobPair (both in state and in memory objects). Currently there are two issues:
To solve these issues we use the
FinalizeBlockEventStagerto store aClobPaircreation event duringDeliverTx, and apply the in-memory side effects later inPrecommit.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
New Features
ClobStagedFinalizeBlockEventinterface for handling CLOB events during block finalization.ClobStagedFinalizeBlockEventmessage type.Precommitfunction to process staged finalize block events.Changes
CreatePerpetualClobPairAndMemStructsfunctionality.Bug Fixes
MaybeCreateOrderbookmethod to prevent potential issues.