Add changes from code review

x/auction/keeper/auctions.go

@@ -20,6 +20,7 @@ func (k Keeper) StartSurplusAuction(ctx sdk.Context, seller string, lot sdk.Coin
 		types.DistantFuture,
 	)
 
+	// NOTE: for the duration of the auction the auction module account holds the lot
 	err := k.supplyKeeper.SendCoinsFromModuleToModule(ctx, seller, types.ModuleName, sdk.NewCoins(lot))
 	if err != nil {
 		return 0, err
@@ -36,7 +37,7 @@ func (k Keeper) StartSurplusAuction(ctx sdk.Context, seller string, lot sdk.Coin
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", auction.GetID())),
 			sdk.NewAttribute(types.AttributeKeyAuctionType, auction.GetType()),
 			sdk.NewAttribute(types.AttributeKeyBidDenom, auction.Bid.Denom),
-			sdk.NewAttribute(types.AttributeKeyLotDenom, auction.Lot.Denom),
+			sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()),
 		),
 	)
 	return auctionID, nil
@@ -50,14 +51,17 @@ func (k Keeper) StartDebtAuction(ctx sdk.Context, buyer string, bid sdk.Coin, in
 		bid,
 		initialLot,
 		types.DistantFuture,
-		debt)
+		debt,
+	)
 
 	// 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: this should panic?
 		return 0, sdkerrors.Wrap(types.ErrInvalidModulePermissions, supply.Minter)
 	}
 
+	// NOTE: for the duration of the auction the auction module account holds the debt
 	err := k.supplyKeeper.SendCoinsFromModuleToModule(ctx, buyer, types.ModuleName, sdk.NewCoins(debt))
 	if err != nil {
 		return 0, err
@@ -74,7 +78,7 @@ func (k Keeper) StartDebtAuction(ctx sdk.Context, buyer string, bid sdk.Coin, in
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", auction.GetID())),
 			sdk.NewAttribute(types.AttributeKeyAuctionType, auction.GetType()),
 			sdk.NewAttribute(types.AttributeKeyBidDenom, auction.Bid.Denom),
-			sdk.NewAttribute(types.AttributeKeyLotDenom, auction.Lot.Denom),
+			sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()),
 		),
 	)
 	return auctionID, nil
@@ -98,6 +102,7 @@ func (k Keeper) StartCollateralAuction(
 		debt,
 	)
 
+	// NOTE: for the duration of the auction the auction module account holds the debt and the lot
 	err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, seller, types.ModuleName, sdk.NewCoins(lot))
 	if err != nil {
 		return 0, err
@@ -118,7 +123,8 @@ func (k Keeper) StartCollateralAuction(
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", auction.GetID())),
 			sdk.NewAttribute(types.AttributeKeyAuctionType, auction.GetType()),
 			sdk.NewAttribute(types.AttributeKeyBidDenom, auction.Bid.Denom),
-			sdk.NewAttribute(types.AttributeKeyLotDenom, auction.Lot.Denom),
+			sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()),
+			sdk.NewAttribute(types.AttributeKeyMaxBidAmount, auction.MaxBid.String()),
 		),
 	)
 	return auctionID, nil
@@ -218,7 +224,7 @@ func (k Keeper) PlaceBidSurplus(ctx sdk.Context, a types.SurplusAuction, bidder
 			types.EventTypeAuctionBid,
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", a.ID)),
 			sdk.NewAttribute(types.AttributeKeyBidder, a.Bidder.String()),
-			sdk.NewAttribute(types.AttributeKeyBidAmount, a.Bid.Amount.String()),
+			sdk.NewAttribute(types.AttributeKeyBid, a.Bid.String()),
 			sdk.NewAttribute(types.AttributeKeyEndTime, fmt.Sprintf("%d", a.EndTime.Unix())),
 		),
 	)
@@ -233,6 +239,7 @@ func (k Keeper) PlaceForwardBidCollateral(ctx sdk.Context, a types.CollateralAuc
 		return a, sdkerrors.Wrapf(types.ErrInvalidBidDenom, "%s ≠ %s", bid.Denom, a.Bid.Denom)
 	}
 	if a.IsReversePhase() {
+		// TODO: panic maybe?
 		return a, sdkerrors.Wrapf(types.ErrCollateralAuctionIsInReversePhase, "%d", a.ID)
 	}
 	minNewBidAmt := a.Bid.Amount.Add( // new bids must be some % greater than old bid, and at least 1 larger to avoid replacing an old bid at no cost
@@ -294,7 +301,7 @@ func (k Keeper) PlaceForwardBidCollateral(ctx sdk.Context, a types.CollateralAuc
 			types.EventTypeAuctionBid,
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", a.ID)),
 			sdk.NewAttribute(types.AttributeKeyBidder, a.Bidder.String()),
-			sdk.NewAttribute(types.AttributeKeyBidAmount, a.Bid.Amount.String()),
+			sdk.NewAttribute(types.AttributeKeyBid, a.Bid.String()),
 			sdk.NewAttribute(types.AttributeKeyEndTime, fmt.Sprintf("%d", a.EndTime.Unix())),
 		),
 	)
