From 8f77dbe0003580dc21841be4bcc0ed3e5fed2b63 Mon Sep 17 00:00:00 2001 From: Jonathan Fung Date: Mon, 26 Feb 2024 17:15:19 -0800 Subject: [PATCH 1/5] validatebasic for new MsgBatchCancel --- protocol/x/clob/types/message_batch_cancel.go | 54 ++++++++ .../x/clob/types/message_batch_cancel_test.go | 123 ++++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 protocol/x/clob/types/message_batch_cancel.go create mode 100644 protocol/x/clob/types/message_batch_cancel_test.go diff --git a/protocol/x/clob/types/message_batch_cancel.go b/protocol/x/clob/types/message_batch_cancel.go new file mode 100644 index 00000000000..08d02957046 --- /dev/null +++ b/protocol/x/clob/types/message_batch_cancel.go @@ -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, + } +} + +// 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 +} diff --git a/protocol/x/clob/types/message_batch_cancel_test.go b/protocol/x/clob/types/message_batch_cancel_test.go new file mode 100644 index 00000000000..acf3afe404a --- /dev/null +++ b/protocol/x/clob/types/message_batch_cancel_test.go @@ -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], + }, + { + 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) + }) + } +} From 040422a5520bac2dac7bb0bd1736496da03fd25f Mon Sep 17 00:00:00 2001 From: Jonathan Fung Date: Mon, 26 Feb 2024 21:23:30 -0800 Subject: [PATCH 2/5] constants --- protocol/x/clob/types/constants.go | 4 ++++ protocol/x/clob/types/errors.go | 5 +++++ protocol/x/clob/types/message_batch_cancel.go | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/protocol/x/clob/types/constants.go b/protocol/x/clob/types/constants.go index c0eaf75392b..069e7646e76 100644 --- a/protocol/x/clob/types/constants.go +++ b/protocol/x/clob/types/constants.go @@ -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 + // 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. diff --git a/protocol/x/clob/types/errors.go b/protocol/x/clob/types/errors.go index 5f2ec04f120..779d95fbf61 100644 --- a/protocol/x/clob/types/errors.go +++ b/protocol/x/clob/types/errors.go @@ -206,6 +206,11 @@ var ( 44, "invalid time in force", ) + ErrInvalidBatchCancel = errorsmod.Register( + ModuleName, + 45, + "Invalid Batch Cancel", + ) // Liquidations errors. ErrInvalidLiquidationsConfig = errorsmod.Register( diff --git a/protocol/x/clob/types/message_batch_cancel.go b/protocol/x/clob/types/message_batch_cancel.go index 08d02957046..39d266ef23f 100644 --- a/protocol/x/clob/types/message_batch_cancel.go +++ b/protocol/x/clob/types/message_batch_cancel.go @@ -9,7 +9,7 @@ import ( var _ sdk.Msg = &MsgBatchCancel{} -// NewMsgCancelOrderShortTerm constructs a MsgBatchCancel from an `OrderId` and a `GoodTilBlock`. +// NewMsgBatchCancel constructs a MsgBatchCancel. func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { return &MsgBatchCancel{ SubaccountId: subaccountId, @@ -18,7 +18,7 @@ func NewMsgBatchCancel(subaccountId types.SubaccountId, cancelBatch []OrderBatch } } -// ValidateBasic performs stateless validation for +// ValidateBasic performs stateless validation for the `MsgBatchCancel` msg. func (msg *MsgBatchCancel) ValidateBasic() (err error) { subaccountId := msg.GetSubaccountId() if err := subaccountId.Validate(); err != nil { From cad3af4fc84775e9871721ad39ab20c67dce8a3c Mon Sep 17 00:00:00 2001 From: Jonathan Fung Date: Tue, 27 Feb 2024 10:44:24 -0800 Subject: [PATCH 3/5] pr comments --- protocol/x/clob/types/errors.go | 2 +- protocol/x/clob/types/message_batch_cancel.go | 15 +++++++++- .../x/clob/types/message_batch_cancel_test.go | 29 ++++++++++++++++--- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/protocol/x/clob/types/errors.go b/protocol/x/clob/types/errors.go index 779d95fbf61..792f0a94514 100644 --- a/protocol/x/clob/types/errors.go +++ b/protocol/x/clob/types/errors.go @@ -209,7 +209,7 @@ var ( ErrInvalidBatchCancel = errorsmod.Register( ModuleName, 45, - "Invalid Batch Cancel", + "Invalid batch cancel message", ) // Liquidations errors. diff --git a/protocol/x/clob/types/message_batch_cancel.go b/protocol/x/clob/types/message_batch_cancel.go index 39d266ef23f..dacd5b3cac8 100644 --- a/protocol/x/clob/types/message_batch_cancel.go +++ b/protocol/x/clob/types/message_batch_cancel.go @@ -26,9 +26,22 @@ func (msg *MsgBatchCancel) ValidateBasic() (err error) { } cancelBatches := msg.GetShortTermCancels() + if len(cancelBatches) == 0 { + return errorsmod.Wrapf( + ErrInvalidBatchCancel, + "Batch cancel cannot have zero orders specified.", + ) + } totalNumberCancels := 0 for _, cancelBatch := range cancelBatches { - totalNumberCancels += len(cancelBatch.GetClientIds()) + 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 { diff --git a/protocol/x/clob/types/message_batch_cancel_test.go b/protocol/x/clob/types/message_batch_cancel_test.go index acf3afe404a..504ee7c9921 100644 --- a/protocol/x/clob/types/message_batch_cancel_test.go +++ b/protocol/x/clob/types/message_batch_cancel_test.go @@ -53,11 +53,11 @@ func TestMsgBatchCancel_ValidateBasic(t *testing.T) { []types.OrderBatch{ { ClobPairId: 0, - ClientIds: oneOverMax[:52], + ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2+2], }, { ClobPairId: 1, - ClientIds: oneOverMax[:52], + ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2+2], }, }, 10, @@ -70,11 +70,11 @@ func TestMsgBatchCancel_ValidateBasic(t *testing.T) { []types.OrderBatch{ { ClobPairId: 0, - ClientIds: oneOverMax[:50], + ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2], }, { ClobPairId: 1, - ClientIds: oneOverMax[:50], + ClientIds: oneOverMax[:types.MaxMsgBatchCancelBatchSize/2], }, }, 10, @@ -109,6 +109,27 @@ func TestMsgBatchCancel_ValidateBasic(t *testing.T) { ), err: types.ErrInvalidBatchCancel, }, + "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) { From dc9ee99bff062075b300e681feeb75145180b8bd Mon Sep 17 00:00:00 2001 From: Jonathan Fung Date: Tue, 27 Feb 2024 10:59:14 -0800 Subject: [PATCH 4/5] lint --- protocol/x/clob/types/message_batch_cancel.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/protocol/x/clob/types/message_batch_cancel.go b/protocol/x/clob/types/message_batch_cancel.go index dacd5b3cac8..a6b0416a609 100644 --- a/protocol/x/clob/types/message_batch_cancel.go +++ b/protocol/x/clob/types/message_batch_cancel.go @@ -4,13 +4,13 @@ import ( errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" - types "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types" + satypes "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 { +func NewMsgBatchCancel(subaccountId satypes.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { return &MsgBatchCancel{ SubaccountId: subaccountId, ShortTermCancels: cancelBatch, From a0d1b233fe96b7799b41591c22b4c7ce1bcffa67 Mon Sep 17 00:00:00 2001 From: Jonathan Fung Date: Tue, 27 Feb 2024 11:17:48 -0800 Subject: [PATCH 5/5] more lint --- protocol/x/clob/types/message_batch_cancel.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/protocol/x/clob/types/message_batch_cancel.go b/protocol/x/clob/types/message_batch_cancel.go index a6b0416a609..675f3485775 100644 --- a/protocol/x/clob/types/message_batch_cancel.go +++ b/protocol/x/clob/types/message_batch_cancel.go @@ -10,7 +10,11 @@ import ( var _ sdk.Msg = &MsgBatchCancel{} // NewMsgBatchCancel constructs a MsgBatchCancel. -func NewMsgBatchCancel(subaccountId satypes.SubaccountId, cancelBatch []OrderBatch, goodTilBlock uint32) *MsgBatchCancel { +func NewMsgBatchCancel( + subaccountId satypes.SubaccountId, + cancelBatch []OrderBatch, + goodTilBlock uint32, +) *MsgBatchCancel { return &MsgBatchCancel{ SubaccountId: subaccountId, ShortTermCancels: cancelBatch,