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.

Sign up for GitHub

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

delete recent price from db when delisting #753

Merged
merged 5 commits into from
Jun 22, 2020
Merged

delete recent price from db when delisting #753

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7bda47
delete recent price from db when delisting
Jun 17, 2020
db682e3
refactor
Jun 19, 2020
7ad0314
clean invalid recent prices before calculate lotsize
rickyyangz Jun 22, 2020
d73fff5
Revert "clean invalid recent prices before calculate lotsize"
Jun 22, 2020
6d930e6
change method name
Jun 22, 2020
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
1 change: 1 addition & 0 deletions plugins/dex/order/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,7 @@ func (kp *DexKeeper) DelistTradingPair(ctx sdk.Context, symbol string, postAlloc
if err != nil {
kp.logger.Error("delete trading pair error", "err", err.Error())
}
kp.PairMapper.DeleteRecentPrices(ctx, baseAsset, quoteAsset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better wrap a method to remove both from memory and db.

}

func (kp *DexKeeper) expireAllOrders(ctx sdk.Context, symbol string) []chan Transfer {
Expand Down
28 changes: 19 additions & 9 deletions plugins/dex/order/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ func MakeCodec() *wire.Codec {

cdc.RegisterConcrete(OrderBookSnapshot{}, "dex/OrderBookSnapshot", nil)
cdc.RegisterConcrete(ActiveOrders{}, "dex/ActiveOrders", nil)
cdc.RegisterConcrete(store.RecentPrice{}, "dex/RecentPrice", nil)

return cdc
}
Expand Down Expand Up @@ -545,6 +546,8 @@ func setup() (ctx sdk.Context, mapper auth.AccountKeeper, keeper *DexKeeper) {
cdc := wire.NewCodec()
types.RegisterWire(cdc)
wire.RegisterCrypto(cdc)
cdc.RegisterConcrete(dextypes.TradingPair{}, "dex/TradingPair", nil)
cdc.RegisterConcrete(store.RecentPrice{}, "dex/RecentPrice", nil)
mapper = auth.NewAccountKeeper(cdc, capKey, types.ProtoAppAccount)
accountCache := getAccountCache(cdc, ms, capKey)
pairMapper := store.NewTradingPairMapper(cdc, common.PairStoreKey)
Expand Down Expand Up @@ -851,6 +854,7 @@ func TestKeeper_DelistTradingPair(t *testing.T) {
keeper.FeeManager.UpdateConfig(NewTestFeeConfig())
_, acc := testutils.NewAccount(ctx, am, 0)
addr := acc.GetAddress()
ctx = ctx.WithBlockHeight(2000)

tradingPair := dextypes.NewTradingPair("XYZ-000", "BNB", 1e8)
keeper.PairMapper.AddTradingPair(ctx, tradingPair)
Expand All @@ -868,30 +872,36 @@ func TestKeeper_DelistTradingPair(t *testing.T) {
am.SetAccount(ctx, acc)

msg := NewNewOrderMsg(addr, "123456", Side.BUY, "XYZ-000_BNB", 1e6, 1e6)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "1234562", Side.BUY, "XYZ-000_BNB", 1e6, 1e6)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123457", Side.BUY, "XYZ-000_BNB", 2e6, 1e6)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123458", Side.BUY, "XYZ-000_BNB", 3e6, 1e6)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123459", Side.SELL, "XYZ-000_BNB", 5e6, 1e4)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123460", Side.SELL, "XYZ-000_BNB", 6e6, 1e4)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "1234602", Side.SELL, "XYZ-000_BNB", 6e6, 1e4)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123461", Side.SELL, "XYZ-000_BNB", 7e6, 1e4)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)
msg = NewNewOrderMsg(addr, "123462", Side.BUY, "XYZ-000_BNB", 4e6, 1e6)
keeper.AddOrder(OrderInfo{msg, 42, 84, 42, 84, 0, "", 0}, false)
keeper.AddOrder(OrderInfo{msg, 2000, 84, 42, 84, 0, "", 0}, false)

lastTradePrices := make(map[string]int64, 1)
lastTradePrices["XYZ-000_BNB"] = 3e6
keeper.PairMapper.UpdateRecentPrices(ctx, pricesStoreEvery, numPricesStored, lastTradePrices)
assert.Equal(1, len(keeper.GetAllOrders()))
assert.Equal(9, len(keeper.GetAllOrdersForPair("XYZ-000_BNB")))
assert.Equal(1, len(keeper.engines))
assert.Equal(1, len(keeper.PairMapper.GetRecentPrices(ctx, pricesStoreEvery, numPricesStored)))

keeper.DelistTradingPair(ctx, "XYZ-000_BNB", nil)
assert.Equal(0, len(keeper.GetAllOrders()))
assert.Equal(0, len(keeper.engines))
assert.Equal(0, len(keeper.PairMapper.GetRecentPrices(ctx, pricesStoreEvery, numPricesStored)))

expectFees := types.NewFee(sdk.Coins{
sdk.NewCoin("BNB", 10e4),
Expand Down
35 changes: 33 additions & 2 deletions plugins/dex/store/mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type TradingPairMapper interface {
ListAllTradingPairs(ctx sdk.Context) []types.TradingPair
UpdateRecentPrices(ctx sdk.Context, pricesStoreEvery, numPricesStored int64, lastTradePrices map[string]int64)
GetRecentPrices(ctx sdk.Context, pricesStoreEvery, numPricesStored int64) map[string]*utils.FixedSizeRing
DeleteRecentPrices(ctx sdk.Context, baseAsset, quoteAsset string)
}

var _ TradingPairMapper = mapper{}
Expand Down Expand Up @@ -157,7 +158,7 @@ func (m mapper) GetRecentPrices(ctx sdk.Context, pricesStoreEvery, numPricesStor
} else {
recordStarted = true
}
prices := m.decodeRecentPrices(bz, numPricesStored)
prices := m.decodeRecentPrices(bz)
numSymbol := len(prices.Pair)
for i := 0; i < numSymbol; i++ {
symbol := prices.Pair[i]
Expand All @@ -172,6 +173,36 @@ func (m mapper) GetRecentPrices(ctx sdk.Context, pricesStoreEvery, numPricesStor
return recentPrices
}

func (m mapper) DeleteRecentPrices(ctx sdk.Context, baseAsset, quoteAsset string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pass the trading symbol as the parameter

symbolToDelete := dexUtils.Assets2TradingPair(strings.ToUpper(baseAsset), strings.ToUpper(quoteAsset))
store := ctx.KVStore(m.key)
iter := sdk.KVStorePrefixIterator(store, []byte(recentPricesKeyPrefix))
defer iter.Close()

for ; iter.Valid(); iter.Next() {
bz := iter.Value()
prices := m.decodeRecentPrices(bz)
numSymbol := len(prices.Pair)
for i := 0; i < numSymbol; i++ {
symbol := prices.Pair[i]
if symbol == symbolToDelete {
prices.Pair = removePair(prices.Pair, i)
prices.Price = removePrice(prices.Price, i)
bz := m.cdc.MustMarshalBinaryBare(prices)
store.Set(iter.Key(), bz)
break
}
}
}
}

func removePair(pair []string, i int) []string {
return append(pair[:i], pair[i+1:]...)
}
func removePrice(price []int64, i int) []int64 {
return append(price[:i], price[i+1:]...)
}
Copy link
Contributor

@rickyyangz rickyyangz Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest giving the RecentPrice struct a method(like removePair) to remove both pair and price at the same time.


func (m mapper) encodeRecentPrices(recentPrices map[string]int64) []byte {
value := RecentPrice{}
numSymbol := len(recentPrices)
Expand All @@ -195,7 +226,7 @@ func (m mapper) encodeRecentPrices(recentPrices map[string]int64) []byte {
return bz
}

func (m mapper) decodeRecentPrices(bz []byte, numPricesStored int64) *RecentPrice {
func (m mapper) decodeRecentPrices(bz []byte) *RecentPrice {
value := RecentPrice{}
m.cdc.MustUnmarshalBinaryBare(bz, &value)
return &value
Expand Down
106 changes: 106 additions & 0 deletions plugins/dex/store/mapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
package store

import (
"path"
"path/filepath"
"runtime"
"testing"

"github.com/stretchr/testify/require"

abci "github.com/tendermint/tendermint/abci/types"
"github.com/tendermint/tendermint/libs/db"
dbm "github.com/tendermint/tendermint/libs/db"
"github.com/tendermint/tendermint/libs/log"

Expand Down Expand Up @@ -123,3 +127,105 @@ func TestMapper_UpdateRecentPrices(t *testing.T) {
require.Equal(t, int64(5), allRecentPrices["ABC"].Count())
require.Equal(t, []interface{}{int64(10), int64(10), int64(10), int64(10), int64(10)}, allRecentPrices["ABC"].Elements())
}

func TestMapper_DeleteRecentPrices(t *testing.T) {
pairMapper, ctx := setup()
for i := 0; i < 3000; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be much smaller if it is not a benchmark

lastPrices := make(map[string]int64, 2)
lastPrices["ABC_BNB"] = 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make some changes of the prices

lastPrices["ABC_EFG"] = 3
lastPrices["EFG_BNB"] = 3
ctx = ctx.WithBlockHeight(int64(2 * (i + 1)))
pairMapper.UpdateRecentPrices(ctx, 2, 5, lastPrices)
}

allRecentPrices := pairMapper.GetRecentPrices(ctx, 2, 5)
require.Equal(t, 3, len(allRecentPrices))
require.Equal(t, int64(5), allRecentPrices["ABC_BNB"].Count())
require.Equal(t, []interface{}{int64(10), int64(10), int64(10), int64(10), int64(10)}, allRecentPrices["ABC_BNB"].Elements())
require.Equal(t, int64(5), allRecentPrices["ABC_EFG"].Count())
require.Equal(t, []interface{}{int64(3), int64(3), int64(3), int64(3), int64(3)}, allRecentPrices["ABC_EFG"].Elements())
require.Equal(t, int64(5), allRecentPrices["ABC_EFG"].Count())
require.Equal(t, []interface{}{int64(3), int64(3), int64(3), int64(3), int64(3)}, allRecentPrices["EFG_BNB"].Elements())

pairMapper.DeleteRecentPrices(ctx, "ABC", "EFG")
allRecentPrices = pairMapper.GetRecentPrices(ctx, 2, 5)
require.Equal(t, 2, len(allRecentPrices))
require.Equal(t, int64(5), allRecentPrices["ABC_BNB"].Count())
require.Equal(t, []interface{}{int64(10), int64(10), int64(10), int64(10), int64(10)}, allRecentPrices["ABC_BNB"].Elements())
require.Equal(t, int64(5), allRecentPrices["EFG_BNB"].Count())
require.Equal(t, []interface{}{int64(3), int64(3), int64(3), int64(3), int64(3)}, allRecentPrices["EFG_BNB"].Elements())
}

func TestMapper_DeleteOneRecentPrices(t *testing.T) {
pairMapper, ctx := setup()
for i := 0; i < 10; i++ {
lastPrices := make(map[string]int64, 1)
if i < 5 {
lastPrices["ABC_BNB"] = 10
}
ctx = ctx.WithBlockHeight(int64(2 * (i + 1)))
pairMapper.UpdateRecentPrices(ctx, 2, 10, lastPrices)
}

allRecentPrices := pairMapper.GetRecentPrices(ctx, 2, 10)
require.Equal(t, 1, len(allRecentPrices))
require.Equal(t, int64(5), allRecentPrices["ABC_BNB"].Count())
require.Equal(t, []interface{}{int64(10), int64(10), int64(10), int64(10), int64(10)}, allRecentPrices["ABC_BNB"].Elements())

pairMapper.DeleteRecentPrices(ctx, "ABC", "BNB")
allRecentPrices = pairMapper.GetRecentPrices(ctx, 2, 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameters (ctx, 2, 5) are different from the above (ctx, 2, 10).
I suggest using a const instead.

require.Equal(t, 0, len(allRecentPrices))

//allowed to delete again
pairMapper.DeleteRecentPrices(ctx, "ABC", "BNB")
allRecentPrices = pairMapper.GetRecentPrices(ctx, 2, 5)
require.Equal(t, 0, len(allRecentPrices))
}

func BenchmarkMapper_DeleteRecentPrices(b *testing.B) {
db, pairMapper, ctx := setupForBenchTest()
defer db.Close()
b.ResetTimer()
for i := 0; i < b.N; i++ {
pairMapper.DeleteRecentPrices(ctx, string(i), string(i))
}
}

func setupForBenchTest() (dbm.DB, TradingPairMapper, sdk.Context) {
db, ms, key := setupLevelDbMultiStore()
ctx := sdk.NewContext(ms, abci.Header{Height: 1}, sdk.RunTxModeDeliver, log.NewNopLogger())
var cdc = wire.NewCodec()
cdc.RegisterConcrete(dextypes.TradingPair{}, "dex/TradingPair", nil)
cdc.RegisterConcrete(RecentPrice{}, "dex/RecentPrice", nil)
pairMapper := NewTradingPairMapper(cdc, key)
for i := 0; i < 500; i++ {
tradingPair := dextypes.NewTradingPair(string(i), string(i), 102000)
pairMapper.AddTradingPair(ctx, tradingPair)
}

for i := 0; i < 2000; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using some const instead to make it more readable.

lastPrices := make(map[string]int64, 500)
for j := 0; j < 500; j++ {
lastPrices[string(j)+"_"+string(j)] = 8
}
ctx = ctx.WithBlockHeight(int64(1000 * (i + 1)))
pairMapper.UpdateRecentPrices(ctx, 1000, 2000, lastPrices)
}

return db, pairMapper, ctx
}

func setupLevelDbMultiStore() (dbm.DB, sdk.MultiStore, *sdk.KVStoreKey) {
_, b, _, _ := runtime.Caller(0)
basePath := filepath.Dir(b)
db, err := db.NewGoLevelDB("test", path.Join(basePath, "data"))
if err != nil {
panic(err)
}
key := sdk.NewKVStoreKey("pair")
ms := store.NewCommitMultiStore(db)
ms.MountStoreWithDB(key, sdk.StoreTypeIAVL, db)
ms.LoadLatestVersion()
return db, ms, key
}