@@ -309,6 +316,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc
 		return a, sdkerrors.Wrapf(types.ErrInvalidLotDenom, lot.Denom, a.Lot.Denom)
 	}
 	if !a.IsReversePhase() {
+		// TODO: Panic here?
 		return a, sdkerrors.Wrapf(types.ErrCollateralAuctionIsInForwardPhase, "%d", a.ID)
 	}
 	maxNewLotAmt := a.Lot.Amount.Sub( // new lot must be some % less than old lot, and at least 1 smaller to avoid replacing an old bid at no cost
@@ -336,18 +344,22 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc
 			return a, err
 		}
 	}
+
 	// Decrease in lot is sent to weighted addresses (normally the CDP depositors)
-	// TODO paying out rateably to cdp depositors is vulnerable to errors compounding over multiple bids - check this can't be gamed.
+	// TODO: paying out rateably to cdp depositors is vulnerable to errors compounding over multiple bids - check this can't be gamed.
 	lotPayouts, err := splitCoinIntoWeightedBuckets(a.Lot.Sub(lot), a.LotReturns.Weights)
 	if err != nil {
 		return a, err
 	}
 	for i, payout := range lotPayouts {
+		// if due to rounding, for whatever reason, the lot amount is 0, don't execute the following code
+		if payout.IsPositive() {
 			err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout))
 			if err != nil {
 				return a, err
 			}
 		}
+	}
 
 	// Update Auction
 	a.Bidder = bidder
@@ -363,7 +375,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc
 			types.EventTypeAuctionBid,
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", a.ID)),
 			sdk.NewAttribute(types.AttributeKeyBidder, a.Bidder.String()),
-			sdk.NewAttribute(types.AttributeKeyLotAmount, a.Lot.Amount.String()),
+			sdk.NewAttribute(types.AttributeKeyLot, a.Lot.String()),
 			sdk.NewAttribute(types.AttributeKeyEndTime, fmt.Sprintf("%d", a.EndTime.Unix())),
 		),
 	)
@@ -429,7 +441,7 @@ func (k Keeper) PlaceBidDebt(ctx sdk.Context, a types.DebtAuction, bidder sdk.Ac
 			types.EventTypeAuctionBid,
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", a.ID)),
 			sdk.NewAttribute(types.AttributeKeyBidder, a.Bidder.String()),
-			sdk.NewAttribute(types.AttributeKeyLotAmount, a.Lot.Amount.String()),
+			sdk.NewAttribute(types.AttributeKeyLot, a.Lot.String()),
 			sdk.NewAttribute(types.AttributeKeyEndTime, fmt.Sprintf("%d", a.EndTime.Unix())),
 		),
 	)
@@ -442,10 +454,13 @@ func (k Keeper) CloseAuction(ctx sdk.Context, auctionID uint64) error {
 
 	auction, found := k.GetAuction(ctx, auctionID)
 	if !found {
+		// TODO: panic if we are closing an auction in BB that isn't in the store?
+		// how else would we get here?
 		return sdkerrors.Wrapf(types.ErrAuctionNotFound, "%d", auctionID)
 	}
 
 	if ctx.BlockTime().Before(auction.GetEndTime()) {
+		// TODO: Do we check this upstream? should this be a panic
 		return sdkerrors.Wrapf(types.ErrAuctionHasNotExpired, "block time %s, auction end time %s", ctx.BlockTime().UTC(), auction.GetEndTime().UTC())
 	}
 
@@ -473,6 +488,7 @@ func (k Keeper) CloseAuction(ctx sdk.Context, auctionID uint64) error {
 		sdk.NewEvent(
 			types.EventTypeAuctionClose,
 			sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", auction.GetID())),
+			// TODO: add closed height here to facilitate the query of the auction after it closes
 		),
 	)
 	return nil
@@ -480,17 +496,23 @@ func (k Keeper) CloseAuction(ctx sdk.Context, auctionID uint64) error {
 
 // PayoutDebtAuction pays out the proceeds for a debt auction, first minting the coins.
 func (k Keeper) PayoutDebtAuction(ctx sdk.Context, a types.DebtAuction) error {
+	// create the coins that are needed to pay off the debt
 	err := k.supplyKeeper.MintCoins(ctx, a.Initiator, sdk.NewCoins(a.Lot))
 	if err != nil {
+		// TODO: how would we get here? should this be a panic?
 		return err
 	}
+	// send the new coins from the initiator module to the bidder
 	err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, a.Initiator, a.Bidder, sdk.NewCoins(a.Lot))
 	if err != nil {
+		// TODO: how would we get here? should this be a panic?
 		return err
 	}
+	// if there is remaining debt, return it to the calling module to manage
 	if a.CorrespondingDebt.IsPositive() {
 		err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(a.CorrespondingDebt))
 		if err != nil {
+			// TODO: how would we get here? should this be a panic?
 			return err
 		}
 	}
@@ -499,8 +521,10 @@ func (k Keeper) PayoutDebtAuction(ctx sdk.Context, a types.DebtAuction) error {
 
 // PayoutSurplusAuction pays out the proceeds for a surplus auction.
 func (k Keeper) PayoutSurplusAuction(ctx sdk.Context, a types.SurplusAuction) error {
+	// Send the tokens from the auction module account where they are being managed to the bidder who won the auction
 	err := k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Lot))
 	if err != nil {
+		// TODO: how would we get here? should this be a panic?
 		return err
 	}
 	return nil
@@ -508,13 +532,18 @@ func (k Keeper) PayoutSurplusAuction(ctx sdk.Context, a types.SurplusAuction) er
 
 // PayoutCollateralAuction pays out the proceeds for a collateral auction.
 func (k Keeper) PayoutCollateralAuction(ctx sdk.Context, a types.CollateralAuction) error {
+	// Send the tokens from the auction module account where they are being managed to the bidder who won the auction
 	err := k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.Bidder, sdk.NewCoins(a.Lot))
 	if err != nil {
+		// TODO: how would we get here? should this be a panic?
 		return err
 	}
+
+	// if there is remaining debt after the auction, send it back to the initiating module for management
 	if a.CorrespondingDebt.IsPositive() {
 		err = k.supplyKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, a.Initiator, sdk.NewCoins(a.CorrespondingDebt))
 		if err != nil {
+			// TODO: how would we get here? should this be a panic?
 			return err
 		}
 	}
