stop sending zero coins

x/auction/keeper/auctions.go

@@ -34,7 +34,7 @@ func (k Keeper) StartReverseAuction(ctx sdk.Context, buyer string, bid sdk.Coin,
 
 	// This auction type mints coins at close. Need to check module account has minting privileges to avoid potential err in endblocker.
 	macc := k.supplyKeeper.GetModuleAccount(ctx, buyer)
-	if !macc.HasPermission(supply.Minter) { // TODO ideally don't want to import supply
+	if !macc.HasPermission(supply.Minter) {
 		return 0, sdk.ErrInternal("module does not have minting permissions")
 	}
 	// store the auction
@@ -103,7 +103,11 @@ func (k Keeper) PlaceBid(ctx sdk.Context, auctionID uint64, bidder sdk.AccAddres
 			return err
 		}
 	case types.ForwardReverseAuction:
-		a, err = k.PlaceBidForwardReverse(ctx, auc, bidder, bid, lot)
+		if !auc.IsReversePhase() {
+			a, err = k.PlaceBidForwardReverseForward(ctx, auc, bidder, bid)
+		} else {
+			a, err = k.PlaceBidForwardReverseReverse(ctx, auc, bidder, lot)
+		}
 		if err != nil {
 			return err
 		}
@@ -126,24 +130,23 @@ func (k Keeper) PlaceBidForward(ctx sdk.Context, a types.ForwardAuction, bidder
 	}
 
 	// Move Coins
-	increment := bid.Sub(a.Bid)
+	if !bidder.Equals(a.Bidder) && !a.Bid.IsZero() { // catch edge case of someone updating their bid with a low balance, also don't send if amt is zero
-	bidAmtToReturn := a.Bid
+		// pay back previous bidder
-	if bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance
+		err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(a.Bid))
-		bidAmtToReturn = sdk.NewInt64Coin(a.Bid.Denom, 0)
-	}
-	err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(bidAmtToReturn.Add(increment)))
 		if err != nil {
 			return a, err
 		}
-	err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(bidAmtToReturn))
+		err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid))
 		if err != nil {
 			return a, err
 		}
-	err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(increment)) // increase in bid size is burned
+	}
+	// burn increase in bid
+	err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, a.Initiator, sdk.NewCoins(bid.Sub(a.Bid)))
 	if err != nil {
 		return a, err
 	}
-	err = k.supplyKeeper.BurnCoins(ctx, a.Initiator, sdk.NewCoins(increment))
+	err = k.supplyKeeper.BurnCoins(ctx, a.Initiator, sdk.NewCoins(bid.Sub(a.Bid)))
 	if err != nil {
 		return a, err
 	}
@@ -156,26 +159,50 @@ func (k Keeper) PlaceBidForward(ctx sdk.Context, a types.ForwardAuction, bidder
 
 	return a, nil
 }
