move index updates to Get/Set for more safety

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 {

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)
 

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
+// StoreNewAuction stores an auction, adding a new ID
-func (k Keeper) storeNewAuction(ctx sdk.Context, auction types.Auction) (types.ID, sdk.Error) {
+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
+	// remove the auction from the byTime index if it is already in there
-	// existingAuction, found := k.GetAuction(ctx, auction.GetID())
+	existingAuction, found := k.GetAuction(ctx, auction.GetID())
-	// if found {
+	if found {
-	// 	k.removeFromQueue(ctx, existingAuction.GetEndTime(), existingAuction.GetID())
+		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
+	// add to index
-	//k.InsertIntoQueue(ctx, auction.GetEndTime(), auction.GetID())
+	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
+	// remove from index
-	//auction, found := k.GetAuction(ctx, auctionID)
+	auction, found := k.GetAuction(ctx, auctionID)
-	// if found {
+	if found {
-	// 	k.removeFromQueue(ctx, auction.GetEndTime(), auctionID)
+		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
+// InsertIntoIndex adds an auction ID and end time into the byTime index
-func (k Keeper) InsertIntoQueue(ctx sdk.Context, endTime time.Time, auctionID types.ID) {
+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
+// RemoveFromIndex removes an auction ID and end time from the byTime index
-func (k Keeper) RemoveFromQueue(ctx sdk.Context, endTime time.Time, auctionID types.ID) {
+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
+// IterateAuctions provides an iterator over all stored auctions.
-// each auction, cb will be called. If the cb returns true, the iterator
+// For each auction, cb will be called. If cb returns true, the iterator will close and stop.
-// 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)
 

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
+	// check auction is in the index
-	// iter := keeper.GetQueueIterator(ctx, 100000)
+	keeper.IterateAuctionsByTime(ctx, auction.GetEndTime(), func(readID types.ID) bool {
-	// require.Equal(t, 1, len(convertIteratorToSlice(keeper, iter)))
+		require.Equal(t, auction.GetID(), readID)
-	// iter.Close()
+		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
+	// check auction not in index
-	// iter = keeper.GetQueueIterator(ctx, 100000)
+	keeper.IterateAuctionsByTime(ctx, time.Unix(999999999, 0), func(readID types.ID) bool {
-	// require.Equal(t, 0, len(convertIteratorToSlice(keeper, iter)))
+		require.Fail(t, "index should be empty", " found auction ID '%s", readID)
-	// iter.Close()
+		return false
-
+	})
 }
 
 func TestIncrementNextAuctionID(t *testing.T) {
@@ -66,27 +65,32 @@ func TestIncrementNextAuctionID(t *testing.T) {
 
 }
 
-// func TestIterateAuctions(t *testing.T) {
+func TestIterateAuctions(t *testing.T) {
-// 	// setup keeper
+	// setup
-// 	tApp := app.NewTestApp()
+	tApp := app.NewTestApp()
-// 	keeper := tApp.GetAuctionKeeper()
+	tApp.InitializeFromGenesisStates()
-// 	ctx := tApp.NewContext(true, abci.Header{})
+	keeper := tApp.GetAuctionKeeper()
+	ctx := tApp.NewContext(true, abci.Header{})
 
-// 	auctions := []types.Auction{
+	auctions := []types.Auction{
-// 		&types.ForwardAuction{},
+		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),
-// 	for _, a := range auctions {
+		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),
-// 		keeper.SetAuction(ctx, a)
+	}
-// 	}
+	for _, a := range auctions {
+		keeper.SetAuction(ctx, a)
+	}
 
-// 	var readAuctions []types.Auction
+	// run
-// 	keeper.IterateAuctions(ctx, func(a types.Auction) bool {
+	var readAuctions []types.Auction
-// 		readAuctions = append(readAuctions, a)
+	keeper.IterateAuctions(ctx, func(a types.Auction) bool {
-// 		return false
+		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
-}