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

[CT-514] Clob MsgBatchCancel functionality #1110

Merged
merged 20 commits into from
Mar 4, 2024

Conversation

jonfung-dydx
Copy link
Contributor

@jonfung-dydx jonfung-dydx commented Feb 28, 2024

Changes included in this PR:

  • go.mod replace included to use new changes here
  • Clob Ante Handler -> Keeper BatchCancel flow implemented
  • Single e2e test for this flow, more to come
  • Various locations in the code (i.e InterfaceRegistries) now account for MsgBatchCancel
  • Proto updates: In the OrderBatch object, repeated fixed32 was having issues deserializing/serializing. Changed to repeated uint32. Context here

Copy link

linear bot commented Feb 28, 2024

Copy link
Contributor

coderabbitai bot commented Feb 28, 2024

Walkthrough

The recent updates enhance the protocol with the MsgBatchCancel functionality, enabling efficient cancellation of batches of short-term orders. These changes span various components, introducing new constants, mock functions, keeper methods, error handling, and tests. Additionally, minor adjustments like formatting changes, debugging logs, and message type handling in existing functions complement the primary feature introduction, aiming to optimize order management and resource utilization within the protocol's ecosystem.

Changes

Files Change Summary
protocol/lib/log/constants.go Added MsgBatchCancel and adjusted formatting.
protocol/mocks/ClobKeeper.go, protocol/x/clob/types/clob_keeper.go Added BatchCancelShortTermOrder function/mock.
protocol/x/clob/ante/clob.go, protocol/x/clob/ante/ante_wrapper.go, protocol/app/process/other_msgs.go Added handling and logging for MsgBatchCancel.
protocol/x/clob/keeper/orders.go Implemented BatchCancelShortTermOrder with error logging and validation.
protocol/x/clob/types/errors.go Added ErrBatchCancelAllFailed error code.
protocol/testutil/app/app.go, protocol/app/module/interface_registry.go, protocol/app/process/utils_disallow_msgs.go, protocol/app/process/utils_disallow_msgs_test.go Enhanced message handling and disallowing logic for MsgBatchCancel.
protocol/x/clob/e2e/batch_cancel_test.go Introduced tests for batch cancel functionality.
protocol/testutil/constants/messages.go, protocol/testutil/encoding/utils.go Added MsgBatchCancel initialization and encoding configuration.
proto/dydxprotocol/clob/tx.proto, indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts Updated OrderBatch client_ids type for space optimization.

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>.
    • Generate unit-tests 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 tests 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

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.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 9bf9875 and 3f61fa0.
Files selected for processing (6)
  • protocol/lib/log/constants.go (1 hunks)
  • protocol/mocks/ClobKeeper.go (1 hunks)
  • protocol/x/clob/ante/clob.go (1 hunks)
  • protocol/x/clob/keeper/orders.go (1 hunks)
  • protocol/x/clob/types/clob_keeper.go (1 hunks)
  • protocol/x/clob/types/errors.go (1 hunks)
Additional comments: 5
protocol/x/clob/types/clob_keeper.go (1)
  • 25-28: The addition of the BatchCancelShortTermOrder method to the ClobKeeper interface correctly implements the required functionality for batch canceling short-term orders.
protocol/x/clob/ante/clob.go (1)
  • 133-151: The implementation for processing MsgBatchCancel messages correctly handles short-term cancels, appropriately skips processing during ReCheckTx, and logs relevant information. This aligns well with the intended functionality and follows best practices.
protocol/x/clob/types/errors.go (1)
  • 214-218: The addition of the ErrBatchCancelAllFailed error with code 46 is well-implemented. It provides clear and robust error handling for scenarios where all order cancels in a batch fail.
protocol/mocks/ClobKeeper.go (1)
  • 53-65: The addition of the BatchCancelShortTermOrder mock function aligns with the introduction of the MsgBatchCancel functionality. This mock will facilitate unit testing of components that depend on the ClobKeeper interface, ensuring they can simulate the behavior of batch canceling short-term orders without requiring an actual implementation. The function signature correctly accepts a types.Context and a pointer to clobtypes.MsgBatchCancel, and it returns an error, which is consistent with the expected behavior for such an operation. The use of mock.Called to simulate the function call and allow for the setup of return values and expectations is correctly implemented.
