From 5f885b82a19e7329121928d4d4a8fcd3fdb7c5b6 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Tue, 3 Oct 2023 15:46:59 -0600 Subject: [PATCH] [1658]: Store the last order id and use it for getting the next order id to use. Prior to this, an id could be reused if an order were created then filled before the next order was created. And while our stuff could handle that okay, external systems probably want that to be unique. --- x/exchange/keeper/keys.go | 9 ++++++++ x/exchange/keeper/keys_test.go | 24 +++++++++++++++++--- x/exchange/keeper/market.go | 13 ++++++----- x/exchange/keeper/orders.go | 40 ++++++++++++++++++---------------- 4 files changed, 58 insertions(+), 28 deletions(-) diff --git a/x/exchange/keeper/keys.go b/x/exchange/keeper/keys.go index c8cdc91d16..29760a938b 100644 --- a/x/exchange/keeper/keys.go +++ b/x/exchange/keeper/keys.go @@ -28,6 +28,8 @@ import ( // // Known Market IDs: 0x07 | => nil // +// Last Order ID: 0x08 => uint64 +// // Markets: // Some aspects of a market are stored using the accounts module and the MarketAccount type. // Others are stored in the exchange module. @@ -74,6 +76,8 @@ const ( KeyTypeLastMarketID = byte(0x06) // KeyTypeKnownMarketID is the type byte for known market id entries. KeyTypeKnownMarketID = byte(0x07) + // KeyTypeLastOrderID is the type byte for the id of the last order created. + KeyTypeLastOrderID = byte(0x08) // KeyTypeMarket is the type byte for market entries. KeyTypeMarket = byte(0x01) // KeyTypeOrder is the type byte for order entries. @@ -237,6 +241,11 @@ func ParseKeySuffixKnownMarketID(suffix []byte) (uint32, bool) { return uint32FromBz(suffix) } +// MakeKeyLastOrderID creates the key for the id of the last order created. +func MakeKeyLastOrderID() []byte { + return []byte{KeyTypeLastOrderID} +} + // keyPrefixMarket creates the root of a market's key with extra capacity for the rest. func keyPrefixMarket(marketID uint32, extraCap int) []byte { return prepKey(KeyTypeMarket, uint32Bz(marketID), extraCap) diff --git a/x/exchange/keeper/keys_test.go b/x/exchange/keeper/keys_test.go index 4bda45c0c1..bfa93f0a17 100644 --- a/x/exchange/keeper/keys_test.go +++ b/x/exchange/keeper/keys_test.go @@ -32,10 +32,17 @@ type expectedPrefix struct { value []byte } +// keyTestCase is used with checkKey to run some standardized checks on a test case for a store key. type keyTestCase struct { - maker func() []byte - expected []byte - expPanic string + // maker is a function that creates the key value to check. + // A maker is used (instead of just providing the value to check) so that + // the function in question can be checked for panics. + maker func() []byte + // expected is the expected result of the maker. + expected []byte + // expPanic is the panic message expected when the maker is called. + expPanic string + // expPrefixes are all the prefixes that the result of the maker is expected to have. expPrefixes []expectedPrefix } @@ -83,6 +90,7 @@ func TestKeyTypeUniqueness(t *testing.T) { {name: "KeyTypeParams", value: keeper.KeyTypeParams}, {name: "KeyTypeLastMarketID", value: keeper.KeyTypeLastMarketID}, {name: "KeyTypeKnownMarketID", value: keeper.KeyTypeKnownMarketID}, + {name: "KeyTypeLastOrderID", value: keeper.KeyTypeLastOrderID}, {name: "KeyTypeMarket", value: keeper.KeyTypeMarket}, {name: "KeyTypeOrder", value: keeper.KeyTypeOrder}, {name: "KeyTypeMarketToOrderIndex", value: keeper.KeyTypeMarketToOrderIndex}, @@ -435,6 +443,16 @@ func TestParseKeySuffixKnownMarketID(t *testing.T) { } } +func TestMakeKeyLastOrderID(t *testing.T) { + ktc := keyTestCase{ + maker: func() []byte { + return keeper.MakeKeyLastOrderID() + }, + expected: []byte{keeper.KeyTypeLastOrderID}, + } + checkKey(t, ktc, "MakeKeyLastOrderID") +} + func TestGetKeyPrefixMarket(t *testing.T) { tests := []struct { name string diff --git a/x/exchange/keeper/market.go b/x/exchange/keeper/market.go index 9b61a45031..9264ebbdc1 100644 --- a/x/exchange/keeper/market.go +++ b/x/exchange/keeper/market.go @@ -28,10 +28,9 @@ func setLastAutoMarketID(store sdk.KVStore, marketID uint32) { store.Set(key, value) } -// NextMarketID finds the next available market id, updates the last auto-selected +// nextMarketID finds the next available market id, updates the last auto-selected // market id store entry, and returns the unused id it found. -func (k Keeper) NextMarketID(ctx sdk.Context) uint32 { - store := k.getStore(ctx) +func nextMarketID(store sdk.KVStore) uint32 { marketID := getLastAutoMarketID(store) + 1 for { key := MakeKeyKnownMarketID(marketID) @@ -1161,7 +1160,7 @@ func storeMarket(store sdk.KVStore, market exchange.Market) { // validated and also allows for the market account to already exist. func (k Keeper) initMarket(ctx sdk.Context, store sdk.KVStore, market exchange.Market) { if market.MarketId == 0 { - market.MarketId = k.NextMarketID(ctx) + market.MarketId = nextMarketID(store) } marketID := market.MarketId @@ -1196,8 +1195,10 @@ func (k Keeper) CreateMarket(ctx sdk.Context, market exchange.Market) (uint32, e return 0, errors.Join(errAsk, errBid) } + store := k.getStore(ctx) + if market.MarketId == 0 { - market.MarketId = k.NextMarketID(ctx) + market.MarketId = nextMarketID(store) } marketAddr := exchange.GetMarketAddress(market.MarketId) @@ -1213,7 +1214,7 @@ func (k Keeper) CreateMarket(ctx sdk.Context, market exchange.Market) (uint32, e k.accountKeeper.NewAccount(ctx, marketAcc) k.accountKeeper.SetAccount(ctx, marketAcc) - storeMarket(k.getStore(ctx), market) + storeMarket(store, market) return market.MarketId, nil } diff --git a/x/exchange/keeper/orders.go b/x/exchange/keeper/orders.go index e2dc55e94d..cce16155ab 100644 --- a/x/exchange/keeper/orders.go +++ b/x/exchange/keeper/orders.go @@ -10,31 +10,33 @@ import ( db "github.com/tendermint/tm-db" - "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" "github.com/provenance-io/provenance/x/exchange" ) -// TODO[1658]: Create a last-order-id store entry and use that in getNextOrderID. +// getLastOrderID gets the id of the last order created. +func getLastOrderID(store sdk.KVStore) uint64 { + key := MakeKeyLastOrderID() + value := store.Get(key) + rv, _ := uint64FromBz(value) + return rv +} -// getNextOrderID gets the next available order id from the store. -func (k Keeper) getNextOrderID(ctx sdk.Context) uint64 { - store := prefix.NewStore(k.getStore(ctx), GetKeyPrefixOrder()) - iter := store.ReverseIterator(nil, nil) - defer iter.Close() +// setLastOrderID sets the id of the last order created. +func setLastOrderID(store sdk.KVStore, orderID uint64) { + key := MakeKeyLastOrderID() + value := uint64Bz(orderID) + store.Set(key, value) +} - toAdd := uint64(1) - for ; iter.Valid(); iter.Next() { - orderIDBz := iter.Key() - orderID, ok := uint64FromBz(orderIDBz) - if ok { - return orderID + toAdd - } - toAdd++ - } - return toAdd +// nextOrderID finds the next available order id, updates the last order id +// store entry, and returns the unused id it found. +func nextOrderID(store sdk.KVStore) uint64 { + orderID := getLastOrderID(store) + 1 + setLastOrderID(store, orderID) + return orderID } // getOrderStoreKeyValue creates the store key and value representing the provided order. @@ -590,7 +592,7 @@ func (k Keeper) CreateAskOrder(ctx sdk.Context, askOrder exchange.AskOrder, crea } } - orderID := k.getNextOrderID(ctx) + orderID := nextOrderID(store) order := exchange.NewOrder(orderID).WithAsk(&askOrder) if err := k.setOrderInStore(store, *order); err != nil { return 0, fmt.Errorf("error storing ask order: %w", err) @@ -630,7 +632,7 @@ func (k Keeper) CreateBidOrder(ctx sdk.Context, bidOrder exchange.BidOrder, crea } } - orderID := k.getNextOrderID(ctx) + orderID := nextOrderID(store) order := exchange.NewOrder(orderID).WithBid(&bidOrder) if err := k.setOrderInStore(store, *order); err != nil { return 0, fmt.Errorf("error storing bid order: %w", err)