From f86d0f3c3bc5495714a18cdaf9f87a0b46d67db3 Mon Sep 17 00:00:00 2001 From: Jack Zampolin Date: Mon, 4 May 2020 10:52:36 -0700 Subject: [PATCH 01/11] Add changes from code review --- x/auction/keeper/auctions.go | 54 ++++++++++++++++++++++++++++-------- x/auction/keeper/keeper.go | 1 + x/auction/keeper/math.go | 3 +- x/auction/types/events.go | 21 ++++++++------ x/cdp/abci.go | 1 + x/cdp/keeper/seize.go | 4 +-- 6 files changed, 60 insertions(+), 24 deletions(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index c6152652..bfa06006 100644 --- a/x/auction/keeper/auctions.go +++ b/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,16 +344,20 @@ 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 { - err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout)) - if err != nil { - return a, err + // 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 + } } } @@ -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()) } } diff --git a/x/auction/keeper/keeper.go b/x/auction/keeper/keeper.go index 7250db4e..d9756357 100644 --- a/x/auction/keeper/keeper.go +++ b/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) diff --git a/x/auction/keeper/math.go b/x/auction/keeper/math.go index 437b4f22..7b7d8468 100644 --- a/x/auction/keeper/math.go +++ b/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") } diff --git a/x/auction/types/events.go b/x/auction/types/events.go index 9d85743a..366ed5e1 100644 --- a/x/auction/types/events.go +++ b/x/auction/types/events.go @@ -6,13 +6,16 @@ const ( EventTypeAuctionBid = "auction_bid" EventTypeAuctionClose = "auction_close" - AttributeValueCategory = ModuleName - AttributeKeyAuctionID = "auction_id" - AttributeKeyAuctionType = "auction_type" - AttributeKeyBidder = "bidder" - AttributeKeyBidDenom = "bid_denom" - AttributeKeyLotDenom = "lot_denom" - AttributeKeyBidAmount = "bid_amount" - AttributeKeyLotAmount = "lot_amount" - AttributeKeyEndTime = "end_time" + AttributeValueCategory = ModuleName + AttributeKeyAuctionID = "auction_id" + AttributeKeyAuctionType = "auction_type" + AttributeKeyBidder = "bidder" + AttributeKeyBidDenom = "bid_denom" + AttributeKeyLotDenom = "lot_denom" + AttributeKeyLot = "lot" + AttributeKeyMaxBidAmount = "max_bid" + AttributeKeyBidAmount = "bid_amount" + AttributeKeyBid = "bid" + AttributeKeyLotAmount = "lot_amount" + AttributeKeyEndTime = "end_time" ) diff --git a/x/cdp/abci.go b/x/cdp/abci.go index 92781fa0..1207c8bc 100644 --- a/x/cdp/abci.go +++ b/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( diff --git a/x/cdp/keeper/seize.go b/x/cdp/keeper/seize.go index 67075bc7..b10c25ce 100644 --- a/x/cdp/keeper/seize.go +++ b/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 From 4039086e8d2354600576599ef2b936d19f211baa Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Fri, 8 May 2020 16:07:11 +0100 Subject: [PATCH 02/11] tidy up events --- x/auction/keeper/auctions.go | 12 ++++++------ x/auction/keeper/math.go | 3 +-- x/auction/spec/04_events.md | 18 ++++++++++-------- x/auction/types/events.go | 23 ++++++++++------------- 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index bfa06006..9b7ac4ba 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -36,7 +36,7 @@ func (k Keeper) StartSurplusAuction(ctx sdk.Context, seller string, lot sdk.Coin types.EventTypeAuctionStart, 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.AttributeKeyBid, auction.Bid.String()), sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()), ), ) @@ -77,7 +77,7 @@ func (k Keeper) StartDebtAuction(ctx sdk.Context, buyer string, bid sdk.Coin, in types.EventTypeAuctionStart, 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.AttributeKeyBid, auction.Bid.String()), sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()), ), ) @@ -122,9 +122,9 @@ func (k Keeper) StartCollateralAuction( types.EventTypeAuctionStart, 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.AttributeKeyBid, auction.Bid.String()), sdk.NewAttribute(types.AttributeKeyLot, auction.Lot.String()), - sdk.NewAttribute(types.AttributeKeyMaxBidAmount, auction.MaxBid.String()), + sdk.NewAttribute(types.AttributeKeyMaxBid, auction.MaxBid.String()), ), ) return auctionID, nil @@ -352,7 +352,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc 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 the payout amount is 0, don't send 0 coins if payout.IsPositive() { err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout)) if err != nil { @@ -488,7 +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 + sdk.NewAttribute(types.AttributeKeyCloseBlock, fmt.Sprintf("%d", ctx.BlockHeight())) ), ) return nil diff --git a/x/auction/keeper/math.go b/x/auction/keeper/math.go index 7b7d8468..5759471d 100644 --- a/x/auction/keeper/math.go +++ b/x/auction/keeper/math.go @@ -11,8 +11,7 @@ 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: negative buckets don't make sense in this context + // Limit input to +ve numbers as algorithm hasn't been designed to work with -ve numbers. if amount.IsNegative() { panic("negative amount") } diff --git a/x/auction/spec/04_events.md b/x/auction/spec/04_events.md index 36232b2f..4da22b31 100644 --- a/x/auction/spec/04_events.md +++ b/x/auction/spec/04_events.md @@ -4,12 +4,13 @@ The `x/auction` module emits the following events: ## Triggered By Other Modules -| Type | Attribute Key | Attribute Value | -|---------------|---------------|---------------------| -| auction_start | auction_id | {auction ID} | -| auction_start | auction_type | {auction type} | -| auction_start | lot_denom | {auction lot denom} | -| auction_start | bid_denom | {auction bid denom} | +| Type | Attribute Key | Attribute Value | +|---------------|---------------|-----------------| +| auction_start | auction_id | {auction ID} | +| auction_start | auction_type | {auction type} | +| auction_start | lot | {coin amount} | +| auction_start | bid | {coin amount} | +| auction_start | max_bid | {coin amount} | ## Handlers @@ -19,8 +20,8 @@ The `x/auction` module emits the following events: |-------------|---------------|--------------------| | auction_bid | auction_id | {auction ID} | | auction_bid | bidder | {latest bidder} | -| auction_bid | bid_amount | {coin amount} | -| auction_bid | lot_amount | {coin amount} | +| auction_bid | bid | {coin amount} | +| auction_bid | lot | {coin amount} | | auction_bid | end_time | {auction end time} | | message | module | auction | | message | sender | {sender address} | @@ -30,3 +31,4 @@ The `x/auction` module emits the following events: | Type | Attribute Key | Attribute Value | |---------------|---------------|-----------------| | auction_close | auction_id | {auction ID} | +| auction_close | close_block | {block height} | diff --git a/x/auction/types/events.go b/x/auction/types/events.go index 366ed5e1..b9389945 100644 --- a/x/auction/types/events.go +++ b/x/auction/types/events.go @@ -1,21 +1,18 @@ package types -// Events for auction module +// Events for the module const ( EventTypeAuctionStart = "auction_start" EventTypeAuctionBid = "auction_bid" EventTypeAuctionClose = "auction_close" - AttributeValueCategory = ModuleName - AttributeKeyAuctionID = "auction_id" - AttributeKeyAuctionType = "auction_type" - AttributeKeyBidder = "bidder" - AttributeKeyBidDenom = "bid_denom" - AttributeKeyLotDenom = "lot_denom" - AttributeKeyLot = "lot" - AttributeKeyMaxBidAmount = "max_bid" - AttributeKeyBidAmount = "bid_amount" - AttributeKeyBid = "bid" - AttributeKeyLotAmount = "lot_amount" - AttributeKeyEndTime = "end_time" + AttributeValueCategory = ModuleName + AttributeKeyAuctionID = "auction_id" + AttributeKeyAuctionType = "auction_type" + AttributeKeyBidder = "bidder" + AttributeKeyLot = "lot" + AttributeKeyMaxBid = "max_bid" + AttributeKeyBid = "bid" + AttributeKeyEndTime = "end_time" + AttributeKeyCloseBlock = "close_block" ) From 976f8f632d6faac3fc1f4d9b4c998d954b3dbb4e Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Fri, 8 May 2020 16:10:59 +0100 Subject: [PATCH 03/11] remove todo - leave to caller to deal with panic --- x/auction/keeper/keeper.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/auction/keeper/keeper.go b/x/auction/keeper/keeper.go index d9756357..6dc09704 100644 --- a/x/auction/keeper/keeper.go +++ b/x/auction/keeper/keeper.go @@ -74,7 +74,6 @@ 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) @@ -129,7 +128,7 @@ func (k Keeper) DeleteAuction(ctx sdk.Context, auctionID uint64) { } // InsertIntoByTimeIndex adds an auction ID and end time into the byTime index. -func (k Keeper) InsertIntoByTimeIndex(ctx sdk.Context, endTime time.Time, auctionID uint64) { // TODO make private, and find way to make tests work +func (k Keeper) InsertIntoByTimeIndex(ctx sdk.Context, endTime time.Time, auctionID uint64) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AuctionByTimeKeyPrefix) store.Set(types.GetAuctionByTimeKey(endTime, auctionID), types.Uint64ToBytes(auctionID)) } From 8899a7ff04c88f4d442845c195002de57fb93107 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Mon, 11 May 2020 14:55:32 +0100 Subject: [PATCH 04/11] replace some errors with panics --- x/auction/alias.go | 142 +++++++++++++++++------------------ x/auction/keeper/auctions.go | 28 ++----- x/auction/types/errors.go | 26 +++---- x/auction/types/genesis.go | 2 +- 4 files changed, 85 insertions(+), 113 deletions(-) diff --git a/x/auction/alias.go b/x/auction/alias.go index 1cbf3593..f0ae986b 100644 --- a/x/auction/alias.go +++ b/x/auction/alias.go @@ -1,109 +1,105 @@ package auction +// DO NOT EDIT - generated by aliasgen tool (github.com/rhuairahrighairidh/aliasgen) + import ( "github.com/kava-labs/kava/x/auction/keeper" "github.com/kava-labs/kava/x/auction/types" ) -// nolint -// autogenerated code using github.com/rigelrozanski/multitool -// aliases generated for the following subdirectories: -// ALIASGEN: github.com/kava-labs/kava/x/auction/keeper -// ALIASGEN: github.com/kava-labs/kava/x/auction/types - const ( - EventTypeAuctionStart = types.EventTypeAuctionStart - EventTypeAuctionBid = types.EventTypeAuctionBid - EventTypeAuctionClose = types.EventTypeAuctionClose - AttributeValueCategory = types.AttributeValueCategory AttributeKeyAuctionID = types.AttributeKeyAuctionID AttributeKeyAuctionType = types.AttributeKeyAuctionType + AttributeKeyBid = types.AttributeKeyBid AttributeKeyBidder = types.AttributeKeyBidder - AttributeKeyBidDenom = types.AttributeKeyBidDenom - AttributeKeyLotDenom = types.AttributeKeyLotDenom - AttributeKeyBidAmount = types.AttributeKeyBidAmount - AttributeKeyLotAmount = types.AttributeKeyLotAmount + AttributeKeyCloseBlock = types.AttributeKeyCloseBlock AttributeKeyEndTime = types.AttributeKeyEndTime - DefaultNextAuctionID = types.DefaultNextAuctionID - ModuleName = types.ModuleName - StoreKey = types.StoreKey - RouterKey = types.RouterKey - DefaultParamspace = types.DefaultParamspace - QuerierRoute = types.QuerierRoute - DefaultMaxAuctionDuration = types.DefaultMaxAuctionDuration + AttributeKeyLot = types.AttributeKeyLot + AttributeKeyMaxBid = types.AttributeKeyMaxBid + AttributeValueCategory = types.AttributeValueCategory DefaultBidDuration = types.DefaultBidDuration + DefaultMaxAuctionDuration = types.DefaultMaxAuctionDuration + DefaultNextAuctionID = types.DefaultNextAuctionID + DefaultParamspace = types.DefaultParamspace + EventTypeAuctionBid = types.EventTypeAuctionBid + EventTypeAuctionClose = types.EventTypeAuctionClose + EventTypeAuctionStart = types.EventTypeAuctionStart + ModuleName = types.ModuleName + QuerierRoute = types.QuerierRoute QueryGetAuction = types.QueryGetAuction QueryGetAuctions = types.QueryGetAuctions QueryGetParams = types.QueryGetParams + RouterKey = types.RouterKey + StoreKey = types.StoreKey ) var ( - // functions aliases - NewKeeper = keeper.NewKeeper - NewQuerier = keeper.NewQuerier - RegisterInvariants = keeper.RegisterInvariants - NewSurplusAuction = types.NewSurplusAuction - NewDebtAuction = types.NewDebtAuction - NewCollateralAuction = types.NewCollateralAuction - NewWeightedAddresses = types.NewWeightedAddresses - RegisterCodec = types.RegisterCodec - ErrInvalidInitialAuctionID = types.ErrInvalidInitialAuctionID - ErrInvalidModulePermissions = types.ErrInvalidModulePermissions - ErrUnrecognizedAuctionType = types.ErrUnrecognizedAuctionType - ErrAuctionNotFound = types.ErrAuctionNotFound - ErrAuctionHasNotExpired = types.ErrAuctionHasNotExpired - ErrAuctionHasExpired = types.ErrAuctionHasExpired - ErrInvalidBidDenom = types.ErrInvalidBidDenom - ErrInvalidLotDenom = types.ErrInvalidLotDenom - ErrBidTooSmall = types.ErrBidTooSmall - ErrBidTooLarge = types.ErrBidTooLarge - ErrLotTooSmall = types.ErrLotTooSmall - ErrLotTooLarge = types.ErrLotTooLarge - ErrCollateralAuctionIsInReversePhase = types.ErrCollateralAuctionIsInReversePhase - ErrCollateralAuctionIsInForwardPhase = types.ErrCollateralAuctionIsInForwardPhase - NewGenesisState = types.NewGenesisState - DefaultGenesisState = types.DefaultGenesisState - GetAuctionKey = types.GetAuctionKey - GetAuctionByTimeKey = types.GetAuctionByTimeKey - Uint64ToBytes = types.Uint64ToBytes - Uint64FromBytes = types.Uint64FromBytes - NewMsgPlaceBid = types.NewMsgPlaceBid - NewParams = types.NewParams - DefaultParams = types.DefaultParams - ParamKeyTable = types.ParamKeyTable - NewQueryAllAuctionParams = types.NewQueryAllAuctionParams - NewAuctionWithPhase = types.NewAuctionWithPhase + // function aliases + ModuleAccountInvariants = keeper.ModuleAccountInvariants + NewKeeper = keeper.NewKeeper + NewQuerier = keeper.NewQuerier + RegisterInvariants = keeper.RegisterInvariants + ValidAuctionInvariant = keeper.ValidAuctionInvariant + ValidIndexInvariant = keeper.ValidIndexInvariant + DefaultGenesisState = types.DefaultGenesisState + DefaultParams = types.DefaultParams + GetAuctionByTimeKey = types.GetAuctionByTimeKey + GetAuctionKey = types.GetAuctionKey + NewAuctionWithPhase = types.NewAuctionWithPhase + NewCollateralAuction = types.NewCollateralAuction + NewDebtAuction = types.NewDebtAuction + NewGenesisState = types.NewGenesisState + NewMsgPlaceBid = types.NewMsgPlaceBid + NewParams = types.NewParams + NewQueryAllAuctionParams = types.NewQueryAllAuctionParams + NewSurplusAuction = types.NewSurplusAuction + NewWeightedAddresses = types.NewWeightedAddresses + ParamKeyTable = types.ParamKeyTable + RegisterCodec = types.RegisterCodec + Uint64FromBytes = types.Uint64FromBytes + Uint64ToBytes = types.Uint64ToBytes // variable aliases - DistantFuture = types.DistantFuture - ModuleCdc = types.ModuleCdc - AuctionKeyPrefix = types.AuctionKeyPrefix - AuctionByTimeKeyPrefix = types.AuctionByTimeKeyPrefix - NextAuctionIDKey = types.NextAuctionIDKey - DefaultIncrement = types.DefaultIncrement - KeyBidDuration = types.KeyBidDuration - KeyMaxAuctionDuration = types.KeyMaxAuctionDuration - KeyIncrementSurplus = types.KeyIncrementSurplus - KeyIncrementDebt = types.KeyIncrementDebt - KeyIncrementCollateral = types.KeyIncrementCollateral + AuctionByTimeKeyPrefix = types.AuctionByTimeKeyPrefix + AuctionKeyPrefix = types.AuctionKeyPrefix + DefaultIncrement = types.DefaultIncrement + DistantFuture = types.DistantFuture + ErrAuctionHasExpired = types.ErrAuctionHasExpired + ErrAuctionHasNotExpired = types.ErrAuctionHasNotExpired + ErrAuctionNotFound = types.ErrAuctionNotFound + ErrBidTooLarge = types.ErrBidTooLarge + ErrBidTooSmall = types.ErrBidTooSmall + ErrInvalidBidDenom = types.ErrInvalidBidDenom + ErrInvalidInitialAuctionID = types.ErrInvalidInitialAuctionID + ErrInvalidLotDenom = types.ErrInvalidLotDenom + ErrLotTooLarge = types.ErrLotTooLarge + ErrLotTooSmall = types.ErrLotTooSmall + ErrUnrecognizedAuctionType = types.ErrUnrecognizedAuctionType + KeyBidDuration = types.KeyBidDuration + KeyIncrementCollateral = types.KeyIncrementCollateral + KeyIncrementDebt = types.KeyIncrementDebt + KeyIncrementSurplus = types.KeyIncrementSurplus + KeyMaxAuctionDuration = types.KeyMaxAuctionDuration + ModuleCdc = types.ModuleCdc + NextAuctionIDKey = types.NextAuctionIDKey ) type ( Keeper = keeper.Keeper Auction = types.Auction + AuctionWithPhase = types.AuctionWithPhase Auctions = types.Auctions BaseAuction = types.BaseAuction - SurplusAuction = types.SurplusAuction - DebtAuction = types.DebtAuction CollateralAuction = types.CollateralAuction - WeightedAddresses = types.WeightedAddresses - SupplyKeeper = types.SupplyKeeper + DebtAuction = types.DebtAuction GenesisAuction = types.GenesisAuction GenesisAuctions = types.GenesisAuctions GenesisState = types.GenesisState MsgPlaceBid = types.MsgPlaceBid Params = types.Params - QueryAuctionParams = types.QueryAuctionParams QueryAllAuctionParams = types.QueryAllAuctionParams - AuctionWithPhase = types.AuctionWithPhase + QueryAuctionParams = types.QueryAuctionParams + SupplyKeeper = types.SupplyKeeper + SurplusAuction = types.SurplusAuction + WeightedAddresses = types.WeightedAddresses ) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 9b7ac4ba..9deca689 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -57,8 +57,7 @@ func (k Keeper) StartDebtAuction(ctx sdk.Context, buyer string, bid sdk.Coin, in // 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) + panic(fmt.Errorf("module '%s' does not have '%s' permission", buyer, supply.Minter)) } // NOTE: for the duration of the auction the auction module account holds the debt @@ -239,8 +238,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) + panic("cannot place forward bid on auction in reverse phase") } 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 sdk.MaxInt( @@ -316,8 +314,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) + panic("cannot place reverse bid on auction in forward phase") } 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 sdk.MaxInt( @@ -454,13 +451,10 @@ 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()) } @@ -488,7 +482,7 @@ func (k Keeper) CloseAuction(ctx sdk.Context, auctionID uint64) error { sdk.NewEvent( types.EventTypeAuctionClose, sdk.NewAttribute(types.AttributeKeyAuctionID, fmt.Sprintf("%d", auction.GetID())), - sdk.NewAttribute(types.AttributeKeyCloseBlock, fmt.Sprintf("%d", ctx.BlockHeight())) + sdk.NewAttribute(types.AttributeKeyCloseBlock, fmt.Sprintf("%d", ctx.BlockHeight())), ), ) return nil @@ -499,20 +493,17 @@ 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 + panic(fmt.Errorf("could not mint coins: %w", 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 } } @@ -524,7 +515,6 @@ func (k Keeper) PayoutSurplusAuction(ctx sdk.Context, a types.SurplusAuction) er // 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 @@ -535,7 +525,6 @@ func (k Keeper) PayoutCollateralAuction(ctx sdk.Context, a types.CollateralAucti // 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 } @@ -543,7 +532,6 @@ func (k Keeper) PayoutCollateralAuction(ctx sdk.Context, a types.CollateralAucti 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 } } @@ -576,12 +564,6 @@ func earliestTime(t1, t2 time.Time) time.Time { // splitCoinIntoWeightedBuckets divides up some amount of coins according to some weights. 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()) - } - } amounts := splitIntIntoWeightedBuckets(coin.Amount, buckets) result := make([]sdk.Coin, len(amounts)) for i, a := range amounts { diff --git a/x/auction/types/errors.go b/x/auction/types/errors.go index 6aa78673..30b696fd 100644 --- a/x/auction/types/errors.go +++ b/x/auction/types/errors.go @@ -7,30 +7,24 @@ import sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" var ( // ErrInvalidInitialAuctionID error for when the initial auction ID hasn't been set ErrInvalidInitialAuctionID = sdkerrors.Register(ModuleName, 2, "initial auction ID hasn't been set") - // ErrInvalidModulePermissions error for when module doesn't have valid permissions - ErrInvalidModulePermissions = sdkerrors.Register(ModuleName, 3, "module does not have required permission") // ErrUnrecognizedAuctionType error for unrecognized auction type - ErrUnrecognizedAuctionType = sdkerrors.Register(ModuleName, 4, "unrecognized auction type") + ErrUnrecognizedAuctionType = sdkerrors.Register(ModuleName, 3, "unrecognized auction type") // ErrAuctionNotFound error for when an auction is not found - ErrAuctionNotFound = sdkerrors.Register(ModuleName, 5, "auction not found") + ErrAuctionNotFound = sdkerrors.Register(ModuleName, 4, "auction not found") // ErrAuctionHasNotExpired error for attempting to close an auction that has not passed its end time - ErrAuctionHasNotExpired = sdkerrors.Register(ModuleName, 6, "auction can't be closed as curent block time has not passed auction end time") + ErrAuctionHasNotExpired = sdkerrors.Register(ModuleName, 5, "auction can't be closed as curent block time has not passed auction end time") // ErrAuctionHasExpired error for when an auction is closed and unavailable for bidding - ErrAuctionHasExpired = sdkerrors.Register(ModuleName, 7, "auction has closed") + ErrAuctionHasExpired = sdkerrors.Register(ModuleName, 6, "auction has closed") // ErrInvalidBidDenom error for when bid denom doesn't match auction bid denom - ErrInvalidBidDenom = sdkerrors.Register(ModuleName, 8, "bid denom doesn't match auction bid denom") + ErrInvalidBidDenom = sdkerrors.Register(ModuleName, 7, "bid denom doesn't match auction bid denom") // ErrInvalidLotDenom error for when lot denom doesn't match auction lot denom - ErrInvalidLotDenom = sdkerrors.Register(ModuleName, 9, "lot denom doesn't match auction lot denom") + ErrInvalidLotDenom = sdkerrors.Register(ModuleName, 8, "lot denom doesn't match auction lot denom") // ErrBidTooSmall error for when bid is not greater than auction's min bid amount - ErrBidTooSmall = sdkerrors.Register(ModuleName, 10, "bid is not greater than auction's min new bid amount") + ErrBidTooSmall = sdkerrors.Register(ModuleName, 9, "bid is not greater than auction's min new bid amount") // ErrBidTooLarge error for when bid is larger than auction's maximum allowed bid - ErrBidTooLarge = sdkerrors.Register(ModuleName, 11, "bid is greater than auction's max bid") + ErrBidTooLarge = sdkerrors.Register(ModuleName, 10, "bid is greater than auction's max bid") // ErrLotTooSmall error for when lot is less than zero - ErrLotTooSmall = sdkerrors.Register(ModuleName, 12, "lot is not greater than auction's min new lot amount") + ErrLotTooSmall = sdkerrors.Register(ModuleName, 11, "lot is not greater than auction's min new lot amount") // ErrLotTooLarge error for when lot is not smaller than auction's max new lot amount - ErrLotTooLarge = sdkerrors.Register(ModuleName, 13, "lot is greater than auction's max new lot amount") - // ErrCollateralAuctionIsInReversePhase error for when attempting to place a forward bid on a collateral auction in reverse phase - ErrCollateralAuctionIsInReversePhase = sdkerrors.Register(ModuleName, 14, "invalid bid: auction is in reverse phase") - // ErrCollateralAuctionIsInForwardPhase error for when attempting to place a reverse bid on a collateral auction in forward phase - ErrCollateralAuctionIsInForwardPhase = sdkerrors.Register(ModuleName, 15, "invalid bid: auction is in forward phase") + ErrLotTooLarge = sdkerrors.Register(ModuleName, 12, "lot is greater than auction's max new lot amount") ) diff --git a/x/auction/types/genesis.go b/x/auction/types/genesis.go index e63df0d3..9af28045 100644 --- a/x/auction/types/genesis.go +++ b/x/auction/types/genesis.go @@ -76,7 +76,7 @@ func (gs GenesisState) Validate() error { ids[a.GetID()] = true if a.GetID() >= gs.NextAuctionID { - return fmt.Errorf("found auction ID >= the nextAuctionID (%d >= %d)", a.GetID(), gs.NextAuctionID) + return fmt.Errorf("found auction ID ≥ the nextAuctionID (%d ≥ %d)", a.GetID(), gs.NextAuctionID) } } return nil From 5987d966ef3ec91253a2bdd74f61f93e9d2e237a Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Mon, 11 May 2020 20:45:00 +0100 Subject: [PATCH 05/11] increase auction maths safety --- x/auction/genesis_test.go | 5 +- x/auction/keeper/auctions.go | 2 +- x/auction/keeper/math.go | 27 ++++++--- x/auction/keeper/math_test.go | 96 +++++++++++++++++++++++++++++--- x/auction/types/auctions.go | 10 +++- x/auction/types/auctions_test.go | 87 ++++++++++++++++------------- x/auction/types/msg_test.go | 2 - x/auction/types/params_test.go | 4 -- 8 files changed, 167 insertions(+), 66 deletions(-) diff --git a/x/auction/genesis_test.go b/x/auction/genesis_test.go index 78ecf1f9..28ba48e2 100644 --- a/x/auction/genesis_test.go +++ b/x/auction/genesis_test.go @@ -9,17 +9,20 @@ import ( abci "github.com/tendermint/tendermint/abci/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/kava-labs/kava/app" "github.com/kava-labs/kava/x/auction" ) +var _, testAddrs = app.GeneratePrivKeyAddressPairs(2) var testTime = time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC) var testAuction = auction.NewCollateralAuction( "seller", c("lotdenom", 10), testTime, c("biddenom", 1000), - auction.WeightedAddresses{}, + auction.WeightedAddresses{Addresses: testAddrs, Weights: []sdk.Int{sdk.OneInt(), sdk.OneInt()}}, c("debt", 1000), ).WithID(3).(auction.GenesisAuction) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 9deca689..8a66f939 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -343,7 +343,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc } // 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. + // Note: splitting an integer amount across weighted buckets results in small errors. lotPayouts, err := splitCoinIntoWeightedBuckets(a.Lot.Sub(lot), a.LotReturns.Weights) if err != nil { return a, err diff --git a/x/auction/keeper/math.go b/x/auction/keeper/math.go index 5759471d..2fd9198f 100644 --- a/x/auction/keeper/math.go +++ b/x/auction/keeper/math.go @@ -7,41 +7,52 @@ import ( ) // splitIntIntoWeightedBuckets divides an initial +ve integer among several buckets in proportion to the buckets' weights -// It uses the largest remainder method: -// https://en.wikipedia.org/wiki/Largest_remainder_method -// see also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100 +// It uses the largest remainder method: 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 { - // Limit input to +ve numbers as algorithm hasn't been designed to work with -ve numbers. + // Limit input to +ve numbers as algorithm hasn't been scoped to work with -ve numbers. if amount.IsNegative() { panic("negative amount") } + if len(buckets) < 1 { + panic("no buckets") + } for _, bucket := range buckets { if bucket.IsNegative() { panic("negative bucket") } } - totalWeights := totalInts(buckets...) + // 1) Split the amount by weights, recording whole number part and remainder + + totalWeights := totalInts(buckets...) + if !totalWeights.IsPositive() { + panic("total weights must sum to > 0") + } - // split amount by weights, recording whole number part and remainder quotients := make([]quoRem, len(buckets)) for i := range buckets { + // amount * ( weight/total_weight ) q := amount.Mul(buckets[i]).Quo(totalWeights) r := amount.Mul(buckets[i]).Mod(totalWeights) quotients[i] = quoRem{index: i, quo: q, rem: r} } - // apportion left over to buckets with the highest remainder (to minimize error) + // 2) Calculate total left over from remainders, and apportion it to buckets with the highest remainder (to minimize error) + + // sort by decreasing remainder order sort.Slice(quotients, func(i, j int) bool { - return quotients[i].rem.GT(quotients[j].rem) // decreasing remainder order + return quotients[i].rem.GT(quotients[j].rem) }) + // calculate total left over from remainders allocated := sdk.ZeroInt() for _, qr := range quotients { allocated = allocated.Add(qr.quo) } leftToAllocate := amount.Sub(allocated) + // apportion according to largest remainder results := make([]sdk.Int, len(quotients)) for _, qr := range quotients { results[qr.index] = qr.quo diff --git a/x/auction/keeper/math_test.go b/x/auction/keeper/math_test.go index 6ff02765..1412988c 100644 --- a/x/auction/keeper/math_test.go +++ b/x/auction/keeper/math_test.go @@ -10,19 +10,97 @@ import ( func TestSplitIntIntoWeightedBuckets(t *testing.T) { testCases := []struct { - name string - amount sdk.Int - buckets []sdk.Int - want []sdk.Int + name string + amount sdk.Int + buckets []sdk.Int + want []sdk.Int + expectPanic bool }{ - {"2split1,1", i(2), is(1, 1), is(1, 1)}, - {"100split1,9", i(100), is(1, 9), is(10, 90)}, - {"7split1,2", i(7), is(1, 2), is(2, 5)}, - {"17split1,1,1", i(17), is(1, 1, 1), is(6, 6, 5)}, + { + name: "0split0", + amount: i(0), + buckets: is(0), + expectPanic: true, + }, + { + name: "5splitnil", + amount: i(5), + buckets: is(), + expectPanic: true, + }, + { + name: "-2split1,1", + amount: i(-2), + buckets: is(1, 1), + expectPanic: true, + }, + { + name: "2split1,-1", + amount: i(2), + buckets: is(1, -1), + expectPanic: true, + }, + { + name: "0split0,0,0,1", + amount: i(0), + buckets: is(0, 0, 0, 1), + want: is(0, 0, 0, 0), + }, + { + name: "2split1,1", + amount: i(2), + buckets: is(1, 1), + want: is(1, 1), + }, + { + name: "100split1,9", + amount: i(100), + buckets: is(1, 9), + want: is(10, 90), + }, + { + name: "100split9,1", + amount: i(100), + buckets: is(9, 1), + want: is(90, 10), + }, + { + name: "7split1,2", + amount: i(7), + buckets: is(1, 2), + want: is(2, 5), + }, + { + name: "17split1,1,1", + amount: i(17), + buckets: is(1, 1, 1), + want: is(6, 6, 5), + }, + { + name: "10split1000000,1", + amount: i(10), + buckets: is(1000000, 1), + want: is(10, 0), + }, + { + name: "334733353split730777,31547", + amount: i(334733353), + buckets: is(730777, 31547), + want: is(320881194, 13852159), + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - got := splitIntIntoWeightedBuckets(tc.amount, tc.buckets) + var got []sdk.Int + run := func() { + got = splitIntIntoWeightedBuckets(tc.amount, tc.buckets) + } + if tc.expectPanic { + require.Panics(t, run) + } else { + require.NotPanics(t, run) + } + require.Equal(t, tc.want, got) }) } diff --git a/x/auction/types/auctions.go b/x/auction/types/auctions.go index 2e06b86e..ca5ea74f 100644 --- a/x/auction/types/auctions.go +++ b/x/auction/types/auctions.go @@ -287,15 +287,23 @@ func NewWeightedAddresses(addrs []sdk.AccAddress, weights []sdk.Int) (WeightedAd return wa, nil } -// Validate checks for that the weights are not negative and that lengths match. +// Validate checks for that the weights are not negative, not all zero, and the lengths match. func (wa WeightedAddresses) Validate() error { + if len(wa.Weights) < 1 { + return fmt.Errorf("must be at least 1 weighted address") + } if len(wa.Addresses) != len(wa.Weights) { return fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", len(wa.Addresses), len(wa.Weights)) } + totalWeight := sdk.ZeroInt() for _, w := range wa.Weights { if w.IsNegative() { return fmt.Errorf("weights contain a negative amount: %s", w) } + totalWeight = totalWeight.Add(w) + } + if !totalWeight.IsPositive() { + return fmt.Errorf("total weight must be positive") } return nil } diff --git a/x/auction/types/auctions_test.go b/x/auction/types/auctions_test.go index 49953cbe..7494612d 100644 --- a/x/auction/types/auctions_test.go +++ b/x/auction/types/auctions_test.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "testing" "time" @@ -24,48 +25,65 @@ const ( TestAccAddress2 = "kava1pdfav2cjhry9k79nu6r8kgknnjtq6a7rcr0qlr" ) +func d(amount string) sdk.Dec { return sdk.MustNewDecFromStr(amount) } +func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } +func i(n int64) sdk.Int { return sdk.NewInt(n) } +func is(ns ...int64) (is []sdk.Int) { + for _, n := range ns { + is = append(is, sdk.NewInt(n)) + } + return +} + func TestNewWeightedAddresses(t *testing.T) { tests := []struct { - name string - addresses []sdk.AccAddress - weights []sdk.Int - expectpass bool + name string + addresses []sdk.AccAddress + weights []sdk.Int + expectedErr error }{ { - "normal", - []sdk.AccAddress{ + name: "normal", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - sdk.NewInt(8), - }, - true, + weights: is(6, 8), + expectedErr: nil, }, { - "mismatched", - []sdk.AccAddress{ + name: "mismatched", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - }, - false, + weights: is(6), + expectedErr: fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", 2, 1), }, { - "negativeWeight", - []sdk.AccAddress{ + name: "negativeWeight", + addresses: []sdk.AccAddress{ sdk.AccAddress([]byte(TestAccAddress1)), sdk.AccAddress([]byte(TestAccAddress2)), }, - []sdk.Int{ - sdk.NewInt(6), - sdk.NewInt(-8), + weights: is(6, -8), + expectedErr: fmt.Errorf("weights contain a negative amount: %s", i(-8)), + }, + { + name: "zero total weights", + addresses: []sdk.AccAddress{ + sdk.AccAddress([]byte(TestAccAddress1)), + sdk.AccAddress([]byte(TestAccAddress2)), }, - false, + weights: is(0, 0), + expectedErr: fmt.Errorf("total weight must be positive"), + }, + { + name: "no weights", + addresses: nil, + weights: nil, + expectedErr: fmt.Errorf("must be at least 1 weighted address"), }, } @@ -75,27 +93,16 @@ func TestNewWeightedAddresses(t *testing.T) { // Attempt to instantiate new WeightedAddresses weightedAddresses, err := NewWeightedAddresses(tc.addresses, tc.weights) - if tc.expectpass { - // Confirm there is no error - require.Nil(t, err) - + if tc.expectedErr != nil { + // Confirm the error + require.EqualError(t, err, tc.expectedErr.Error()) + } else { + require.NoError(t, err) // Check addresses, weights require.Equal(t, tc.addresses, weightedAddresses.Addresses) require.Equal(t, tc.weights, weightedAddresses.Weights) - } else { - // Confirm that there is an error - require.NotNil(t, err) - - switch tc.name { - case "mismatched": - require.Contains(t, err.Error(), "number of addresses doesn't match number of weights") - case "negativeWeight": - require.Contains(t, err.Error(), "weights contain a negative amount") - default: - // Unexpected error state - t.Fail() - } } + }) } } diff --git a/x/auction/types/msg_test.go b/x/auction/types/msg_test.go index 7ac2909c..4c5ee977 100644 --- a/x/auction/types/msg_test.go +++ b/x/auction/types/msg_test.go @@ -39,5 +39,3 @@ func TestMsgPlaceBid_ValidateBasic(t *testing.T) { }) } } - -func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } diff --git a/x/auction/types/params_test.go b/x/auction/types/params_test.go index f2d65091..88d30f43 100644 --- a/x/auction/types/params_test.go +++ b/x/auction/types/params_test.go @@ -5,8 +5,6 @@ import ( "time" "github.com/stretchr/testify/require" - - sdk "github.com/cosmos/cosmos-sdk/types" ) func TestParams_Validate(t *testing.T) { @@ -105,5 +103,3 @@ func TestParams_Validate(t *testing.T) { }) } } - -func d(amount string) sdk.Dec { return sdk.MustNewDecFromStr(amount) } From 7c477eb75db277a5f42371582031b4deac2f1e28 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Mon, 11 May 2020 20:48:42 +0100 Subject: [PATCH 06/11] add cdp math fix and debugging --- x/cdp/keeper/auctions.go | 8 ++++++-- x/cdp/keeper/seize.go | 2 ++ x/cdp/keeper/seize_test.go | 5 +++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/x/cdp/keeper/auctions.go b/x/cdp/keeper/auctions.go index 5228ef08..5b739aee 100644 --- a/x/cdp/keeper/auctions.go +++ b/x/cdp/keeper/auctions.go @@ -1,6 +1,8 @@ package keeper import ( + "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/kava-labs/kava/x/cdp/types" @@ -60,6 +62,7 @@ func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt if err != nil { return err } + fmt.Printf("AuctionCollateral:createFromD:loop%d:liquidator:%s\n", i, k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) debt = debt.Sub(debtChange) totalCollateral = totalCollateral.Sub(collateralChange) dep.Amount = sdk.NewCoin(collateralDenom, collateralAmount.Sub(collateralChange)) @@ -68,7 +71,7 @@ func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt if !dep.Amount.IsZero() { // figure out how much debt this deposit accounts for // (depositCollateral / totalCollateral) * totalDebtFromCDP - debtCoveredByDeposit := (collateralAmount.Quo(totalCollateral)).Mul(debt) + debtCoveredByDeposit := collateralAmount.Mul(debt).Quo(totalCollateral) // integer division (ie rounds down) // if adding this deposit to the other partial deposits is less than a lot if (partialAuctionDeposits.SumCollateral().Add(collateralAmount)).LT(auctionSize) { // append the deposit to the partial deposits and zero out the deposit @@ -79,7 +82,7 @@ func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt // if the sum of partial deposits now makes a lot partialCollateral := sdk.NewCoin(collateralDenom, auctionSize.Sub(partialAuctionDeposits.SumCollateral())) partialAmount := partialCollateral.Amount - partialDebt := (partialAmount.Quo(collateralAmount)).Mul(debtCoveredByDeposit) + partialDebt := partialAmount.Mul(debtCoveredByDeposit).Quo(collateralAmount) // integer division (ie rounds down) // create a partial deposit from the deposit partialDep := newPartialDeposit(dep.Depositor, partialCollateral, partialDebt) @@ -90,6 +93,7 @@ func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt if err != nil { return err } + fmt.Printf("AuctionCollateral:createFromPD:loop%d:liquidator:%s\n", i, k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) debt = debt.Sub(debtChange) totalCollateral = totalCollateral.Sub(collateralChange) // reset partial deposits and update the deposit amount diff --git a/x/cdp/keeper/seize.go b/x/cdp/keeper/seize.go index b10c25ce..8d0112cc 100644 --- a/x/cdp/keeper/seize.go +++ b/x/cdp/keeper/seize.go @@ -32,6 +32,7 @@ func (k Keeper) SeizeCollateral(ctx sdk.Context, cdp types.CDP) error { if err != nil { return err } + fmt.Printf("SeizeCollateral:liquidator: %s\n", k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) // liquidate deposits and send collateral from cdp to liquidator for _, dep := range deposits { @@ -49,6 +50,7 @@ func (k Keeper) SeizeCollateral(ctx sdk.Context, cdp types.CDP) error { } k.DeleteDeposit(ctx, dep.CdpID, dep.Depositor) } + fmt.Printf("SeizeCollateral:liquidator: %s\n", k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) err = k.AuctionCollateral(ctx, deposits, debt, cdp.Principal.Denom) if err != nil { return err diff --git a/x/cdp/keeper/seize_test.go b/x/cdp/keeper/seize_test.go index 45b707b9..b76f1e6c 100644 --- a/x/cdp/keeper/seize_test.go +++ b/x/cdp/keeper/seize_test.go @@ -2,6 +2,7 @@ package keeper_test import ( "errors" + "fmt" "math/rand" "testing" "time" @@ -166,6 +167,10 @@ func (suite *SeizeTestSuite) TestSeizeCollateralMultiDeposit() { p := cdp.Principal.Amount cl := cdp.Collateral.Amount tpb := suite.keeper.GetTotalPrincipal(suite.ctx, "xrp", "usdx") + fmt.Printf("%s\n", cdp) + fmt.Printf("%s\n", deposits) + fmt.Printf("cdpaccount: %s\n", suite.app.GetSupplyKeeper().GetModuleAccount(suite.ctx, types.ModuleName)) + fmt.Printf("liquidator: %s\n", suite.app.GetSupplyKeeper().GetModuleAccount(suite.ctx, types.LiquidatorMacc)) err = suite.keeper.SeizeCollateral(suite.ctx, cdp) suite.NoError(err) tpa := suite.keeper.GetTotalPrincipal(suite.ctx, "xrp", "usdx") From d489bacfacac78edfe509941762a25864e7fef1d Mon Sep 17 00:00:00 2001 From: Ruaridh Date: Mon, 11 May 2020 21:07:39 +0100 Subject: [PATCH 07/11] tidy payout function Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- x/auction/keeper/auctions.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index 8a66f939..e3c6a1df 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -350,11 +350,12 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc } for i, payout := range lotPayouts { // if the payout amount is 0, don't send 0 coins - if payout.IsPositive() { - err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout)) - if err != nil { - return a, err - } + if !payout.IsPositive() { + break + } + err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout)) + if err != nil { + return a, err } } From 89b63a3cba0a7eb92081dafc178bf8a63445da54 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Mon, 11 May 2020 18:35:16 -0400 Subject: [PATCH 08/11] fix: simplify collateral auction logic --- x/cdp/keeper/auctions.go | 172 ++++++++----------------------------- x/cdp/keeper/seize.go | 2 - x/cdp/keeper/seize_test.go | 5 -- 3 files changed, 34 insertions(+), 145 deletions(-) diff --git a/x/cdp/keeper/auctions.go b/x/cdp/keeper/auctions.go index 5b739aee..b6f55069 100644 --- a/x/cdp/keeper/auctions.go +++ b/x/cdp/keeper/auctions.go @@ -1,8 +1,6 @@ package keeper import ( - "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/kava-labs/kava/x/cdp/types" @@ -13,100 +11,15 @@ const ( dump = 100 ) -type partialDeposit struct { - Depositor sdk.AccAddress - Amount sdk.Coin - DebtShare sdk.Int -} - -func newPartialDeposit(depositor sdk.AccAddress, amount sdk.Coin, ds sdk.Int) partialDeposit { - return partialDeposit{ - Depositor: depositor, - Amount: amount, - DebtShare: ds, - } -} - -type partialDeposits []partialDeposit - -func (pd partialDeposits) SumCollateral() (sum sdk.Int) { - sum = sdk.ZeroInt() - for _, d := range pd { - sum = sum.Add(d.Amount.Amount) - } - return -} - -func (pd partialDeposits) SumDebt() (sum sdk.Int) { - sum = sdk.ZeroInt() - for _, d := range pd { - sum = sum.Add(d.DebtShare) - } - return -} - // AuctionCollateral creates auctions from the input deposits which attempt to raise the corresponding amount of debt func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt sdk.Int, bidDenom string) error { - auctionSize := k.getAuctionSize(ctx, deposits[0].Amount.Denom) - partialAuctionDeposits := partialDeposits{} - totalCollateral := deposits.SumCollateral() - for totalCollateral.GT(sdk.ZeroInt()) { - for i, dep := range deposits { - if dep.Amount.IsZero() { - continue - } - collateralAmount := dep.Amount.Amount - collateralDenom := dep.Amount.Denom - // create auctions from individual deposits that are larger than the auction size - debtChange, collateralChange, err := k.CreateAuctionsFromDeposit(ctx, dep, debt, totalCollateral, auctionSize, bidDenom) - if err != nil { - return err - } - fmt.Printf("AuctionCollateral:createFromD:loop%d:liquidator:%s\n", i, k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) - debt = debt.Sub(debtChange) - totalCollateral = totalCollateral.Sub(collateralChange) - dep.Amount = sdk.NewCoin(collateralDenom, collateralAmount.Sub(collateralChange)) - collateralAmount = collateralAmount.Sub(collateralChange) - // if there is leftover collateral that is less than a lot - if !dep.Amount.IsZero() { - // figure out how much debt this deposit accounts for - // (depositCollateral / totalCollateral) * totalDebtFromCDP - debtCoveredByDeposit := collateralAmount.Mul(debt).Quo(totalCollateral) // integer division (ie rounds down) - // if adding this deposit to the other partial deposits is less than a lot - if (partialAuctionDeposits.SumCollateral().Add(collateralAmount)).LT(auctionSize) { - // append the deposit to the partial deposits and zero out the deposit - pd := newPartialDeposit(dep.Depositor, dep.Amount, debtCoveredByDeposit) - partialAuctionDeposits = append(partialAuctionDeposits, pd) - dep.Amount = sdk.NewCoin(collateralDenom, sdk.ZeroInt()) - } else { - // if the sum of partial deposits now makes a lot - partialCollateral := sdk.NewCoin(collateralDenom, auctionSize.Sub(partialAuctionDeposits.SumCollateral())) - partialAmount := partialCollateral.Amount - partialDebt := partialAmount.Mul(debtCoveredByDeposit).Quo(collateralAmount) // integer division (ie rounds down) - // create a partial deposit from the deposit - partialDep := newPartialDeposit(dep.Depositor, partialCollateral, partialDebt) - // append it to the partial deposits - partialAuctionDeposits = append(partialAuctionDeposits, partialDep) - // create an auction from the partial deposits - debtChange, collateralChange, err := k.CreateAuctionFromPartialDeposits(ctx, partialAuctionDeposits, debt, totalCollateral, auctionSize, bidDenom) - if err != nil { - return err - } - fmt.Printf("AuctionCollateral:createFromPD:loop%d:liquidator:%s\n", i, k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) - debt = debt.Sub(debtChange) - totalCollateral = totalCollateral.Sub(collateralChange) - // reset partial deposits and update the deposit amount - partialAuctionDeposits = partialDeposits{} - dep.Amount = sdk.NewCoin(collateralDenom, collateralAmount.Sub(partialAmount)) - } - } - deposits[i] = dep - totalCollateral = deposits.SumCollateral() - } - } - if partialAuctionDeposits.SumCollateral().GT(sdk.ZeroInt()) { - _, _, err := k.CreateAuctionFromPartialDeposits(ctx, partialAuctionDeposits, debt, totalCollateral, partialAuctionDeposits.SumCollateral(), bidDenom) + auctionSize := k.getAuctionSize(ctx, deposits[0].Amount.Denom) + totalCollateral := deposits.SumCollateral() + for _, deposit := range deposits { + + debtCoveredByDeposit := (sdk.NewDecFromInt(deposit.Amount.Amount).Quo(sdk.NewDecFromInt(totalCollateral))).Mul(sdk.NewDecFromInt(debt)).RoundInt() + err := k.CreateAuctionsFromDeposit(ctx, deposit.Amount, deposit.Depositor, debtCoveredByDeposit, auctionSize, bidDenom) if err != nil { return err } @@ -114,54 +27,37 @@ func (k Keeper) AuctionCollateral(ctx sdk.Context, deposits types.Deposits, debt return nil } -// CreateAuctionsFromDeposit creates auctions from the input deposit until there is less than auctionSize left on the deposit +// CreateAuctionsFromDeposit creates auctions from the input deposit func (k Keeper) CreateAuctionsFromDeposit( - ctx sdk.Context, dep types.Deposit, debt sdk.Int, totalCollateral sdk.Int, auctionSize sdk.Int, - principalDenom string) (debtChange sdk.Int, collateralChange sdk.Int, err error) { - debtChange = sdk.ZeroInt() - collateralChange = sdk.ZeroInt() - depositAmount := dep.Amount.Amount - depositDenom := dep.Amount.Denom - for depositAmount.GTE(auctionSize) { - // figure out how much debt is covered by one lots worth of collateral - depositDebtAmount := (sdk.NewDecFromInt(auctionSize).Quo(sdk.NewDecFromInt(totalCollateral))).Mul(sdk.NewDecFromInt(debt)).RoundInt() - penalty := k.ApplyLiquidationPenalty(ctx, depositDenom, depositDebtAmount) - // start an auction for one lot, attempting to raise depositDebtAmount plus the liquidation penalty - _, err := k.auctionKeeper.StartCollateralAuction( - ctx, types.LiquidatorMacc, sdk.NewCoin(depositDenom, auctionSize), sdk.NewCoin(principalDenom, depositDebtAmount.Add(penalty)), []sdk.AccAddress{dep.Depositor}, - []sdk.Int{auctionSize}, sdk.NewCoin(k.GetDebtDenom(ctx), depositDebtAmount)) - if err != nil { - return sdk.ZeroInt(), sdk.ZeroInt(), err + ctx sdk.Context, collateral sdk.Coin, returnAddr sdk.AccAddress, debt, auctionSize sdk.Int, + principalDenom string) (err error) { + + amountToAuction := sdk.NewInt(collateral.Amount.Int64()) + totalCollateralAmount := sdk.NewInt(collateral.Amount.Int64()) + remainingDebt := sdk.NewInt(debt.Int64()) + for amountToAuction.GT(sdk.ZeroInt()) { + for amountToAuction.GT(auctionSize) { + debtCoveredByAuction := (sdk.NewDecFromInt(auctionSize).Quo(sdk.NewDecFromInt(totalCollateralAmount))).Mul(sdk.NewDecFromInt(debt)).RoundInt() + penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, debtCoveredByAuction) + _, err := k.auctionKeeper.StartCollateralAuction( + ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, auctionSize), sdk.NewCoin(principalDenom, debtCoveredByAuction.Add(penalty)), []sdk.AccAddress{returnAddr}, + []sdk.Int{auctionSize}, sdk.NewCoin(k.GetDebtDenom(ctx), debtCoveredByAuction)) + if err != nil { + return err + } + amountToAuction = amountToAuction.Sub(auctionSize) + remainingDebt = remainingDebt.Sub(debtCoveredByAuction) } - depositAmount = depositAmount.Sub(auctionSize) - totalCollateral = totalCollateral.Sub(auctionSize) - debt = debt.Sub(depositDebtAmount) - // subtract one lot's worth of debt from the total debt covered by this deposit - debtChange = debtChange.Add(depositDebtAmount) - collateralChange = collateralChange.Add(auctionSize) - + penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, remainingDebt) + _, err := k.auctionKeeper.StartCollateralAuction( + ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, amountToAuction), sdk.NewCoin(principalDenom, remainingDebt.Add(penalty)), []sdk.AccAddress{returnAddr}, + []sdk.Int{amountToAuction}, sdk.NewCoin(k.GetDebtDenom(ctx), remainingDebt)) + if err != nil { + return err + } + amountToAuction = sdk.ZeroInt() } - return debtChange, collateralChange, nil -} - -// CreateAuctionFromPartialDeposits creates an auction from the input partial deposits -func (k Keeper) CreateAuctionFromPartialDeposits(ctx sdk.Context, partialDeps partialDeposits, debt sdk.Int, collateral sdk.Int, auctionSize sdk.Int, bidDenom string) (debtChange, collateralChange sdk.Int, err error) { - - returnAddrs := []sdk.AccAddress{} - returnWeights := []sdk.Int{} - depositDenom := partialDeps[0].Amount.Denom - for _, pd := range partialDeps { - returnAddrs = append(returnAddrs, pd.Depositor) - returnWeights = append(returnWeights, pd.DebtShare) - } - penalty := k.ApplyLiquidationPenalty(ctx, depositDenom, partialDeps.SumDebt()) - _, err = k.auctionKeeper.StartCollateralAuction(ctx, types.LiquidatorMacc, sdk.NewCoin(partialDeps[0].Amount.Denom, auctionSize), sdk.NewCoin(bidDenom, partialDeps.SumDebt().Add(penalty)), returnAddrs, returnWeights, sdk.NewCoin(k.GetDebtDenom(ctx), partialDeps.SumDebt())) - if err != nil { - return sdk.ZeroInt(), sdk.ZeroInt(), err - } - debtChange = partialDeps.SumDebt() - collateralChange = partialDeps.SumCollateral() - return debtChange, collateralChange, nil + return nil } // NetSurplusAndDebt burns surplus and debt coins equal to the minimum of surplus and debt balances held by the liquidator module account diff --git a/x/cdp/keeper/seize.go b/x/cdp/keeper/seize.go index 8d0112cc..b10c25ce 100644 --- a/x/cdp/keeper/seize.go +++ b/x/cdp/keeper/seize.go @@ -32,7 +32,6 @@ func (k Keeper) SeizeCollateral(ctx sdk.Context, cdp types.CDP) error { if err != nil { return err } - fmt.Printf("SeizeCollateral:liquidator: %s\n", k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) // liquidate deposits and send collateral from cdp to liquidator for _, dep := range deposits { @@ -50,7 +49,6 @@ func (k Keeper) SeizeCollateral(ctx sdk.Context, cdp types.CDP) error { } k.DeleteDeposit(ctx, dep.CdpID, dep.Depositor) } - fmt.Printf("SeizeCollateral:liquidator: %s\n", k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc)) err = k.AuctionCollateral(ctx, deposits, debt, cdp.Principal.Denom) if err != nil { return err diff --git a/x/cdp/keeper/seize_test.go b/x/cdp/keeper/seize_test.go index b76f1e6c..45b707b9 100644 --- a/x/cdp/keeper/seize_test.go +++ b/x/cdp/keeper/seize_test.go @@ -2,7 +2,6 @@ package keeper_test import ( "errors" - "fmt" "math/rand" "testing" "time" @@ -167,10 +166,6 @@ func (suite *SeizeTestSuite) TestSeizeCollateralMultiDeposit() { p := cdp.Principal.Amount cl := cdp.Collateral.Amount tpb := suite.keeper.GetTotalPrincipal(suite.ctx, "xrp", "usdx") - fmt.Printf("%s\n", cdp) - fmt.Printf("%s\n", deposits) - fmt.Printf("cdpaccount: %s\n", suite.app.GetSupplyKeeper().GetModuleAccount(suite.ctx, types.ModuleName)) - fmt.Printf("liquidator: %s\n", suite.app.GetSupplyKeeper().GetModuleAccount(suite.ctx, types.LiquidatorMacc)) err = suite.keeper.SeizeCollateral(suite.ctx, cdp) suite.NoError(err) tpa := suite.keeper.GetTotalPrincipal(suite.ctx, "xrp", "usdx") From 6b478a0f95e97ac463fa6effd3f6564ef4e680da Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Tue, 12 May 2020 00:57:48 +0100 Subject: [PATCH 09/11] minor refactor --- x/cdp/keeper/auctions.go | 41 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/x/cdp/keeper/auctions.go b/x/cdp/keeper/auctions.go index b6f55069..e7c0b7ec 100644 --- a/x/cdp/keeper/auctions.go +++ b/x/cdp/keeper/auctions.go @@ -32,31 +32,32 @@ func (k Keeper) CreateAuctionsFromDeposit( ctx sdk.Context, collateral sdk.Coin, returnAddr sdk.AccAddress, debt, auctionSize sdk.Int, principalDenom string) (err error) { - amountToAuction := sdk.NewInt(collateral.Amount.Int64()) - totalCollateralAmount := sdk.NewInt(collateral.Amount.Int64()) - remainingDebt := sdk.NewInt(debt.Int64()) - for amountToAuction.GT(sdk.ZeroInt()) { - for amountToAuction.GT(auctionSize) { - debtCoveredByAuction := (sdk.NewDecFromInt(auctionSize).Quo(sdk.NewDecFromInt(totalCollateralAmount))).Mul(sdk.NewDecFromInt(debt)).RoundInt() - penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, debtCoveredByAuction) - _, err := k.auctionKeeper.StartCollateralAuction( - ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, auctionSize), sdk.NewCoin(principalDenom, debtCoveredByAuction.Add(penalty)), []sdk.AccAddress{returnAddr}, - []sdk.Int{auctionSize}, sdk.NewCoin(k.GetDebtDenom(ctx), debtCoveredByAuction)) - if err != nil { - return err - } - amountToAuction = amountToAuction.Sub(auctionSize) - remainingDebt = remainingDebt.Sub(debtCoveredByAuction) - } - penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, remainingDebt) + amountToAuction := collateral.Amount + totalCollateralAmount := collateral.Amount + remainingDebt := debt + if !amountToAuction.IsPositive() { + return nil + } + for amountToAuction.GT(auctionSize) { + debtCoveredByAuction := (sdk.NewDecFromInt(auctionSize).Quo(sdk.NewDecFromInt(totalCollateralAmount))).Mul(sdk.NewDecFromInt(debt)).RoundInt() + penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, debtCoveredByAuction) _, err := k.auctionKeeper.StartCollateralAuction( - ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, amountToAuction), sdk.NewCoin(principalDenom, remainingDebt.Add(penalty)), []sdk.AccAddress{returnAddr}, - []sdk.Int{amountToAuction}, sdk.NewCoin(k.GetDebtDenom(ctx), remainingDebt)) + ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, auctionSize), sdk.NewCoin(principalDenom, debtCoveredByAuction.Add(penalty)), []sdk.AccAddress{returnAddr}, + []sdk.Int{auctionSize}, sdk.NewCoin(k.GetDebtDenom(ctx), debtCoveredByAuction)) if err != nil { return err } - amountToAuction = sdk.ZeroInt() + amountToAuction = amountToAuction.Sub(auctionSize) + remainingDebt = remainingDebt.Sub(debtCoveredByAuction) } + penalty := k.ApplyLiquidationPenalty(ctx, collateral.Denom, remainingDebt) + _, err = k.auctionKeeper.StartCollateralAuction( + ctx, types.LiquidatorMacc, sdk.NewCoin(collateral.Denom, amountToAuction), sdk.NewCoin(principalDenom, remainingDebt.Add(penalty)), []sdk.AccAddress{returnAddr}, + []sdk.Int{amountToAuction}, sdk.NewCoin(k.GetDebtDenom(ctx), remainingDebt)) + if err != nil { + return err + } + return nil } From bc1a6a68e0beac7c33c804a7b3ddf8f9cd3da220 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Tue, 12 May 2020 01:06:32 +0100 Subject: [PATCH 10/11] bugfix --- x/auction/keeper/auctions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auction/keeper/auctions.go b/x/auction/keeper/auctions.go index e3c6a1df..e9b2a397 100644 --- a/x/auction/keeper/auctions.go +++ b/x/auction/keeper/auctions.go @@ -351,7 +351,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc for i, payout := range lotPayouts { // if the payout amount is 0, don't send 0 coins if !payout.IsPositive() { - break + continue } err = k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, a.LotReturns.Addresses[i], sdk.NewCoins(payout)) if err != nil { From f0c750cb4a6a66fa79ed6243a0380021a621ab29 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Tue, 12 May 2020 01:10:57 +0100 Subject: [PATCH 11/11] remove cdp review comment --- x/cdp/abci.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/cdp/abci.go b/x/cdp/abci.go index 1207c8bc..92781fa0 100644 --- a/x/cdp/abci.go +++ b/x/cdp/abci.go @@ -25,7 +25,6 @@ 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(