@@ -549,6 +578,7 @@ func earliestTime(t1, t2 time.Time) time.Time {
 func splitCoinIntoWeightedBuckets(coin sdk.Coin, buckets []sdk.Int) ([]sdk.Coin, error) {
 	for _, bucket := range buckets {
 		if bucket.IsNegative() {
+			// TODO: Panic here? How would the code get here (should catch at validation)
 			return nil, fmt.Errorf("cannot split %s into bucket with negative weight (%s)", coin.String(), bucket.String())
 		}
 	}

x/auction/keeper/keeper.go

@@ -74,6 +74,7 @@ func (k Keeper) IncrementNextAuctionID(ctx sdk.Context) error {
 func (k Keeper) StoreNewAuction(ctx sdk.Context, auction types.Auction) (uint64, error) {
 	newAuctionID, err := k.GetNextAuctionID(ctx)
 	if err != nil {
+		// TODO: We might want to panic on this condition
 		return 0, err
 	}
 	auction = auction.WithID(newAuctionID)

x/auction/keeper/math.go

@@ -11,7 +11,8 @@ import (
 // https://en.wikipedia.org/wiki/Largest_remainder_method
 // see also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100
 func splitIntIntoWeightedBuckets(amount sdk.Int, buckets []sdk.Int) []sdk.Int {
-	// TODO ideally change algorithm to work with -ve numbers. Limiting to +ve numbers until them
+	// TODO: ideally change algorithm to work with -ve numbers. Limiting to +ve numbers until them
+	// TODO: negative buckets don't make sense in this context
 	if amount.IsNegative() {
 		panic("negative amount")
 	}

x/auction/types/events.go

@@ -12,7 +12,10 @@ const (
 	AttributeKeyBidder       = "bidder"
 	AttributeKeyBidDenom     = "bid_denom"
 	AttributeKeyLotDenom     = "lot_denom"
+	AttributeKeyLot          = "lot"
+	AttributeKeyMaxBidAmount = "max_bid"
 	AttributeKeyBidAmount    = "bid_amount"
+	AttributeKeyBid          = "bid"
 	AttributeKeyLotAmount    = "lot_amount"
 	AttributeKeyEndTime      = "end_time"
 )

x/cdp/abci.go

@@ -25,6 +25,7 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k Keeper) {
 		err := k.UpdateFeesForAllCdps(ctx, cp.Denom)
 
 		// handle if an error is returned then propagate up
+		// TODO: panic here probably for mainnet (optional: have a bool that says panic or no)
 		if err != nil {
 			ctx.EventManager().EmitEvent(
 				sdk.NewEvent(

x/cdp/keeper/seize.go

@@ -13,8 +13,8 @@ import (
 // 1. updates the fees for the input cdp,
 // 2. sends collateral for all deposits from the cdp module to the liquidator module account
 // 3. Applies the liquidation penalty and mints the corresponding amount of debt coins in the cdp module
-// 3. moves debt coins from the cdp module to the liquidator module account,
-// 4. decrements the total amount of principal outstanding for that collateral type
+// 4. moves debt coins from the cdp module to the liquidator module account,
+// 5. decrements the total amount of principal outstanding for that collateral type
 // (this is the equivalent of saying that fees are no longer accumulated by a cdp once it gets liquidated)
 func (k Keeper) SeizeCollateral(ctx sdk.Context, cdp types.CDP) error {
 	// Calculate the previous collateral ratio