R4R: Move liquidator functions to cdp module #280

karzak · 2020-01-13T17:05:42Z

Features:

CollateralAuctions are created immediately when the cdp is seized
Implemented netting of surplus and debt
Implemented starting of surplus/debt auctions
Added gov denom to genesis state
Added params for surplus and debt auction thresholds

rhuairahrighairidh

Overall looks good, left some comments on the mathy parts.

rhuairahrighairidh · 2020-01-14T14:45:49Z

x/cdp/keeper/auctions.go

+				if balance.LT(netAmount) {
+					err = k.supplyKeeper.BurnCoins(ctx, types.LiquidatorMacc, sdk.NewCoins(sdk.NewCoin(dp.Denom, balance)))
+					if err != nil {
+						panic(err)


return error instead of panic? Then it's up to the caller to decide whether to panic or do something else.

rhuairahrighairidh · 2020-01-14T14:55:00Z

x/cdp/keeper/auctions.go

+	remainingDebt := k.GetTotalDebt(ctx, types.LiquidatorMacc)
+	params := k.GetParams(ctx)
+	if remainingDebt.GTE(params.DebtAuctionThreshold) {
+		_, err := k.auctionKeeper.StartDebtAuction(ctx, types.LiquidatorMacc, sdk.NewCoin("usdx", sdk.NewInt(1)), sdk.NewCoin(k.GetGovDenom(ctx), remainingDebt.Mul(sdk.NewInt(100))), sdk.NewCoin(k.GetDebtDenom(ctx), remainingDebt))


I think the bid arg here should be remainingDebt as its a reverse auction and that is the amount you want to raise.
Also might be neater to move the 100x multiplier for the lot to a constant

rhuairahrighairidh · 2020-01-14T14:57:14Z

x/cdp/keeper/auctions.go

+}
+
+// HandleSurplusAndDebtAuctions nets the surplus and debt balances and then creates surplus or debt auctions if the remaining balance is above the auction threshold parameter
+func (k Keeper) HandleSurplusAndDebtAuctions(ctx sdk.Context) {


RunSurplusAndDebtAuctions? 'Handle' is a bit vague

rhuairahrighairidh · 2020-01-14T15:05:45Z

x/cdp/keeper/auctions.go

+		for _, dp := range k.GetParams(ctx).DebtParams {
+			for surplusToBurn.GT(sdk.ZeroInt()) {
+				balance := k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc).GetCoins().AmountOf(dp.Denom)
+				if balance.LT(netAmount) {
+					err = k.supplyKeeper.BurnCoins(ctx, types.LiquidatorMacc, sdk.NewCoins(sdk.NewCoin(dp.Denom, balance)))
+					if err != nil {
+						panic(err)
+					}
+					surplusToBurn = surplusToBurn.Sub(balance)
+				} else {
+					err = k.supplyKeeper.BurnCoins(ctx, types.LiquidatorMacc, sdk.NewCoins(sdk.NewCoin(dp.Denom, surplusToBurn)))
+					if err != nil {
+						panic(err)
+					}
+					surplusToBurn = sdk.ZeroInt()
+				}
+			}


It looks like this can get caught in a loop on the first dp (if it's balance is < netAmount)

Good catch! The outer and inner loops should be switched.

rhuairahrighairidh · 2020-01-14T15:11:06Z

x/cdp/keeper/auctions.go

+	auctionSize := k.getAuctionSize(ctx, deposits[0].Amount[0].Denom)
+	partialAuctionDeposits := partialDeposits{}
+	totalCollateral := deposits.SumCollateral()
+	for deposits.SumCollateral().GTE(auctionSize) {


If total deposits are < auctionSize then no auction will be started, but the cdp will still have been seized

Good catch, should be checking for greater than zero.

rhuairahrighairidh · 2020-01-14T15:27:50Z

x/cdp/keeper/auctions.go

+		for i, dep := range deposits {
+			// create auctions from individual deposits that are larger than the auction size
+			k.CreateAuctionsFromDeposit(ctx, &dep, &debt, &totalCollateral, auctionSize, bidDenom)
+			if !dep.Amount[0].Amount.IsZero() {


Found these loops a bit hard to understand, comments might help explain why things are happening

denalimarsh · 2020-01-14T15:45:30Z

x/cdp/keeper/auctions.go

+	auctionSize := k.getAuctionSize(ctx, deposits[0].Amount[0].Denom)
+	partialAuctionDeposits := partialDeposits{}
+	totalCollateral := deposits.SumCollateral()
+	for deposits.SumCollateral().GTE(auctionSize) {


Replace deposits.SumCollateral() with totalCollateral

denalimarsh · 2020-01-14T15:51:11Z

x/cdp/keeper/auctions.go

+type partialDeposit struct {
+	Depositor sdk.AccAddress
+	Amount    sdk.Coins
+	DebtShare sdk.Int
+}


Worth discussing if partialDeposit should be defined in a separate file or moved to cdp/types.

denalimarsh · 2020-01-14T16:06:25Z

x/cdp/keeper/auctions.go

+	partialAuctionDeposits := partialDeposits{}
+	totalCollateral := deposits.SumCollateral()
+	for deposits.SumCollateral().GTE(auctionSize) {
+		for i, dep := range deposits {


Placeholder variables for denom, amount

denalimarsh · 2020-01-14T16:15:00Z

x/cdp/keeper/auctions.go

+}
+
+// CreateAuctionsFromDeposit creates auctions from the input deposit until there is less than auctionSize left on the deposit
+func (k Keeper) CreateAuctionsFromDeposit(ctx sdk.Context, dep *types.Deposit, debt *sdk.Int, totalCollateral *sdk.Int, auctionSize sdk.Int, principalDenom string) {


This method would be simpler if it used local variables to keep track of the data instead of updating the state via ref in each iteration of the loop. Return local variables and use them to update the state back in the primary function.

denalimarsh · 2020-01-14T16:28:00Z

x/cdp/keeper/params.go

+func (k Keeper) getLiquidationPenalty(ctx sdk.Context, denom string) sdk.Dec {
+	cp, found := k.GetCollateral(ctx, denom)
+	if !found {
+		panic(fmt.Sprintf("collateral not found: %s", denom))


Will panic if collateral type type 'random' is removed in between CDP deposit of 'random' and liquidation of the CDP.

Will file an issue on this topic. It's worth documenting and thinking about how we want to handle it, but for now, it would be unlikely for governance to accept a proposal to remove a collateral that still had active cdps.

denalimarsh · 2020-01-14T16:28:34Z

x/cdp/keeper/params.go

+func (k Keeper) getAuctionSize(ctx sdk.Context, denom string) sdk.Int {
+	cp, found := k.GetCollateral(ctx, denom)
+	if !found {
+		panic(fmt.Sprintf("collateral not found: %s", denom))


denalimarsh

One minor change requested, but approving the PR as it can be addressed later.

denalimarsh · 2020-01-15T10:16:17Z

x/cdp/keeper/auctions.go

+				collateralAmount := dep.Amount[0].Amount
+				collateralDenom := dep.Amount[0].Denom


Can move these up a couple of lines and replace dep.Amount[0].Denom, dep.Amount[0].Amount on line 59 with the local variables.

rhuairahrighairidh · 2020-01-15T11:17:06Z

nit: In seize_test.go in setup, a new rand source doesn't need to be set up for each loop iteration.

rhuairahrighairidh

I'd like there to be more tests of the mathy bits before testnet. But this could be a subsequent PR.

rhuairahrighairidh · 2020-01-15T11:26:47Z

x/cdp/keeper/auctions.go

+	if netAmount.GT(sdk.ZeroInt()) {
+		surplusToBurn := netAmount
+		err := k.supplyKeeper.BurnCoins(ctx, types.LiquidatorMacc, sdk.NewCoins(sdk.NewCoin(k.GetDebtDenom(ctx), netAmount)))
+		if err != nil {
+			return err
+		}


This if could be

if netAmount.IsZero() { return nil } surplusToBurn := ...

It would remove a level of indentation from the subsequent section.

rhuairahrighairidh · 2020-01-15T11:30:52Z

x/cdp/keeper/auctions.go

+		for surplusToBurn.GT(sdk.ZeroInt()) {
+			for _, dp := range k.GetParams(ctx).DebtParams {
+				balance := k.supplyKeeper.GetModuleAccount(ctx, types.LiquidatorMacc).GetCoins().AmountOf(dp.Denom)
+				if balance.LT(netAmount) {


Should this beif balance.LT(surplusToBurn) {?
netAmount doesn't change in this loop.

rhuairahrighairidh · 2020-01-15T11:39:02Z

x/cdp/keeper/auctions.go

+			if dep.Amount.IsZero() {
+				// remove the deposit from the slice if it is empty
+				deposits = append(deposits[:i], deposits[i+1:]...)
+				i--


my editor is saying i-- is an ineffective assignment. Maye although it's updated here, the next for loop iteration overwrites it?
Could also be a false positive

rhuairahrighairidh

LGTM

* wip: tpyes and keeper methods * wip: iterators * wip: types and keeper methods * wip: add msgs * wip: client methods * wip: rebase develop * wip: types tests * wip: keeper tests, small fixes * wip: add cdp tests * wip: deposit tests * wip: keeper tests * wip: tests and module methods * feat: error when fetching expired price * feat: conversion factor for external assets * feat: debt floor for new cdps * feat: save deposits on export genesis * feat: ensure messages implement msg * feat: index deposits by status * fix: stray comment * wip: address review comments * address review comments * wip: move liquidation to cdp module * wip: handle liquidations directly * wip: use new auction interface * feat: auction collateral in cdp begin block * feat: update param validation * feat: surplus and debt auctions * address review comments * address review comments * fix: auction multiple deposits * clean up netting function

karzak added 29 commits December 12, 2019 14:27

wip: tpyes and keeper methods

ac471e0

wip: iterators

e2d5797

wip: types and keeper methods

9f8b733

wip: add msgs

a4293be

wip: client methods

dce5687

wip: rebase develop

9e8d857

wip: types tests

d548ff6

wip: keeper tests, small fixes

b8cbb54

wip: add cdp tests

f3ee33d

wip: deposit tests

5a36f9f

wip: keeper tests

f4b7869

wip: tests and module methods

03320ae

feat: error when fetching expired price

153ff5f

feat: conversion factor for external assets

8786042

feat: debt floor for new cdps

3bbc4f7

feat: save deposits on export genesis

4791b4d

feat: ensure messages implement msg

dc9e103

feat: index deposits by status

4cb344a

fix: stray comment

0a65454

wip: address review comments

5a31fe2

address review comments

039e142

wip: move liquidation to cdp module

7e898d7

Merge 'origin/develop' into kd-move-liquidator

62bec67

merge auction changes

be7d4ba

wip: handle liquidations directly

1632088

wip: use new auction interface

489c6a2

feat: auction collateral in cdp begin block

73384da

feat: update param validation

8155fc2

feat: surplus and debt auctions

729b754

karzak added the R4R When a PR is ready for review label Jan 13, 2020

karzak added the cdp issues for cdp features label Jan 13, 2020

karzak requested review from rhuairahrighairidh and denalimarsh January 13, 2020 17:06

karzak added 2 commits January 14, 2020 15:14

Merge remote-tracking branch 'origin/develop' into kd-move-liquidator

107b444

Merge 'origin/develop' into kd-move-liquidator

47f4dfd

rhuairahrighairidh requested changes Jan 14, 2020

View reviewed changes

denalimarsh reviewed Jan 14, 2020

View reviewed changes

address review comments

88e7840

karzak requested a review from rhuairahrighairidh January 15, 2020 10:02

denalimarsh approved these changes Jan 15, 2020

View reviewed changes

address review comments

482bad3

rhuairahrighairidh requested changes Jan 15, 2020

View reviewed changes

karzak added 2 commits January 15, 2020 13:43

fix: auction multiple deposits

927166c

clean up netting function

794b28e

rhuairahrighairidh self-requested a review January 15, 2020 14:18

rhuairahrighairidh approved these changes Jan 15, 2020

View reviewed changes

karzak merged commit 9b1bf55 into develop Jan 15, 2020

karzak deleted the kd-move-liquidator branch January 15, 2020 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Move liquidator functions to cdp module #280

R4R: Move liquidator functions to cdp module #280

karzak commented Jan 13, 2020

rhuairahrighairidh left a comment

rhuairahrighairidh Jan 14, 2020

rhuairahrighairidh Jan 14, 2020

rhuairahrighairidh Jan 14, 2020

rhuairahrighairidh Jan 14, 2020

karzak Jan 14, 2020

rhuairahrighairidh Jan 14, 2020

karzak Jan 14, 2020

rhuairahrighairidh Jan 14, 2020

denalimarsh Jan 14, 2020

denalimarsh Jan 14, 2020

denalimarsh Jan 14, 2020

denalimarsh Jan 14, 2020

denalimarsh Jan 14, 2020

karzak Jan 15, 2020

denalimarsh Jan 14, 2020

denalimarsh left a comment

denalimarsh Jan 15, 2020

rhuairahrighairidh commented Jan 15, 2020

rhuairahrighairidh left a comment

rhuairahrighairidh Jan 15, 2020

rhuairahrighairidh Jan 15, 2020

rhuairahrighairidh Jan 15, 2020

rhuairahrighairidh left a comment

		collateralAmount := dep.Amount[0].Amount
		collateralDenom := dep.Amount[0].Denom

R4R: Move liquidator functions to cdp module #280

R4R: Move liquidator functions to cdp module #280

Conversation

karzak commented Jan 13, 2020

rhuairahrighairidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denalimarsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuairahrighairidh commented Jan 15, 2020

rhuairahrighairidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuairahrighairidh left a comment

Choose a reason for hiding this comment