From db3c39aaa525fa40f000daea8bb4edddbd45eed6 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Tue, 31 Dec 2019 11:56:39 +0000 Subject: [PATCH] move index updates to Get/Set for more safety --- x/auction/keeper/auctions.go | 14 ++---- x/auction/keeper/auctions_test.go | 2 +- x/auction/keeper/keeper.go | 47 ++++++++++---------- x/auction/keeper/keeper_test.go | 74 ++++++++++++++----------------- 4 files changed, 61 insertions(+), 76 deletions(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 96d305bd..0ece2e5c 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -20,7 +20,7 @@ func (k Keeper) StartForwardAuction(ctx sdk.Context, seller string, lot sdk.Coin return 0, err } // store the auction - auctionID, err := k.storeNewAuction(ctx, auction) + auctionID, err := k.StoreNewAuction(ctx, auction) if err != nil { return 0, err } @@ -38,7 +38,7 @@ func (k Keeper) StartReverseAuction(ctx sdk.Context, buyer string, bid sdk.Coin, return 0, sdk.ErrInternal("module does not have minting permissions") } // store the auction - auctionID, err := k.storeNewAuction(ctx, auction) + auctionID, err := k.StoreNewAuction(ctx, auction) if err != nil { return 0, err } @@ -60,7 +60,7 @@ func (k Keeper) StartForwardReverseAuction(ctx sdk.Context, seller string, lot s return 0, err } // store the auction - auctionID, err := k.storeNewAuction(ctx, auction) + auctionID, err := k.StoreNewAuction(ctx, auction) if err != nil { return 0, err } @@ -112,13 +112,7 @@ func (k Keeper) PlaceBid(ctx sdk.Context, auctionID types.ID, bidder sdk.AccAddr } // store updated auction - existing, found := k.GetAuction(ctx, a.GetID()) - if found { - k.RemoveFromQueue(ctx, existing.GetEndTime(), existing.GetID()) - } k.SetAuction(ctx, a) - k.InsertIntoQueue(ctx, a.GetEndTime(), a.GetID()) - return nil } @@ -298,8 +292,6 @@ func (k Keeper) CloseAuction(ctx sdk.Context, auctionID types.ID) sdk.Error { // Delete auction from store (and queue) k.DeleteAuction(ctx, auctionID) - k.RemoveFromQueue(ctx, auction.GetEndTime(), auction.GetID()) - return nil } func (k Keeper) MintAndPayoutAuctionLot(ctx sdk.Context, a types.ReverseAuction) sdk.Error { diff --git a/x/auction/keeper/auctions_test.go b/x/auction/keeper/auctions_test.go index aad049a3..e3ea8028 100644 --- a/x/auction/keeper/auctions_test.go +++ b/x/auction/keeper/auctions_test.go @@ -99,7 +99,7 @@ func TestForwardReverseAuctionBasic(t *testing.T) { _, addrs := app.GeneratePrivKeyAddressPairs(4) buyer := addrs[0] returnAddrs := addrs[1:] - returnWeights := []sdk.Int{i(30), i(20), i(10)} + returnWeights := is(30, 20, 10) sellerModName := liquidator.ModuleName sellerAddr := supply.NewModuleAddress(sellerModName) diff --git a/x/auction/keeper/keeper.go b/x/auction/keeper/keeper.go index dfb7dd49..d665599b 100644 --- a/x/auction/keeper/keeper.go +++ b/x/auction/keeper/keeper.go @@ -57,8 +57,8 @@ func (k Keeper) IncrementNextAuctionID(ctx sdk.Context) sdk.Error { return nil } -// storeNewAuction stores an auction, adding a new ID, and setting indexes -func (k Keeper) storeNewAuction(ctx sdk.Context, auction types.Auction) (types.ID, sdk.Error) { +// StoreNewAuction stores an auction, adding a new ID +func (k Keeper) StoreNewAuction(ctx sdk.Context, auction types.Auction) (types.ID, sdk.Error) { newAuctionID, err := k.GetNextAuctionID(ctx) if err != nil { return 0, err @@ -66,7 +66,6 @@ func (k Keeper) storeNewAuction(ctx sdk.Context, auction types.Auction) (types.I auction = auction.WithID(newAuctionID) k.SetAuction(ctx, auction) - k.InsertIntoQueue(ctx, auction.GetEndTime(), auction.GetID()) err = k.IncrementNextAuctionID(ctx) if err != nil { @@ -75,24 +74,22 @@ func (k Keeper) storeNewAuction(ctx sdk.Context, auction types.Auction) (types.I return newAuctionID, nil } -// TODO should get/set/delete be responsible for updating auctionByTime index? - // SetAuction puts the auction into the database and adds it to the queue // it overwrites any pre-existing auction with same ID func (k Keeper) SetAuction(ctx sdk.Context, auction types.Auction) { - // remove the auction from the queue if it is already in there - // existingAuction, found := k.GetAuction(ctx, auction.GetID()) - // if found { - // k.removeFromQueue(ctx, existingAuction.GetEndTime(), existingAuction.GetID()) - // } + // remove the auction from the byTime index if it is already in there + existingAuction, found := k.GetAuction(ctx, auction.GetID()) + if found { + k.RemoveFromIndex(ctx, existingAuction.GetEndTime(), existingAuction.GetID()) + } // store auction store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionKeyPrefix) bz := k.cdc.MustMarshalBinaryLengthPrefixed(auction) store.Set(types.GetAuctionKey(auction.GetID()), bz) - // add to the queue - //k.InsertIntoQueue(ctx, auction.GetEndTime(), auction.GetID()) + // add to index + k.InsertIntoIndex(ctx, auction.GetEndTime(), auction.GetID()) } // getAuction gets an auction from the store by auctionID @@ -111,29 +108,32 @@ func (k Keeper) GetAuction(ctx sdk.Context, auctionID types.ID) (types.Auction, // DeleteAuction removes an auction from the store without any validation func (k Keeper) DeleteAuction(ctx sdk.Context, auctionID types.ID) { - // remove from queue - //auction, found := k.GetAuction(ctx, auctionID) - // if found { - // k.removeFromQueue(ctx, auction.GetEndTime(), auctionID) - // } + // remove from index + auction, found := k.GetAuction(ctx, auctionID) + if found { + k.RemoveFromIndex(ctx, auction.GetEndTime(), auctionID) + } // delete auction store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionKeyPrefix) store.Delete(types.GetAuctionKey(auctionID)) } -// Inserts a AuctionID into the queue at endTime -func (k Keeper) InsertIntoQueue(ctx sdk.Context, endTime time.Time, auctionID types.ID) { +// InsertIntoIndex adds an auction ID and end time into the byTime index +func (k Keeper) InsertIntoIndex(ctx sdk.Context, endTime time.Time, auctionID types.ID) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionByTimeKeyPrefix) store.Set(types.GetAuctionByTimeKey(endTime, auctionID), auctionID.Bytes()) } -// removes an auctionID from the queue -func (k Keeper) RemoveFromQueue(ctx sdk.Context, endTime time.Time, auctionID types.ID) { +// RemoveFromIndex removes an auction ID and end time from the byTime index +func (k Keeper) RemoveFromIndex(ctx sdk.Context, endTime time.Time, auctionID types.ID) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionByTimeKeyPrefix) store.Delete(types.GetAuctionByTimeKey(endTime, auctionID)) } +// IterateAuctionByTime provides an iterator over auctions ordered by auction.EndTime. +// For each auction cb will be callled. If cb returns true the iterator will close and stop. +// TODO can the cutoff time be removed in favour of caller specifying cutoffs in the callback? func (k Keeper) IterateAuctionsByTime(ctx sdk.Context, inclusiveCutoffTime time.Time, cb func(auctionID types.ID) (stop bool)) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionByTimeKeyPrefix) iterator := store.Iterator( @@ -152,9 +152,8 @@ func (k Keeper) IterateAuctionsByTime(ctx sdk.Context, inclusiveCutoffTime time. } } -// IterateAuctions provides an iterator over all stored auctions. For -// each auction, cb will be called. If the cb returns true, the iterator -// will close and stop. +// IterateAuctions provides an iterator over all stored auctions. +// For each auction, cb will be called. If cb returns true, the iterator will close and stop. func (k Keeper) IterateAuctions(ctx sdk.Context, cb func(auction types.Auction) (stop bool)) { iterator := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), types.AuctionKeyPrefix) diff --git a/x/auction/keeper/keeper_test.go b/x/auction/keeper/keeper_test.go index f6e9a1d9..3667aeb8 100644 --- a/x/auction/keeper/keeper_test.go +++ b/x/auction/keeper/keeper_test.go @@ -4,12 +4,10 @@ import ( "testing" "time" - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" abci "github.com/tendermint/tendermint/abci/types" "github.com/kava-labs/kava/app" - "github.com/kava-labs/kava/x/auction/keeper" "github.com/kava-labs/kava/x/auction/types" ) @@ -29,10 +27,11 @@ func SetGetDeleteAuction(t *testing.T) { // check before and after match require.True(t, found) require.Equal(t, auction, readAuction) - // check auction is in queue - // iter := keeper.GetQueueIterator(ctx, 100000) - // require.Equal(t, 1, len(convertIteratorToSlice(keeper, iter))) - // iter.Close() + // check auction is in the index + keeper.IterateAuctionsByTime(ctx, auction.GetEndTime(), func(readID types.ID) bool { + require.Equal(t, auction.GetID(), readID) + return false + }) // delete auction keeper.DeleteAuction(ctx, id) @@ -40,11 +39,11 @@ func SetGetDeleteAuction(t *testing.T) { // check auction does not exist _, found = keeper.GetAuction(ctx, id) require.False(t, found) - // check auction not in queue - // iter = keeper.GetQueueIterator(ctx, 100000) - // require.Equal(t, 0, len(convertIteratorToSlice(keeper, iter))) - // iter.Close() - + // check auction not in index + keeper.IterateAuctionsByTime(ctx, time.Unix(999999999, 0), func(readID types.ID) bool { + require.Fail(t, "index should be empty", " found auction ID '%s", readID) + return false + }) } func TestIncrementNextAuctionID(t *testing.T) { @@ -66,27 +65,32 @@ func TestIncrementNextAuctionID(t *testing.T) { } -// func TestIterateAuctions(t *testing.T) { -// // setup keeper -// tApp := app.NewTestApp() -// keeper := tApp.GetAuctionKeeper() -// ctx := tApp.NewContext(true, abci.Header{}) +func TestIterateAuctions(t *testing.T) { + // setup + tApp := app.NewTestApp() + tApp.InitializeFromGenesisStates() + keeper := tApp.GetAuctionKeeper() + ctx := tApp.NewContext(true, abci.Header{}) -// auctions := []types.Auction{ -// &types.ForwardAuction{}, -// } -// for _, a := range auctions { -// keeper.SetAuction(ctx, a) -// } + auctions := []types.Auction{ + types.NewForwardAuction("sellerMod", c("denom", 12345678), "anotherdenom", time.Date(1998, time.January, 1, 0, 0, 0, 0, time.UTC)).WithID(0), + types.NewReverseAuction("buyerMod", c("denom", 12345678), c("anotherdenom", 12345678), time.Date(1998, time.January, 1, 0, 0, 0, 0, time.UTC)).WithID(1), + types.NewForwardReverseAuction("sellerMod", c("denom", 12345678), time.Date(1998, time.January, 1, 0, 0, 0, 0, time.UTC), c("anotherdenom", 12345678), types.WeightedAddresses{}).WithID(2), + } + for _, a := range auctions { + keeper.SetAuction(ctx, a) + } -// var readAuctions []types.Auction -// keeper.IterateAuctions(ctx, func(a types.Auction) bool { -// readAuctions = append(readAuctions, a) -// return false -// }) + // run + var readAuctions []types.Auction + keeper.IterateAuctions(ctx, func(a types.Auction) bool { + readAuctions = append(readAuctions, a) + return false + }) -// require.Equal(t, auctions, readAuctions) -// } + // check + require.Equal(t, auctions, readAuctions) +} func TestIterateAuctionsByTime(t *testing.T) { // setup keeper @@ -109,7 +113,7 @@ func TestIterateAuctionsByTime(t *testing.T) { {time.Date(9999, time.January, 1, 0, 0, 0, 0, time.UTC), 0}, // distant future } for _, v := range byTimeIndex { - keeper.InsertIntoQueue(ctx, v.endTime, v.auctionID) + keeper.InsertIntoIndex(ctx, v.endTime, v.auctionID) } // read out values from index up to a cutoff time and check they are as expected @@ -129,13 +133,3 @@ func TestIterateAuctionsByTime(t *testing.T) { require.Equal(t, expectedIndex, readIndex) } - -func convertIteratorToSlice(keeper keeper.Keeper, iterator sdk.Iterator) []types.ID { - var queue []types.ID - for ; iterator.Valid(); iterator.Next() { - var auctionID types.ID - types.ModuleCdc.MustUnmarshalBinaryLengthPrefixed(iterator.Value(), &auctionID) - queue = append(queue, auctionID) - } - return queue -}