Refactor dex EndBlock to optimize store access #763

codchen · 2023-05-08T09:30:23Z

Describe your changes and provide context

dex EndBlock would always load all order book entries before processing, even if there is no or very few incoming orders to match against. This PR changes it so that entries are only loaded as needed. The core logic changes are in:

types/orders.go
exchange/limit_order.go
exchange/market_order.go

Migrations are needed due to order book entry key encoding changes, which are necessary to make order book entries sorted in store. There is also a new parameter added.

Testing performed to validate your change

unit test

codchen · 2023-05-08T13:30:16Z

x/dex/exchange/limit_order.go

 			),
 		)
-		minPrice = sdk.MinDec(minPrice, orderbook.Longs.Entries[longPtr].GetPrice())
-		maxPrice = sdk.MaxDec(maxPrice, orderbook.Longs.Entries[longPtr].GetPrice())
+		minPrice = sdk.MinDec(minPrice, longEntry.GetPrice()) // tony: need to revisit whether it's intended to use long entry price here. Keeping it as-is so that test expectations are met


@LCyson is it intentional here to use long entry price to get minPrice? not sure how minPrice is used with TriggeredOrder

hm yea likely an issue here, minPrice should be the lowest short

we can leave it as it is for now, we already remove support for TRIGGER ORDER correct

LCyson · 2023-05-08T18:11:05Z

x/dex/keeper/long_book.go

+	return
+}
+
+func (k Keeper) GetTopNLongBooksForPairStarting(ctx sdk.Context, contractAddr string, priceDenom string, assetDenom string, n int, startExclusive sdk.Dec) (list []types.OrderBookEntry) {


can we add comment here - function description + parameters description

LCyson · 2023-05-08T18:37:29Z

x/dex/keeper/long_book.go

+	defer iterator.Close()
+
+	// Fast-forward
+	// TODO: add iterator interface that allows starting at a certain subkey under prefix


this todo is to add a O(logn) access method to replace the current O(n) way we're doing this correct

LCyson · 2023-05-08T18:46:53Z

x/dex/utils/dec.go

+)
+
+func DecToBigEndian(d sdk.Dec) (res []byte) {
+	// if d.IsNegative() {


remove comments

LCyson · 2023-05-08T18:51:04Z

x/dex/utils/dec.go

+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+func DecToBigEndian(d sdk.Dec) (res []byte) {


is there a lib we can leverage, feel less error-prune in that way

not really since Dec is a cosmos struct, and cosmos doesn't have an encoding for Dec that preserves numerical ordering

LCyson · 2023-05-08T18:53:35Z

x/dex/utils/dec_test.go

+)
+
+var DECS = []sdk.Dec{
+	sdk.MustNewDecFromStr("90.0"),


can we add an overflow dec test case here

how to define an overflow dec? I thought dec is supposed to be immune to overflow

LCyson · 2023-05-08T21:40:24Z

x/dex/exchange/limit_order.go

 			),
 		)
-		minPrice = sdk.MinDec(minPrice, orderbook.Longs.Entries[longPtr].GetPrice())
-		maxPrice = sdk.MaxDec(maxPrice, orderbook.Longs.Entries[longPtr].GetPrice())
+		minPrice = sdk.MinDec(minPrice, longEntry.GetPrice()) // tony: need to revisit whether it's intended to use long entry price here. Keeping it as-is so that test expectations are met


hm yea likely an issue here, minPrice should be the lowest short

LCyson · 2023-05-09T05:02:24Z

x/dex/types/orders.go