-func (k Keeper) PlaceBidForwardReverse(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, bid sdk.Coin, lot sdk.Coin) (types.ForwardReverseAuction, sdk.Error) {
+
-	// Validate New Bid // TODO min bid increments, make validation code less confusing
+// TODO naming
-	if !a.Bid.IsEqual(a.MaxBid) {
+func (k Keeper) PlaceBidForwardReverseForward(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, bid sdk.Coin) (types.ForwardReverseAuction, sdk.Error) {
-		// Auction is in forward phase, a bid here can put the auction into forward or reverse phases
+	// Validate bid
+	if a.IsReversePhase() {
+		return a, sdk.ErrInternal("auction is not in forward phase")
+	}
 	if !a.Bid.IsLT(bid) {
 		return a, sdk.ErrInternal("auction in forward phase, new bid not higher than last bid")
 	}
 	if a.MaxBid.IsLT(bid) {
 		return a, sdk.ErrInternal("bid higher than max bid")
 	}
-		if lot.IsNegative() || a.Lot.IsLT(lot) {
+	// Move Coins
-			return a, sdk.ErrInternal("lot out of bounds")
+	// pay back previous bidder
+	if !bidder.Equals(a.Bidder) && !a.Bid.IsZero() { // catch edge case of someone updating their bid with a low balance, also don't send if amt is zero
+		err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(a.Bid))
+		if err != nil {
+			return a, err
 		}
-		if lot.IsLT(a.Lot) && !bid.IsEqual(a.MaxBid) {
+		err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid))
-			return a, sdk.ErrInternal("auction cannot enter reverse phase without bidding max bid")
+		if err != nil {
+			return a, err
 		}
-	} else {
+	}
-		// Auction is in reverse phase, it can never leave reverse phase
+	// pay increase in bid to auction initiator
-		if !bid.IsEqual(a.MaxBid) {
+	err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, a.Initiator, sdk.NewCoins(bid.Sub(a.Bid)))
-			return a, sdk.ErrInternal("") // not necessary
+	if err != nil {
+		return a, err
+	}
+
+	// Update Auction
+	a.Bidder = bidder
+	a.Bid = bid
+	// increment timeout
+	a.EndTime = earliestTime(ctx.BlockTime().Add(types.DefaultBidDuration), a.MaxEndTime)
+
+	return a, nil
+}
+
+func (k Keeper) PlaceBidForwardReverseReverse(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, lot sdk.Coin) (types.ForwardReverseAuction, sdk.Error) {
+	// Validate bid
+	if !a.IsReversePhase() {
+		return a, sdk.ErrInternal("auction not in reverse phase")
 	}
 	if lot.IsNegative() {
 		return a, sdk.ErrInternal("can't bid negative amount")
@@ -183,29 +210,20 @@ func (k Keeper) PlaceBidForwardReverse(ctx sdk.Context, a types.ForwardReverseAu
 	if !lot.IsLT(a.Lot) {
 		return a, sdk.ErrInternal("auction in reverse phase, new bid not less than previous amount")
 	}
-	}
 
 	// Move Coins
-	bidIncrement := bid.Sub(a.Bid)
+	if !bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance
-	bidAmtToReturn := a.Bid
+		err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(a.Bid))
-	lotDecrement := a.Lot.Sub(lot)
-	if bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance
-		bidAmtToReturn = sdk.NewInt64Coin(a.Bid.Denom, 0)
-	}
-	err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(bidAmtToReturn.Add(bidIncrement)))
 		if err != nil {
 			return a, err
 		}
-	err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(bidAmtToReturn))
+		err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid))
 		if err != nil {
 			return a, err
 		}
-	err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(bidIncrement))
-	if err != nil {
-		return a, err
 	}
 	// FIXME paying out rateably to cdp depositors is vulnerable to errors compounding over multiple bids
-	lotPayouts, err := splitCoinIntoWeightedBuckets(lotDecrement, a.LotReturns.Weights)
+	lotPayouts, err := splitCoinIntoWeightedBuckets(a.Lot.Sub(lot), a.LotReturns.Weights)
 	if err != nil {
 		return a, err
 	}
@@ -219,12 +237,12 @@ func (k Keeper) PlaceBidForwardReverse(ctx sdk.Context, a types.ForwardReverseAu
 	// Update Auction
 	a.Bidder = bidder
 	a.Lot = lot
-	a.Bid = bid
 	// increment timeout
 	a.EndTime = earliestTime(ctx.BlockTime().Add(types.DefaultBidDuration), a.MaxEndTime)
 
 	return a, nil
 }
+
 func (k Keeper) PlaceBidReverse(ctx sdk.Context, a types.ReverseAuction, bidder sdk.AccAddress, lot sdk.Coin) (types.ReverseAuction, sdk.Error) {
 	// Validate New Bid
 	if lot.Denom != a.Lot.Denom {
@@ -238,18 +256,16 @@ func (k Keeper) PlaceBidReverse(ctx sdk.Context, a types.ReverseAuction, bidder
 	}
 
 	// Move Coins
-	bidAmtToReturn := a.Bid
+	if !bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance
-	if bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance
+		err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(a.Bid))
-		bidAmtToReturn = sdk.NewInt64Coin(a.Bid.Denom, 0)
-	}
-	err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(bidAmtToReturn))
 		if err != nil {
 			return a, err
 		}
-	err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(bidAmtToReturn))
+		err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid))
 		if err != nil {
 			return a, err
 		}
