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.

Sign up for GitHub

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-520] Validatebasic for new MsgBatchCancel #1101

Merged
merged 5 commits into from
Feb 27, 2024
Merged

[CT-520] Validatebasic for new MsgBatchCancel #1101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f77dbe
validatebasic for new MsgBatchCancel
jonfung-dydx Feb 27, 2024
040422a
constants
jonfung-dydx Feb 27, 2024
cad3af4
pr comments
jonfung-dydx Feb 27, 2024
dc9ee99
lint
jonfung-dydx Feb 27, 2024
a0d1b23
more lint
jonfung-dydx Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions protocol/x/clob/types/message_batch_cancel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package types

import (
errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
types "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
)

var _ sdk.Msg = &MsgBatchCancel{}

// 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,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering if transactions that are obviously wrong (e.g. failing ValidateBasic) can now enter mempool and be included in a block

Copy link
Contributor Author

@jonfung-dydx jonfung-dydx Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cosmos/cosmos-sdk#15648

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. The error message for duplicate client IDs (lines 35-40) could be enhanced by specifying it's within a single batch for clarity.
  2. The check for the total number of cancels exceeding MaxMsgBatchCancelBatchSize (lines 45-51) is crucial. Ensure MaxMsgBatchCancelBatchSize is defined and documented elsewhere in the codebase, as it's a key constraint.
  3. 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.

}
123 changes: 123 additions & 0 deletions protocol/x/clob/types/message_batch_cancel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package types_test

import (
"testing"

"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
"github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
"github.com/stretchr/testify/require"
)

func TestMsgBatchCancel_ValidateBasic(t *testing.T) {
oneOverMax := []uint32{}
for i := uint32(0); i < types.MaxMsgBatchCancelBatchSize+1; i++ {
oneOverMax = append(oneOverMax, i)
}

tests := map[string]struct {
msg types.MsgBatchCancel
err error
}{
"invalid subaccount": {
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],
jonfung-dydx marked this conversation as resolved.
Show resolved Hide resolved
},
{
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],
jonfung-dydx marked this conversation as resolved.
Show resolved Hide resolved
},
{
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,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

add cases where len(short_term_cancel) == 0 and len(client_ids) == 0?

}
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)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The test cases in TestMsgBatchCancel_ValidateBasic are comprehensive, covering a wide range of scenarios. A few suggestions for improvement:

  1. 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.
  2. 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 MaxMsgBatchCancelBatchSize is not always 100, use a more generic description or reference the constant directly.
  3. The use of constants.Alice_Num0 and constants.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.

}
Loading