@@ -3,49 +3,145 @@ package types
 import (


can we add unit tests for new functions in this file

LCyson · 2023-05-09T05:05:32Z

x/dex/types/unit.go

+	Unit_NANO:     sdk.NewDec(1_000_000_000),
+}
+
+func ConvertDecToStandard(unit Unit, dec sdk.Dec) sdk.Dec {


if this is only used by test package can we move this to unit_test

i deleted it since it doesn't seem to be used anywhere

LCyson · 2023-05-09T05:06:56Z

x/dex/types/orders.go

+	c.CachedEntries = append(c.CachedEntries, loaded...)
+}
+
+func (c *CachedSortedOrderBookEntries) SettleQuantity(ctx sdk.Context, quantity sdk.Dec) (res []ToSettle, settled sdk.Dec) {


ditto - add comment

LCyson · 2023-05-09T05:17:13Z

x/dex/types/orders.go

+		} else {
+			newFirstAllocationIdx = idx
+			if settled.Equal(quantity) {
+				break


we should remove allocations[newFirstAllocationIdx] when all its quantity gets settled here right

this is the case where the order is fully settled before applying the current allocation. In other words the order book entry isn't exhausted yet - it still at least has this current allocation left

LCyson · 2023-05-09T05:23:06Z

x/dex/types/orders.go

-	Entries      []OrderBookEntry
-	DirtyEntries *datastructures.TypedSyncMap[string, OrderBookEntry]
+	CachedEntries []OrderBookEntry
+	currentPtr    int


can we init this in the new() function

x/dex/migrations/v13_to_v14.go

+	for key, v := range keyToVal {
+		store.Delete([]byte(key))
+		var val types.LongBook
+		dexkeeper.Cdc.MustUnmarshal(v, &val)
+		newKey := append(types.PairPrefix(val.Entry.PriceDenom, val.Entry.AssetDenom), keeper.GetKeyForPrice(val.Price)...)
+		store.Set(newKey, v)
+	}


x/dex/migrations/v13_to_v14.go

+	for key, v := range keyToVal {
+		store.Delete([]byte(key))
+		var val types.ShortBook
+		dexkeeper.Cdc.MustUnmarshal(v, &val)
+		newKey := append(types.PairPrefix(val.Entry.PriceDenom, val.Entry.AssetDenom), keeper.GetKeyForPrice(val.Price)...)
+		store.Set(newKey, v)
+	}


LCyson · 2023-05-10T01:29:47Z

x/dex/exchange/limit_order.go

 			),
 		)
-		minPrice = sdk.MinDec(minPrice, orderbook.Longs.Entries[longPtr].GetPrice())
-		maxPrice = sdk.MaxDec(maxPrice, orderbook.Longs.Entries[longPtr].GetPrice())
+		minPrice = sdk.MinDec(minPrice, longEntry.GetPrice()) // tony: need to revisit whether it's intended to use long entry price here. Keeping it as-is so that test expectations are met


we can leave it as it is for now, we already remove support for TRIGGER ORDER correct

…lock

x/dex/module.go

@@ -292,23 +297,22 @@
 	iterCounter := len(validContractsInfo)
 	endBlockerStartTime := time.Now()
 	for len(validContractsInfo) > 0 {
-		newValidContractsInfo, newOutOfRentContractsInfo, ctx, ok := contract.EndBlockerAtomic(ctx, &am.keeper, validContractsInfo, am.tracingInfo)
+		newValidContractsInfo, newOutOfRentContractsInfo, failedContractToReasons, ctx, ok := contract.EndBlockerAtomic(ctx, &am.keeper, validContractsInfo, am.tracingInfo)


x/dex/module.go

 			telemetry.IncrCounter(float32(1), am.Name(), "total_unregistered_contracts")
 		}
 		validContractsInfo = am.getAllContractInfo(ctx) // reload contract info to get updated dependencies due to unregister above
