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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
constants
  • Loading branch information
jonfung-dydx committed Feb 27, 2024
commit 040422a5520bac2dac7bb0bd1736496da03fd25f
4 changes: 4 additions & 0 deletions protocol/x/clob/types/constants.go
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions protocol/x/clob/types/errors.go
Original file line number Diff line number Diff line change
@@ -206,6 +206,11 @@ var (
44,
"invalid time in force",
)
ErrInvalidBatchCancel = errorsmod.Register(
ModuleName,
45,
"Invalid Batch Cancel",
jonfung-dydx marked this conversation as resolved.
Show resolved Hide resolved
)

// Liquidations errors.
ErrInvalidLiquidationsConfig = errorsmod.Register(
4 changes: 2 additions & 2 deletions protocol/x/clob/types/message_batch_cancel.go
Original file line number Diff line number Diff line change
@@ -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) {
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 {
Loading