Add order limit for price level and pair limit for contracts #781
Conversation
| events := []sdk.Event{} | ||
| // Validation such that only the user who stored the code can register pairs | ||
| for _, batchPair := range msg.Batchcontractpair { | ||
| contractAddr := batchPair.ContractAddr | ||
| if uint64(len(k.GetAllRegisteredPairs(ctx, contractAddr))) >= maxPairsPerContract { | ||
| return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "contract %s already has %d pairs registered", contractAddr, maxPairsPerContract) |
There was a problem hiding this comment.
is this log message correct?
There was a problem hiding this comment.
fixed, and added migration for params. No need to migrate for the new order counter state since it will be overwritten once order book is changed, and empty keys are handled as 0
x/dex/keeper/order_count.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (k Keeper) GetOrderCount(ctx sdk.Context, contractAddr string, priceDenom string, assetDenom string, direction types.PositionDirection, price sdk.Dec) uint64 { |
There was a problem hiding this comment.
Should we expose a query as well to expose the OrderCount given a contract, pair, direciton and price?
| @@ -20,6 +20,8 @@ var DexWhitelistedKeys = []string{ | |||
| types.SettlementEntryKey, | |||
| types.NextOrderIDKey, | |||
| types.MatchResultKey, | |||
| types.LongOrderCountKey, | |||
There was a problem hiding this comment.
why do we need OrderCount keys for both long and short if we only define one max_order_per_price in param?
There was a problem hiding this comment.
since long orders and short orders are stored separately it's easier to just store their counts separately as well. We can add params to specify max orders for long and for short separately but i don't know if they would ever be set differently
go.mod
Outdated
| @@ -269,7 +269,7 @@ require ( | |||
| replace ( | |||
| github.com/CosmWasm/wasmd => github.com/sei-protocol/sei-wasmd v0.0.1 | |||
| github.com/confio/ics23/go => github.com/cosmos/cosmos-sdk/ics23/go v0.8.0 | |||
| github.com/cosmos/cosmos-sdk => github.com/sei-protocol/sei-cosmos v0.2.29 | |||
| github.com/cosmos/cosmos-sdk => github.com/sei-protocol/sei-cosmos v0.2.29-new-type | |||
There was a problem hiding this comment.
reminder to update this before merge
| return binary.BigEndian.Uint64(value) | ||
| } | ||
|
|
||
| func (k Keeper) DecreaseOrderCount(ctx sdk.Context, contractAddr string, priceDenom string, assetDenom string, direction types.PositionDirection, price sdk.Dec, count uint64) error { |
There was a problem hiding this comment.
nit: why don't we use a generic UpdateOrderCount here
There was a problem hiding this comment.
want to keep integer type consistent instead of having to use a signed int64
x/dex/keeper/order_count_test.go
Outdated
| } { | ||
| require.Equal(t, uint64(0), keeper.GetOrderCount(ctx, keepertest.TestContract, keepertest.TestPair.PriceDenom, keepertest.TestPair.AssetDenom, direction, sdk.NewDec(1))) | ||
| require.Nil(t, keeper.SetOrderCount(ctx, keepertest.TestContract, keepertest.TestPair.PriceDenom, keepertest.TestPair.AssetDenom, direction, sdk.NewDec(1), 5)) | ||
| require.Equal(t, uint64(5), keeper.GetOrderCount(ctx, keepertest.TestContract, keepertest.TestPair.PriceDenom, keepertest.TestPair.AssetDenom, direction, sdk.NewDec(1))) |
There was a problem hiding this comment.
can we add tests for Increase and Descrese count
codchen
force-pushed
the
tony-chen-add-caps
branch
2 times, most recently
from
d757584 to
bde1a5d
Compare
codchen
force-pushed
the
tony-chen-add-caps
branch
from
bde1a5d to
880c4bf
Compare
Describe your changes and provide context
In order to prevent application-level DDoS attack against
dexmodule, we introduce limits for both the number of open orders allowed at a price level for a given asset pair, and the number of asset pairs a contract can have.The naive implementation would be to query order entries in
msg_server_order_placementevery time an order comes in and get the length of existing allocations, however this could become a performance drag since it would need to load all allocations. This PR implemented it in another way by introducing a new counter stateOrderCount, which is updated whenever the number of allocations for a price level changes (i.e. when cancellation or placement are handled in EndBlock). Then, inmsg_server_order_placementit only needs to load this counter which is trivial in terms of latency.For the number of pair per contract we took the naive implementation since the limit of pairs is much smaller than the limit of orders, and transaction frequency of pair registration is a lot less than that of order placement.
Note that the dependency on sei-cosmos needs to be updated to a stable release once one is cut with the new ACL types.
Testing performed to validate your change
unit tests as well as semi-integration test in module_test.go