+	}
 
 	// Update Auction
 	a.Bidder = bidder

x/auction/keeper/auctions_test.go

@@ -140,7 +140,8 @@ func TestForwardReverseAuctionBasic(t *testing.T) {
 	}
 
 	// Place a reverse bid
-	require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50), c("token1", 15))) // bid, lot
+	require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50), c("token1", 15))) // first bid up to max bid to switch phases
+	require.NoError(t, keeper.PlaceBid(ctx, 0, buyer, c("token2", 50), c("token1", 15)))
 	// Check bidder's coins have decreased
 	tApp.CheckBalance(t, ctx, buyer, cs(c("token1", 100), c("token2", 50)))
 	// Check seller's coins have increased

x/auction/types/auctions.go

@@ -72,7 +72,7 @@ func NewForwardAuction(seller string, lot sdk.Coin, bidDenom string, endTime tim
 		// no ID
 		Initiator:  seller,
 		Lot:        lot,
-		Bidder:     nil, // TODO on the first place bid, 0 coins will be sent to this address, check if this causes problems or can be avoided
+		Bidder:     nil,
 		Bid:        sdk.NewInt64Coin(bidDenom, 0),
 		EndTime:    endTime,
 		MaxEndTime: endTime,
@@ -90,10 +90,9 @@ func (a ReverseAuction) WithID(id uint64) Auction { a.ID = id; return a }
 
 // NewReverseAuction creates a new reverse auction
 func NewReverseAuction(buyerModAccName string, bid sdk.Coin, initialLot sdk.Coin, EndTime time.Time) ReverseAuction {
-	// TODO setting the bidder here is a bit hacky
+	// Note: Bidder is set to the initiator's module account address instead of module name. (when the first bid is placed, it is paid out to the initiator)
-	// Needs to be set so that when the first bid is placed, it is paid out to the initiator.
+	// Setting to the module account address bypasses calling supply.SendCoinsFromModuleToModule, instead calls SendCoinsFromModuleToAccount.
-	// Setting to the module account address bypasses calling supply.SendCoinsFromModuleToModule, instead calls SendCoinsFromModuleToModule. Not a problem currently but if checks/logic regarding modules accounts where added to those methods they would be bypassed.
+	// This isn't a problem currently, but if additional logic/validation was added for sending to coins to Module Accounts, it would be bypassed.
-	// Alternative: set address to nil, and catch it in an if statement in place bid
 	auction := ReverseAuction{BaseAuction{
 		// no ID
 		Initiator:  buyerModAccName,
@@ -116,6 +115,10 @@ type ForwardReverseAuction struct {
 // WithID returns an auction with the ID set
 func (a ForwardReverseAuction) WithID(id uint64) Auction { a.ID = id; return a }
 
+func (a ForwardReverseAuction) IsReversePhase() bool {
+	return a.Bid.IsEqual(a.MaxBid)
+}
+
 func (a ForwardReverseAuction) String() string {
 	return fmt.Sprintf(`Auction %d:
   Initiator:              %s
@@ -139,7 +142,7 @@ func NewForwardReverseAuction(seller string, lot sdk.Coin, EndTime time.Time, ma
 			// no ID
 			Initiator:  seller,
 			Lot:        lot,
-			Bidder:     nil, // TODO on the first place bid, 0 coins will be sent to this address, check if this causes problems or can be avoided
+			Bidder:     nil,
 			Bid:        sdk.NewInt64Coin(maxBid.Denom, 0),
 			EndTime:    EndTime,
 			MaxEndTime: EndTime},

x/auction/types/params.go

@@ -12,8 +12,8 @@ import (
 const (
 	// DefaultMaxAuctionDuration max length of auction
 	DefaultMaxAuctionDuration time.Duration = 2 * 24 * time.Hour
-	// DefaultBidDuration how long an auction gets extended when someone bids, roughly 3 hours in blocks
+	// DefaultBidDuration how long an auction gets extended when someone bids
-	DefaultBidDuration time.Duration = 3 * time.Hour
+	DefaultBidDuration time.Duration = 1 * time.Hour
 )
 
 // Parameter keys