protocol/x/clob/keeper/orders.go (1)
  • 50-123: The implementation of BatchCancelShortTermOrder introduces a new functionality to cancel a batch of short-term orders. Here are some observations and recommendations:
  1. Correctness and Logic: The function's logic appears to be correct, with appropriate checks for stateful validation and handling of individual order cancellations within a batch. The use of lib.AssertCheckTxMode(ctx) ensures this function is called in the correct transaction context.

  2. Error Handling: The function logs errors silently for individual order cancellations that fail, which is acceptable given the context. However, it's crucial to ensure that these logs are monitored and analyzed to prevent silent failures from going unnoticed.

  3. Performance: The function iterates over each order in the batch and attempts to cancel them individually. While this is necessary for the functionality, it's important to monitor the performance impact of processing large batches, especially in terms of transaction execution time and resource usage.

  4. Maintainability: The function is well-structured and follows a clear logic flow, which aids in maintainability. The comments provide useful context for understanding the function's purpose and behavior.

  5. Security: No immediate security concerns are evident in the implementation. The function performs necessary validations and relies on existing mechanisms for order cancellation.

  6. Best Practices: The function adheres to best practices in error handling and logging. It might be beneficial to include metrics or telemetry data collection for monitoring the usage and performance of this new functionality.

Overall, the BatchCancelShortTermOrder function is a well-implemented addition to the protocol, enhancing its capabilities in managing short-term orders. It's important to consider the potential performance implications and ensure adequate monitoring and logging mechanisms are in place.

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.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 3f61fa0 and ef75e27.
Files ignored due to path filters (2)
  • protocol/go.mod is excluded by: !**/*.mod
  • protocol/go.sum is excluded by: !**/*.sum
Files selected for processing (5)
  • protocol/testutil/app/app.go (2 hunks)
  • protocol/x/clob/ante/ante_wrapper.go (2 hunks)
  • protocol/x/clob/ante/clob.go (6 hunks)
  • protocol/x/clob/e2e/batch_cancel_test.go (1 hunks)
  • protocol/x/clob/keeper/orders.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • protocol/x/clob/ante/clob.go
  • protocol/x/clob/keeper/orders.go
Additional comments: 2
protocol/x/clob/ante/ante_wrapper.go (1)
  • 4-4: The addition of the fmt import statement is necessary for the new debug print functionality.
protocol/x/clob/e2e/batch_cancel_test.go (1)
  • 1-100: The test TestBatchCancel is well-structured and follows best practices for writing unit tests in Go, including the use of table-driven tests. The use of fmt.Println for debugging within tests is acceptable. Ensure that the test cases accurately reflect the intended behavior of the MsgBatchCancel functionality and that assertions correctly verify the outcomes.

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.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ef75e27 and f6d3e69.
Files selected for processing (9)
  • protocol/app/module/interface_registry.go (2 hunks)
  • protocol/app/process/other_msgs.go (2 hunks)
  • protocol/app/process/other_msgs_test.go (2 hunks)
  • protocol/app/process/utils_disallow_msgs.go (1 hunks)
  • protocol/app/process/utils_disallow_msgs_test.go (1 hunks)
  • protocol/testutil/constants/messages.go (2 hunks)
  • protocol/testutil/encoding/utils.go (2 hunks)
  • protocol/x/clob/ante/clob.go (6 hunks)
  • protocol/x/clob/keeper/orders.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • protocol/x/clob/ante/clob.go
  • protocol/x/clob/keeper/orders.go
Additional comments: 6
protocol/app/process/utils_disallow_msgs.go (1)
  • 21-22: The addition of *clobtypes.MsgBatchCancel to the IsDisallowClobOrderMsgInOtherTxs function correctly implements the functionality to disallow MsgBatchCancel messages in other transactions. This change aligns with the PR's objectives to enhance order management within the protocol.
