From 3a7cb7e4f6de01a1cadea8992309113f2bfd8d74 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Thu, 9 Jan 2020 13:55:45 +0000 Subject: [PATCH] stop sending zero coins --- x/auction/keeper/auctions.go | 158 ++++++++++++++++-------------- x/auction/keeper/auctions_test.go | 3 +- x/auction/types/auctions.go | 15 +-- x/auction/types/params.go | 4 +- 4 files changed, 100 insertions(+), 80 deletions(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 51c7c804..38108079 100644 --- a/x/auction/keeper/auctions.go +++ b/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) - bidAmtToReturn := a.Bid - if bidder.Equals(a.Bidder) { // catch edge case of someone updating their bid with a low balance - bidAmtToReturn = sdk.NewInt64Coin(a.Bid.Denom, 0) + 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 + // pay back previous bidder + err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(a.Bid)) + if err != nil { + return a, err + } + err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid)) + if err != nil { + return a, err + } } - err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, types.ModuleName, sdk.NewCoins(bidAmtToReturn.Add(increment))) + // 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.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(bidAmtToReturn)) - if err != nil { - return a, err - } - err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(increment)) // increase in bid size is burned - 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,56 +159,71 @@ 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 - if !a.Bid.IsEqual(a.MaxBid) { - // Auction is in forward phase, a bid here can put the auction into forward or reverse phases - if !a.Bid.IsLT(bid) { - return a, sdk.ErrInternal("auction in forward phase, new bid not higher than last bid") + +// TODO naming +func (k Keeper) PlaceBidForwardReverseForward(ctx sdk.Context, a types.ForwardReverseAuction, bidder sdk.AccAddress, bid sdk.Coin) (types.ForwardReverseAuction, sdk.Error) { + // 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") + } + // Move Coins + // 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 a.MaxBid.IsLT(bid) { - return a, sdk.ErrInternal("bid higher than max bid") - } - if lot.IsNegative() || a.Lot.IsLT(lot) { - return a, sdk.ErrInternal("lot out of bounds") - } - if lot.IsLT(a.Lot) && !bid.IsEqual(a.MaxBid) { - return a, sdk.ErrInternal("auction cannot enter reverse phase without bidding max bid") - } - } else { - // Auction is in reverse phase, it can never leave reverse phase - if !bid.IsEqual(a.MaxBid) { - return a, sdk.ErrInternal("") // not necessary - } - if lot.IsNegative() { - return a, sdk.ErrInternal("can't bid negative amount") - } - if !lot.IsLT(a.Lot) { - return a, sdk.ErrInternal("auction in reverse phase, new bid not less than previous amount") + err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid)) + if err != nil { + return a, err } } + // pay increase in bid to auction initiator + err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, bidder, a.Initiator, sdk.NewCoins(bid.Sub(a.Bid))) + 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") + } + 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) - bidAmtToReturn := 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)) - if err != nil { - return a, err - } - err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(bidIncrement)) - if err != nil { - return a, err + 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)) + if err != nil { + return a, err + } + err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid)) + 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,17 +256,15 @@ 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 - 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)) - if err != nil { - return a, err + 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)) + if err != nil { + return a, err + } + err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Bid)) + if err != nil { + return a, err + } } // Update Auction diff --git a/x/auction/keeper/auctions_test.go b/x/auction/keeper/auctions_test.go index 696c3382..1f10a28c 100644 --- a/x/auction/keeper/auctions_test.go +++ b/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 diff --git a/x/auction/types/auctions.go b/x/auction/types/auctions.go index 1aafaa36..fa5e1163 100644 --- a/x/auction/types/auctions.go +++ b/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 - // 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 SendCoinsFromModuleToModule. Not a problem currently but if checks/logic regarding modules accounts where added to those methods they would be bypassed. - // Alternative: set address to nil, and catch it in an if statement in place bid + // 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) + // Setting to the module account address bypasses calling supply.SendCoinsFromModuleToModule, instead calls SendCoinsFromModuleToAccount. + // This isn't a problem currently, but if additional logic/validation was added for sending to coins to Module Accounts, it would be bypassed. 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}, diff --git a/x/auction/types/params.go b/x/auction/types/params.go index 1aa0276d..0bbf075d 100644 --- a/x/auction/types/params.go +++ b/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 time.Duration = 3 * time.Hour + // DefaultBidDuration how long an auction gets extended when someone bids + DefaultBidDuration time.Duration = 1 * time.Hour ) // Parameter keys