[CT-520] Validatebasic for new MsgBatchCancel #1101
Conversation
WalkthroughThe update introduces a new message type, Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // NewMsgCancelOrderShortTerm constructs a MsgBatchCancel from an `OrderId` and a `GoodTilBlock`. | ||
| func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { | ||
| return &MsgBatchCancel{ | ||
| SubaccountId: subaccountId, | ||
| ShortTermCancels: cancelBatch, | ||
| GoodTilBlock: goodTilBlock, | ||
| } |
There was a problem hiding this comment.
The constructor NewMsgBatchCancel is well-defined, correctly initializing a MsgBatchCancel instance. However, the parameter name cancelBatch might be misleading as it suggests a single batch, whereas it represents multiple batches. Consider renaming it to cancelBatches for clarity.
- func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel {
+ func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatches []OrderBatch, goodTilBlock uint32) *MsgBatchCancel {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.
| // NewMsgCancelOrderShortTerm constructs a MsgBatchCancel from an `OrderId` and a `GoodTilBlock`. | |
| func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { | |
| return &MsgBatchCancel{ | |
| SubaccountId: subaccountId, | |
| ShortTermCancels: cancelBatch, | |
| GoodTilBlock: goodTilBlock, | |
| } | |
| // NewMsgCancelOrderShortTerm constructs a MsgBatchCancel from an `OrderId` and a `GoodTilBlock`. | |
| func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatches []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { | |
| return &MsgBatchCancel{ | |
| SubaccountId: subaccountId, | |
| ShortTermCancels: cancelBatch, | |
| GoodTilBlock: goodTilBlock, | |
| } |
| // ValidateBasic performs stateless validation for | ||
| func (msg *MsgBatchCancel) ValidateBasic() (err error) { | ||
| subaccountId := msg.GetSubaccountId() | ||
| if err := subaccountId.Validate(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cancelBatches := msg.GetShortTermCancels() | ||
| totalNumberCancels := 0 | ||
| for _, cancelBatch := range cancelBatches { | ||
| totalNumberCancels += len(cancelBatch.GetClientIds()) | ||
| seenClientIds := map[uint32]struct{}{} | ||
| for _, clientId := range cancelBatch.GetClientIds() { | ||
| if _, seen := seenClientIds[clientId]; seen { | ||
| return errorsmod.Wrapf( | ||
| ErrInvalidBatchCancel, | ||
| "Batch cancel cannot have duplicate cancels. Duplicate clob pair id: %+v, client id: %+v", | ||
| cancelBatch.GetClobPairId(), | ||
| clientId, | ||
| ) | ||
| } | ||
| seenClientIds[clientId] = struct{}{} | ||
| } | ||
| } | ||
| if uint32(totalNumberCancels) > MaxMsgBatchCancelBatchSize { | ||
| return errorsmod.Wrapf( | ||
| ErrInvalidBatchCancel, | ||
| "Batch cancel cannot have over %+v orders. Order count: %+v", | ||
| MaxMsgBatchCancelBatchSize, | ||
| totalNumberCancels, | ||
| ) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The ValidateBasic method performs several checks, including subaccount ID validation and ensuring no duplicate client IDs within each batch. However, there are a few areas for improvement:
- The error message for duplicate client IDs (lines 35-40) could be enhanced by specifying it's within a single batch for clarity.
- The check for the total number of cancels exceeding
MaxMsgBatchCancelBatchSize(lines 45-51) is crucial. EnsureMaxMsgBatchCancelBatchSizeis defined and documented elsewhere in the codebase, as it's a key constraint. - Consider adding a comment above the loop starting at line 30 to explain the purpose of the loop, which is to ensure no duplicate client IDs within each batch and to count the total number of cancels.
Overall, the method is logically sound but could benefit from minor refinements for clarity and maintainability.
| msg: *types.NewMsgBatchCancel( | ||
| constants.InvalidSubaccountIdNumber, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: []uint32{ | ||
| 0, 1, 2, 3, | ||
| }, | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: satypes.ErrInvalidSubaccountIdNumber, | ||
| }, | ||
| "over 100 cancels in for one clob pair id": { | ||
| msg: *types.NewMsgBatchCancel( | ||
| constants.Alice_Num0, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: oneOverMax, | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: types.ErrInvalidBatchCancel, | ||
| }, | ||
| "over 100 cancels split over two clob pair id": { | ||
| msg: *types.NewMsgBatchCancel( | ||
| constants.Alice_Num0, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: oneOverMax[:52], | ||
| }, | ||
| { | ||
| ClobPairId: 1, | ||
| ClientIds: oneOverMax[:52], | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: types.ErrInvalidBatchCancel, | ||
| }, | ||
| "success: two clob pair id, 100 cancels": { | ||
| msg: *types.NewMsgBatchCancel( | ||
| constants.Alice_Num0, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: oneOverMax[:50], | ||
| }, | ||
| { | ||
| ClobPairId: 1, | ||
| ClientIds: oneOverMax[:50], | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: nil, | ||
| }, | ||
| "success: one clob pair id, 100 cancels": { | ||
| msg: *types.NewMsgBatchCancel( | ||
| constants.Alice_Num0, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize], | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: nil, | ||
| }, | ||
| "duplicate clob pair ids": { | ||
| msg: *types.NewMsgBatchCancel( | ||
| constants.Alice_Num0, | ||
| []types.OrderBatch{ | ||
| { | ||
| ClobPairId: 0, | ||
| ClientIds: []uint32{ | ||
| 0, 1, 2, 3, 1, | ||
| }, | ||
| }, | ||
| }, | ||
| 10, | ||
| ), | ||
| err: types.ErrInvalidBatchCancel, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| err := tc.msg.ValidateBasic() | ||
| if tc.err != nil { | ||
| require.ErrorIs(t, err, tc.err) | ||
| return | ||
| } | ||
| require.NoError(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test cases in TestMsgBatchCancel_ValidateBasic are comprehensive, covering a wide range of scenarios. A few suggestions for improvement:
- The setup for
oneOverMax(lines 13-16) is clear, but consider adding a brief comment explaining its purpose, which is to create a client ID slice exceeding the maximum allowed size. - In the test case names, such as "over 100 cancels in for one clob pair id" (line 37), ensure consistency in describing the test scenarios. For example, if
MaxMsgBatchCancelBatchSizeis not always 100, use a more generic description or reference the constant directly. - The use of
constants.Alice_Num0andconstants.InvalidSubaccountIdNumber(lines 24, 39, etc.) is good for readability, but ensure these constants are well-documented in their definition to avoid confusion.
Overall, the tests are well-structured and effectively validate the functionality of MsgBatchCancel. Minor improvements in documentation and naming could enhance readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/types/constants.go (1 hunks)
- protocol/x/clob/types/errors.go (1 hunks)
- protocol/x/clob/types/message_batch_cancel.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/types/message_batch_cancel.go
Additional comments: 2
protocol/x/clob/types/constants.go (1)
- 11-13: The addition of
MaxMsgBatchCancelBatchSizewith a value of 100 is a good practice for defining system limits. Ensure this value aligns with the system's capacity and the expected use cases for batch cancellation.protocol/x/clob/types/errors.go (1)
- 209-213: The introduction of
ErrInvalidBatchCancelis a positive step towards robust error handling for the newMsgBatchCancelfunctionality. Verify the uniqueness of the error code within the module. Consider enhancing the error message to provide more detail about common validation failures if applicable.
| @@ -8,6 +8,10 @@ import ( | |||
| // `MsgPlaceOrder` or `MsgCancelOrder` message will be considered valid by the validator. | |||
| const ShortBlockWindow uint32 = 20 | |||
|
|
|||
| // MaxMsgBatchCancelBatchSize represents the maximum number of cancels that a MsgBatchCancel | |||
| // can have in one Msg. | |||
| const MaxMsgBatchCancelBatchSize uint32 = 100 | |||
There was a problem hiding this comment.
for my understanding - did we decide to set to to 100 or 1000?
There was a problem hiding this comment.
We can change this as needed later.
| } | ||
|
|
||
| // ValidateBasic performs stateless validation for the `MsgBatchCancel` msg. | ||
| func (msg *MsgBatchCancel) ValidateBasic() (err error) { |
There was a problem hiding this comment.
so ValidateBasic is actually optional as of v0.50 - I think it's still supported for backwards compatibility but can you maybe spend 10-15 minutes to see how it works w/ the new version and what was the motivation? kind of curious about this
There was a problem hiding this comment.
just wondering if transactions that are obviously wrong (e.g. failing ValidateBasic) can now enter mempool and be included in a block
There was a problem hiding this comment.
Seems like they have moved all validation to the msg server.
Example PRS
cosmos/cosmos-sdk#15832
Based on this comment
https://github.com/cosmos/cosmos-sdk/pull/15782/files#r1163771773
it seems like we lose the functionality of verifying if the proposal message is correct before execution. Apparently simulation covers that case you have mentioned. However, a bit confused as users can still submit messages without simulating first.
| 10, | ||
| ), | ||
| err: types.ErrInvalidBatchCancel, | ||
| }, |
There was a problem hiding this comment.
add cases where len(short_term_cancel) == 0 and len(client_ids) == 0?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/types/errors.go (1 hunks)
- protocol/x/clob/types/message_batch_cancel.go (1 hunks)
- protocol/x/clob/types/message_batch_cancel_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/clob/types/errors.go
- protocol/x/clob/types/message_batch_cancel.go
- protocol/x/clob/types/message_batch_cancel_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/types/message_batch_cancel.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/types/message_batch_cancel.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/types/message_batch_cancel.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/types/message_batch_cancel.go
MsgBatchCancel stateless validation.