protocol/app/process/utils_disallow_msgs_test.go (1)
  • 28-28: The update to the TestIsDisallowClobOrderMsgInOtherTxs function correctly includes *clobtypes.MsgBatchCancel in the test cases, ensuring that the new functionality is covered by the tests. This change is consistent with the PR's objectives and correctly tests the disallowing of MsgBatchCancel messages in other transactions.
protocol/testutil/encoding/utils.go (1)
  • 77-77: The addition of &clobtypes.MsgBatchCancel{} to the encoding configuration correctly updates the system to support the serialization and deserialization of the new MsgBatchCancel message type. This change is necessary for the integration of the new functionality introduced by the PR.
protocol/app/module/interface_registry.go (1)
  • 80-82: The addition of MsgBatchCancel to the CustomGetSigners map in the NewInterfaceRegistry function correctly updates the interface registry to support the signing of the new MsgBatchCancel message type. This change is necessary for the correct operation of the new functionality introduced by the PR.
protocol/testutil/constants/messages.go (1)
  • 60-71: The initialization of Msg_BatchCancel and Msg_BatchCancel_TxBtyes correctly sets up the new MsgBatchCancel message type for testing and usage within the protocol. The specific values provided for SubaccountId, ShortTermCancels, and GoodTilBlock align with the expected structure of a batch cancel message, ensuring that the new functionality is correctly integrated and testable.
protocol/app/process/other_msgs_test.go (1)
  • 91-97: The addition of an error case for MsgBatchCancel in the DecodeOtherMsgsTx test function correctly tests that batch cancel orders are not allowed in OtherTxs. This change ensures that the new functionality introduced by the PR is correctly integrated and tested, aligning with the changes made to disallow MsgBatchCancel messages in other transactions.

@github-actions github-actions bot added the proto label Feb 29, 2024
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f6d3e69 and aaa9470.
Files ignored due to path filters (1)
  • protocol/x/clob/types/tx.pb.go is excluded by: !**/*.pb.go
Files selected for processing (2)
  • proto/dydxprotocol/clob/tx.proto (1 hunks)
  • protocol/x/clob/e2e/batch_cancel_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/e2e/batch_cancel_test.go
Additional comments: 1
proto/dydxprotocol/clob/tx.proto (1)
  • 126-126: The change from fixed32 to uint32 for client_ids in the OrderBatch message could have implications for serialization/deserialization performance and storage, given the differences between fixed and variable-length encoding. It's important to ensure this change aligns with the expected range and usage patterns of client_ids. Additionally, verify the impact on existing data and systems to ensure compatibility and performance are maintained.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between aaa9470 and 48a824c.
Files ignored due to path filters (2)
  • protocol/go.mod is excluded by: !**/*.mod
  • protocol/go.sum is excluded by: !**/*.sum
Files selected for processing (1)
  • protocol/x/clob/ante/clob.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/ante/clob.go

@jonfung-dydx jonfung-dydx requested a review from jayy04 February 29, 2024 05:11
@jonfung-dydx
Copy link
Contributor Author

CT-513
CT-510

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 48a824c and c438032.
Files ignored due to path filters (3)
  • protocol/go.mod is excluded by: !**/*.mod
  • protocol/go.sum is excluded by: !**/*.sum
  • protocol/x/clob/types/tx.pb.go is excluded by: !**/*.pb.go
Files selected for processing (5)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (4 hunks)
  • proto/dydxprotocol/clob/tx.proto (1 hunks)
  • protocol/lib/log/constants.go (1 hunks)
  • protocol/testutil/app/app.go (2 hunks)
  • protocol/x/clob/keeper/orders.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
  • proto/dydxprotocol/clob/tx.proto
  • protocol/lib/log/constants.go
  • protocol/testutil/app/app.go
  • protocol/x/clob/keeper/orders.go