-
+		if len(failedContractToReasons) != 0 {
+			dexutils.GetMemState(ctx.Context()).ClearContractToDependencies()


remove "psu-test-order" handler

BrandonWeng · 2023-05-08T13:45:37Z

x/dex/exchange/limit_order.go


-	for longPtr >= 0 && shortPtr < len(orderbook.Shorts.Entries) && orderbook.Longs.Entries[longPtr].GetPrice().GTE(orderbook.Shorts.Entries[shortPtr].GetPrice()) {
+	for longEntry, shortEntry := orderbook.Longs.Next(ctx), orderbook.Shorts.Next(ctx); longEntry != nil && shortEntry != nil && longEntry.GetPrice().GTE(shortEntry.GetPrice()); longEntry, shortEntry = orderbook.Longs.Next(ctx), orderbook.Shorts.Next(ctx) {


nit, can we split this into multiple lines or have a helper method? It's pretty hard to read

codchen commented May 8, 2023

View reviewed changes

LCyson reviewed May 8, 2023

View reviewed changes

LCyson suggested changes May 9, 2023

View reviewed changes

codchen force-pushed the tony-chen-optimize-orderbook branch from f5c7cba to 26eb3d0 Compare May 9, 2023 14:04

github-advanced-security bot found potential problems May 9, 2023

View reviewed changes

LCyson approved these changes May 10, 2023

View reviewed changes

codchen force-pushed the tony-chen-optimize-orderbook branch 2 times, most recently from 6d3edc7 to ff7d17b Compare May 10, 2023 07:00

codchen added 9 commits May 11, 2023 12:57

Refactor dex EndBlock to optimize store access

4a8221e

migration

bc377ef

comments

5d13359

add tests

cd5c263

use mem store

e9a312f

fix test

c2ac95c

bump

8c6b362

upgrade

969d198

[dex] only process contract if there is related order/cancel in the b…

2514a53

…lock

codchen force-pushed the tony-chen-optimize-orderbook branch from 6a284ac to 2514a53 Compare May 11, 2023 08:32

github-advanced-security bot found potential problems May 11, 2023

View reviewed changes

x/dex/module.go Fixed Show fixed Hide fixed

codchen added 2 commits May 11, 2023 18:45

suspend instead of unregister failing contracts

4c2cf12

remove triggered orders

efabc4f

github-advanced-security bot found potential problems May 11, 2023

View reviewed changes

fix tests

5267383

codchen force-pushed the tony-chen-optimize-orderbook branch from 3478d22 to 5267383 Compare May 11, 2023 14:26

codchen and others added 6 commits May 12, 2023 11:01

fix ACL mapping

8b6e53d

prioritized contract register/unregister/unsuspend msg

1769ce5

add type

04500c1

remove "psu-test-order" handler

97ff7c1

remove "psu-test-order" handler

go mod update

b2d2cc8

Merge branch 'master' into tony-chen-optimize-orderbook

947a39c

BrandonWeng approved these changes May 15, 2023

View reviewed changes

BrandonWeng enabled auto-merge (squash) May 15, 2023 16:20

Merge branch 'master' into tony-chen-optimize-orderbook

5ff845a

BrandonWeng merged commit 593632c into master May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dex EndBlock to optimize store access #763

Refactor dex EndBlock to optimize store access #763

codchen commented May 8, 2023 •

edited

Loading

codchen May 8, 2023

LCyson May 8, 2023

LCyson May 10, 2023

LCyson May 8, 2023

codchen May 9, 2023

LCyson May 8, 2023

codchen May 9, 2023

LCyson May 8, 2023

LCyson May 8, 2023

codchen May 9, 2023

LCyson May 8, 2023

codchen May 9, 2023

LCyson May 8, 2023

LCyson May 9, 2023

codchen May 9, 2023

LCyson May 9, 2023

codchen May 9, 2023

LCyson May 9, 2023

codchen May 9, 2023

LCyson May 9, 2023

codchen May 9, 2023

LCyson May 9, 2023

LCyson May 10, 2023

BrandonWeng May 8, 2023


		for longPtr >= 0 && shortPtr < len(orderbook.Shorts.Entries) && orderbook.Longs.Entries[longPtr].GetPrice().GTE(orderbook.Shorts.Entries[shortPtr].GetPrice()) {
		for longEntry, shortEntry := orderbook.Longs.Next(ctx), orderbook.Shorts.Next(ctx); longEntry != nil && shortEntry != nil && longEntry.GetPrice().GTE(shortEntry.GetPrice()); longEntry, shortEntry = orderbook.Longs.Next(ctx), orderbook.Shorts.Next(ctx) {

Refactor dex EndBlock to optimize store access #763

Refactor dex EndBlock to optimize store access #763

Conversation

codchen commented May 8, 2023 • edited Loading

Describe your changes and provide context

Testing performed to validate your change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codchen commented May 8, 2023 •

edited

Loading