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

Merged
merged 5 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 4 additions & 0 deletions protocol/x/clob/types/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

for my understanding - did we decide to set to to 100 or 1000?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


// StatefulOrderTimeWindow represents the maximum amount of time in seconds past the current block time that a
// long-term/conditional `MsgPlaceOrder` message will be considered valid by the validator.
const StatefulOrderTimeWindow time.Duration = 95 * 24 * time.Hour // 95 days.
Expand Down
5 changes: 5 additions & 0 deletions protocol/x/clob/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ var (
44,
"invalid time in force",
)
ErrInvalidBatchCancel = errorsmod.Register(
ModuleName,
45,
"Invalid batch cancel message",
)

// Liquidations errors.
ErrInvalidLiquidationsConfig = errorsmod.Register(
Expand Down
67 changes: 67 additions & 0 deletions protocol/x/clob/types/message_batch_cancel.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
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{}

// NewMsgBatchCancel constructs a MsgBatchCancel.
func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel {
return &MsgBatchCancel{
SubaccountId: subaccountId,
ShortTermCancels: cancelBatch,
GoodTilBlock: goodTilBlock,
}
}

// ValidateBasic performs stateless validation for the `MsgBatchCancel` msg.
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

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()
if len(cancelBatches) == 0 {
return errorsmod.Wrapf(
ErrInvalidBatchCancel,
"Batch cancel cannot have zero orders specified.",
)
}
totalNumberCancels := 0
for _, cancelBatch := range cancelBatches {
numClientIds := len(cancelBatch.GetClientIds())
if numClientIds == 0 {
return errorsmod.Wrapf(
ErrInvalidBatchCancel,
"Order Batch cannot have zero client ids.",
)
}
totalNumberCancels += numClientIds
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
}
144 changes: 144 additions & 0 deletions protocol/x/clob/types/message_batch_cancel_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
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[:types.MaxMsgBatchCancelBatchSize/2+2],
},
{
ClobPairId: 1,
ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2+2],
},
},
10,
),
err: types.ErrInvalidBatchCancel,
},
"success: two clob pair id, 100 cancels": {
msg: *types.NewMsgBatchCancel(
constants.Alice_Num0,
[]types.OrderBatch{
{
ClobPairId: 0,
ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2],
},
{
ClobPairId: 1,
ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2],
},
},
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?

"zero batches in cancel batch": {
msg: *types.NewMsgBatchCancel(
constants.Alice_Num0,
[]types.OrderBatch{},
10,
),
err: types.ErrInvalidBatchCancel,
},
"zero client ids in cancel batch": {
msg: *types.NewMsgBatchCancel(
constants.Alice_Num0,
[]types.OrderBatch{
{
ClobPairId: 0,
ClientIds: []uint32{},
},
},
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)
})
}
}
Loading