Additional comments: 4
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (4)
  • 170-174: The change from fixed32 to uint32 for the clientIds field in the OrderBatch interface is correctly documented in the comments. This optimization is intended to save space, which is crucial for performance and storage efficiency in blockchain transactions.
  • 187-191: Similarly, the SDKType version of the OrderBatch interface correctly reflects the change in the serialization type of the clientIds field from fixed32 to uint32. This consistency between the interface and its SDKType version is important for maintaining the integrity of the data structure across different parts of the system.
  • 795-795: In the encode function for the OrderBatch, the serialization of clientIds as uint32 is correctly implemented. This change aligns with the updated interface definition and ensures that the serialization process is optimized for space.
  • 820-823: The decode function for the OrderBatch correctly handles the deserialization of clientIds as uint32. This ensures that the data structure is correctly reconstructed from its serialized form, maintaining the integrity of the OrderBatch object during the deserialization process.

CheckTx = "check_tx"
RecheckTx = "recheck_tx"
DeliverTx = "deliver_tx"
MsgBatchCancel = "msg_batch_cancel"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a better group for this? doesn't seem related to CheckTx, DeliverTx, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the batch for tag values, i just haven't been bothered to move anything over to this constants file other than the callbacks

Comment on lines 62 to 71
// Note that we add `+1` here to account for the fact that `ctx.BlockHeight()` is technically the
// previously mined block, not the next block that will be proposed. This is due to the fact that
// this function is only ever called during `CheckTx`.
nextBlockHeight := lib.MustConvertIntegerToUint32(ctx.BlockHeight() + 1)

// Statefully validate the GTB and the clob pair ids.
goodTilBlock := msg.GetGoodTilBlock()
if err := k.validateGoodTilBlock(goodTilBlock, nextBlockHeight); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we do this in CancelShortTermOrder?

Copy link
Contributor Author

@jonfung-dydx jonfung-dydx Mar 1, 2024

Choose a reason for hiding this comment

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

we do but i'd rather just error out here than have each one error out slowly in each CancelShortTermOrder call and then return the user a BatchCancelAllFailed with error: invalid GTB, a top level error invalid gtb seems better

Copy link
Contributor

@jayy04 jayy04 Mar 1, 2024

Choose a reason for hiding this comment

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

then we can just call k.MemClob.CancelOrder directly? I guess not as future proof. no strong prefernce

Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: I kind of don't like this +1 logic everywhere and feel like we should move it to within validateGoodTilBlock.

haven't checked other references to validateGoodTilBlock, so will leave it to you to decide

Copy link
Contributor Author

@jonfung-dydx jonfung-dydx Mar 1, 2024

Choose a reason for hiding this comment

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

Hm we could call memclob directly. I'm ok with either.

Current approach

  • SendOffchainMessages gets called multiple times for each keeper call made
  • each cancel gets its own latency metric exposed

Calling memclob directly

  • can coalesce offchain messages and call SendOffchainMessages once
  • no latency metrics for each cancel

re: the +1 logic, i'm kind of for keeping it as it is since i don't want validateGoodTilBlock method to assume that it's being used in checktx. i'd prefer to keep it as a pure function of its parameters without assuming anything about usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going with keeping the current approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

re: the +1 logic, i'm kind of for keeping it as it is since i don't want validateGoodTilBlock method to assume that it's being used in checktx. i'd prefer to keep it as a pure function of its parameters without assuming anything about usage

I meant do something like

func validateGTB(ctx, ...) {
    if ctx.IsCheckTx() {
        GTB = ctx.BlockHeight() + 1
    }
    GTB = ctx.BlockHeight()
}

slightly cleaner imo, but up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

func (k Keeper) BatchCancelShortTermOrder(
ctx sdk.Context,
msg *types.MsgBatchCancel,
) error {
Copy link
Contributor

@jayy04 jayy04 Mar 1, 2024

Choose a reason for hiding this comment

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

thought about this a little bit more - what do you think about

  1. returning (succeeded []uint32, failed []uint32) here
  2. modify MsgBatchCancelResponse to return this information to the user (currently it's empty)

This way we don't have to add the logic for oneCancelSucceeded and code would be much cleaner, and at the same time, users know exactly which cancels went through. we can also include []error in addition to failed []uint32 to provide reasons for those failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm i'm down to do the suggestion (1), but returning []error seems a little clunky to me.

Reason being, usually when someone calls a function you expect to check one error to see if it failed or not.
I'm ok with returning just a BatchError, or using the errors.join method, but it seems to just stack errors together separated by newlines. not sure how i feel about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jayy04 jayy04 Mar 4, 2024

Choose a reason for hiding this comment

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

I think this function can still return ([]uint32, []uint32) here, the caller can decide what to do with it (e.g. returning ErrBatchCancelAllFailed).

oneCancelSucceeded logic belongs more to the caller imo, slight preference but nbd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 72 to 81
for _, batchOrder := range msg.GetShortTermCancels() {
clobPairId := batchOrder.GetClobPairId()
if _, found := k.GetClobPair(ctx, types.ClobPairId(clobPairId)); !found {
return errorsmod.Wrapf(
types.ErrInvalidClobPairParameter,
"Invalid clob pair id %+v",
clobPairId,
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto - do we not have duplicated validation in CancelShortTermOrder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm same reasoning as above, i'd just like to reject the message asap. but this does take up some state reads, so this could warrant directly calling memclob cancel.

GoodTilBlock: goodTilBlock,
},
}
// Run the short term order. If it errors, just log silently.
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of logging silently we also have the option to wrap and return it back to the user

something like

var wrapped error
for {
    ...
    if err != nil {
        wrapped = errorsmod.Wrapf(wrapped, error, ...)
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I guess we made the decision to only return error when all cancellations fail?

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c438032 and 2d1f0a9.
Files ignored due to path filters (1)
  • protocol/x/clob/types/tx.pb.go is excluded by: !**/*.pb.go
Files selected for processing (3)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (4 hunks)
  • proto/dydxprotocol/clob/tx.proto (1 hunks)
  • protocol/x/clob/e2e/batch_cancel_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts
  • proto/dydxprotocol/clob/tx.proto
  • protocol/x/clob/e2e/batch_cancel_test.go

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2d1f0a9 and c89ef04.
Files selected for processing (1)
  • protocol/x/clob/ante/clob.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/ante/clob.go

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c89ef04 and a8f49a3.
Files selected for processing (1)
  • protocol/x/clob/ante/clob.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/ante/clob.go

clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
)

// These tests are the same as the e2e tests for single order cancellations.
Copy link
Contributor

@jayy04 jayy04 Mar 4, 2024

Choose a reason for hiding this comment

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

nit: can we add a few tests for batch cancelling multiple orders (e.g. oneCancelSucceeded / all failed scenarios) ? can be done in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sep pr

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a8f49a3 and 7fa971b.
Files selected for processing (6)
  • protocol/mocks/ClobKeeper.go (1 hunks)
  • protocol/x/clob/ante/clob.go (4 hunks)
  • protocol/x/clob/e2e/batch_cancel_test.go (1 hunks)
  • protocol/x/clob/keeper/orders.go (1 hunks)
  • protocol/x/clob/types/clob_keeper.go (1 hunks)
  • protocol/x/clob/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • protocol/mocks/ClobKeeper.go
  • protocol/x/clob/ante/clob.go
  • protocol/x/clob/e2e/batch_cancel_test.go
  • protocol/x/clob/keeper/orders.go
  • protocol/x/clob/types/clob_keeper.go
  • protocol/x/clob/types/errors.go

@jonfung-dydx jonfung-dydx merged commit 4755744 into main Mar 4, 2024
31 of 33 checks passed
@jonfung-dydx jonfung-dydx deleted the jonfung/msgBatchCancelProcessing branch March 4, 2024 21:48
Eric-Warehime pushed a commit to skip-mev/v4-chain that referenced this pull request Mar 12, 2024
* wip implementation

* use new cometbft

* Revert "use new cometbft"

This reverts commit e5b8a03.

* go mod tidy

* basic e2e test

* more msgBatchCancels in code

* repeated fixed32 -> uint32

* remove debug prints

* update cometbft replace go.mod sha

* one more debug print

* typo

* regen indexer protos

* update comment on proto

* proto comment changes

* extract stateful validation into own fn

* pr format comments

* clean up test file

* new return type with success and failure

Signed-off-by: Eric <[email protected]>
Eric-Warehime added a commit to skip-mev/v4-chain that referenced this pull request Mar 12, 2024
commit d98f859
Author: Eric <[email protected]>
Date:   Mon Mar 11 12:46:53 2024 -0700

    Update sample pregenesis

    Signed-off-by: Eric <[email protected]>

commit 7f178fe
Author: Mohammed Affan <[email protected]>
Date:   Mon Mar 11 13:46:08 2024 -0400

    [OTE-209] Emit metrics gated through execution mode (dydxprotocol#1157)

    Signed-off-by: Eric <[email protected]>

commit 47e365d
Author: dydxwill <[email protected]>
Date:   Mon Mar 11 13:43:16 2024 -0400

    add aggregate comlink response code stats (dydxprotocol#1162)

    Signed-off-by: Eric <[email protected]>

commit 7774ad9
Author: shrenujb <[email protected]>
Date:   Fri Mar 8 17:30:49 2024 -0500

    [TRA-70] Add state migrations for isolated markets (dydxprotocol#1155)

    Signed-off-by: Shrenuj Bansal <[email protected]>
    Signed-off-by: Eric <[email protected]>

commit 89c7405
Author: Jonathan Fung <[email protected]>
Date:   Thu Mar 7 17:28:06 2024 -0500

    [CT-517] E2E tests batch cancel (dydxprotocol#1149)

    * more end to end test

    * extraprint

    * more e2e test

    Signed-off-by: Eric <[email protected]>

commit 41a3a41
Author: Teddy Ding <[email protected]>
Date:   Thu Mar 7 15:42:30 2024 -0500

    [OTE-200] OIMF protos (dydxprotocol#1125)

    * OIMF protos

    * add default genesis value, modify methods interface

    * fix genesis file

    * fix integration test

    * lint

    Signed-off-by: Eric <[email protected]>

commit 2a062b1
Author: Teddy Ding <[email protected]>
Date:   Thu Mar 7 15:24:15 2024 -0500

    Add `v5` upgrade handler and set up container upgrade test (dydxprotocol#1153)

    * wip

    * update preupgrade_genesis

    * fix preupgrade_genesis.json

    * nit

    * setupUpgradeStoreLoaders for v5.0.0

    Signed-off-by: Eric <[email protected]>

commit b7942b3
Author: jayy04 <[email protected]>
Date:   Thu Mar 7 13:43:48 2024 -0500

    [CT-647] construct the initial orderbook snapshot (dydxprotocol#1147)

    * [CT-647] construct the initial orderbook snapshot

    * [CT-647] initialize new streams and send orderbook snapshot (dydxprotocol#1152)

    * [CT-647] initialize new streams and send orderbook snapshot

    * use sync once

    * comments

    Signed-off-by: Eric <[email protected]>

commit c67a3c6
Author: shrenujb <[email protected]>
Date:   Thu Mar 7 12:40:37 2024 -0500

    [TRA-84] Move SA module address transfers to use perpetual based SA accounts (dydxprotocol#1146)

    Signed-off-by: Shrenuj Bansal <[email protected]
    Signed-off-by: Eric <[email protected]>

commit dba23e0
Author: Mohammed Affan <[email protected]>
Date:   Thu Mar 7 10:34:11 2024 -0500

    update readme link to point to the right page (dydxprotocol#1151)

    Signed-off-by: Eric <[email protected]>

commit b5870d5
Author: Tian <[email protected]>
Date:   Wed Mar 6 16:43:04 2024 -0500

    [TRA-86] scaffold x/vault (dydxprotocol#1148)

    * scaffold x/vault

    Signed-off-by: Eric <[email protected]>

commit 0eca041
Author: jayy04 <[email protected]>
Date:   Wed Mar 6 10:48:42 2024 -0500

    [CT-652] add command line flag for full node streaming (dydxprotocol#1145)

    Signed-off-by: Eric <[email protected]>

commit b319cb8
Author: jayy04 <[email protected]>
Date:   Tue Mar 5 21:58:35 2024 -0500

    [CT-646] stream offchain updates through stream manager (dydxprotocol#1138)

    * [CT-646] stream offchain updates through stream manager

    * comments

    * fix lint

    * get rid of finished

    * comments

    * comments

    Signed-off-by: Eric <[email protected]>

commit 1c54620
Author: shrenujb <[email protected]>
Date:   Tue Mar 5 16:34:19 2024 -0500

    [TRA-78] Add function to retrieve collateral pool addr for a subaccount (dydxprotocol#1142)

    Signed-off-by: Shrenuj Bansal <[email protected]>
    Signed-off-by: Eric <[email protected]>

commit b8c1d62
Author: dydxwill <[email protected]>
Date:   Tue Mar 5 15:03:28 2024 -0500

    [OTE-141] implement post /compliance/geoblock (dydxprotocol#1129)

    Signed-off-by: Eric <[email protected]>

commit ab8c570
Author: Jonathan Fung <[email protected]>
Date:   Tue Mar 5 11:19:53 2024 -0500

    Fix mock-gen dydxprotocol#1140

    Signed-off-by: Eric <[email protected]>

commit 12506a1
Author: shrenujb <[email protected]>
Date:   Mon Mar 4 21:33:28 2024 -0500

    [TRA-64] Use market specific insurance fund for cross or isolated markets (dydxprotocol#1132)

    Signed-off-by: Shrenuj Bansal <[email protected]>
    Signed-off-by: Eric <[email protected]>

commit 929f09e
Author: Jonathan Fung <[email protected]>
Date:   Mon Mar 4 13:48:04 2024 -0800

    [CT-514] Clob `MsgBatchCancel` functionality (dydxprotocol#1110)

    * wip implementation

    * use new cometbft

    * Revert "use new cometbft"

    This reverts commit e5b8a03.

    * go mod tidy

    * basic e2e test

    * more msgBatchCancels in code

    * repeated fixed32 -> uint32

    * remove debug prints

    * update cometbft replace go.mod sha

    * one more debug print

    * typo

    * regen indexer protos

    * update comment on proto

    * proto comment changes

    * extract stateful validation into own fn

    * pr format comments

    * clean up test file

    * new return type with success and failure

    Signed-off-by: Eric <[email protected]>

commit 41de83e
Author: dydxwill <[email protected]>
Date:   Mon Mar 4 12:22:16 2024 -0500

    add index to address read replica lag (dydxprotocol#1137)

    Signed-off-by: Eric <[email protected]>

commit 735d9a8
Author: dydxwill <[email protected]>
Date:   Mon Mar 4 11:56:59 2024 -0500

    rename (dydxprotocol#1136)

    Signed-off-by: Eric <[email protected]>

commit 86617dd
Author: jayy04 <[email protected]>
Date:   Mon Mar 4 10:43:31 2024 -0500

    [CT-644] instantiate grpc stream manager (dydxprotocol#1134)

    * [CT-644] instantiate grpc stream manager

    * update type

    * update channel type

    Signed-off-by: Eric <[email protected]>

commit 32afd64
Author: Eric <[email protected]>
Date:   Mon Mar 11 12:41:06 2024 -0700

    Update go version in Dockerfile

    Signed-off-by: Eric <[email protected]>

commit ba27204
Author: Eric <[email protected]>
Date:   Fri Mar 8 09:44:04 2024 -0800

    Add slinky utils, use that to convert between market and currency pair

commit 667a804
Author: Eric <[email protected]>
Date:   Wed Mar 6 20:43:40 2024 -0800

    Update error messages

commit d53292c
Author: Eric <[email protected]>
Date:   Wed Mar 6 20:16:01 2024 -0800

    Update docstrings, rename OracleClient

commit daad125
Author: Eric <[email protected]>
Date:   Mon Mar 4 10:51:23 2024 -0800

    VoteExtension slinky logic
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.

2 participants