-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
R4R: CDP types and methods #275
Conversation
4752bba
to
9e8d857
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Review comments are mostly focused on general cleaning and limiting code duplication. Are there any specific Golang styles we're using? I'm pretty sure Cosmos has been using Uber's Go style guide.
x/cdp/keeper/deposit.go
Outdated
// deposits blocked if cdp is in liquidation, have to check all deposits | ||
deposits := k.GetDeposits(ctx, cdp.ID) | ||
for _, d := range deposits { | ||
if d.InLiquidation { | ||
return types.ErrCdpNotAvailable(k.codespace, cdp.ID) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated below on lines 73-79 and in draw.go
on lines 17-22 and 95-100. Could be potentially cleaned up by moving this block into a new method such as func (k Keeper) ValidateAvailableCDP()
.
if !found { | ||
return types.ErrCdpNotFound(k.codespace, owner, collateral[0].Denom) | ||
} | ||
// withdrawals blocked if cdp is in liquidation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear we reduce duplication here, as the GetCdpByOwnerAndDenom
call is necessary, so to replace it was a ValidateCdpFound()
method wouldn't do much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was referring to lines 73-79. It appears that my single line selection during reviews is buggy and automatically selects a 4 line block with the single selected line at the bottom, noticed it over in #276 as well. That seems to be the source of the confusion here.
collateralizationRatio, err := k.CalculateCollateralizationRatio(ctx, cdp.Collateral.Sub(collateral), cdp.Principal, cdp.AccumulatedFees.Add(fees)) | ||
if err != nil { | ||
return err | ||
} | ||
liquidationRatio := k.getLiquidationRatio(ctx, collateral[0].Denom) | ||
if collateralizationRatio.LT(liquidationRatio) { | ||
return types.ErrInvalidCollateralRatio(k.codespace, collateral[0].Denom, collateralizationRatio, liquidationRatio) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to draw.go
lines 32-40 and cdp.go
lines 27-34. Can we move this to a ValidateCollateralizationRatio()
helper function which checks for ErrInvalidCollateralRatio
?
x/cdp/keeper/draw.go
Outdated
} | ||
|
||
// fee calculation | ||
periods := sdk.NewInt(ctx.BlockTime().Unix() - cdp.FeesUpdated.Unix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to use ...Sub(sdk.NewInt(cdp.FeesUpdated.Unix()))
, as done in deposit.go
lines 49 and 91.
x/cdp/keeper/draw.go
Outdated
} | ||
|
||
// calculate fees | ||
periods := sdk.NewInt(ctx.BlockTime().Unix() - cdp.FeesUpdated.Unix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
x/cdp/keeper/deposit.go
Outdated
if deposit.InLiquidation { | ||
return types.ErrDepositNotAvailable(k.codespace, cdp.ID, depositor) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the liquidation status check on line 76, will this condition ever be true?
x/cdp/keeper/cdp.go
Outdated
collateralRatio, err := k.CalculateCollateralizationRatio(ctx, collateral, principal, sdk.NewCoins()) | ||
if err != nil { | ||
return err | ||
} | ||
liquidationRatio := k.getLiquidationRatio(ctx, collateral[0].Denom) | ||
if collateralRatio.LT(liquidationRatio) { | ||
return types.ErrInvalidCollateralRatio(k.codespace, collateral[0].Denom, collateralRatio, liquidationRatio) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #275 (comment)
if dc.Amount.LT(dp.DebtFloor) { | ||
return types.ErrBelowDebtFloor(k.codespace, sdk.NewCoins(dc), dp.DebtFloor) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a DebtFloor check also be added somewhere in DepositCollateral()
/ValidatePrincipalDraw()
to prevent dust with additional principal coin types? May allow us to combine this function with ValidatePrincipalDraw()
, but I'm not sure I fully understand all the potential implications of such an addition.
x/cdp/types/keys.go
Outdated
// - 0x07<denom>:feeRate | ||
// - 0x08:previousBlockTime | ||
// - 0x20 - 0xff are reserved for collaterals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment? Correct me if I'm wrong but collateral types are assigned byte ids, which as used in store keys, but their namespace doesn't conflict with the other store keys. So they can start at 0x00.
func (k Keeper) IndexCdpByCollateralRatio(ctx sdk.Context, denom string, id uint64, collateralRatio sdk.Dec) { | ||
store := prefix.NewStore(ctx.KVStore(k.key), types.CollateralRatioIndexPrefix) | ||
db, _ := k.GetDenomPrefix(ctx, denom) | ||
store.Set(types.CollateralRatioKey(db, id, collateralRatio), types.GetCdpIDBytes(id)) | ||
} | ||
|
||
// RemoveCdpCollateralRatioIndex deletes the cdp id from the store's index of cdps by collateral type and collateral to debt ratio | ||
func (k Keeper) RemoveCdpCollateralRatioIndex(ctx sdk.Context, denom string, id uint64, collateralRatio sdk.Dec) { | ||
store := prefix.NewStore(ctx.KVStore(k.key), types.CollateralRatioIndexPrefix) | ||
db, _ := k.GetDenomPrefix(ctx, denom) | ||
store.Delete(types.CollateralRatioKey(db, id, collateralRatio)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the staking
module, any index updates are written into the Get/Set methods. I kind of like that approach - reduces code repitition in higher level keeper methods, and avoids the posibility that indexes are not updated when they should be.
Do you think it's worth doing for CDP indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure what the suggestion is here. Looking through staking
, the Set
methods are pretty small and there are separate methods for indexes (ie SetValidatorByPowerIndex
). The main difference appears to be that there is only one key prefix, which I'm not sure I can accomplish. I thought about the various indexes and iterators that were desirable for CDPs and couldn't come up with an approach that had fewer keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah looking at staking again it's only the SetUnbondingDelegations
and SetRedelegations
methods that update their indexes. Didn't see validator indexes are not updated in SetValidator
.
The idea was that I couldn't think of any reason you would want to set a CDP in the store, but not update the CDP indexes. Having this happen by accident could be bad. Therefore it seems safer to combine SetCDP and IndexCdpByCollateralRatio into one function.
x/cdp/keeper/deposit.go
Outdated
deposit.Amount = deposit.Amount.Sub(collateral) | ||
k.SetDeposit(ctx, deposit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifdeposit.Amount
is reduced down to zero, the deposit is never removed from state. Think this could be exploited to add arbitrary numbers of zero size deposits.
x/cdp/types/keys.go
Outdated
// - 0x01<collateralDenomPrefix>:<cdpID_Bytes>: CDP | ||
// - cdps are prefix by denom prefix so we can iterate over cdps of one type | ||
// - uses : as separator, otherwise the cdp with ID 11 would be selected when iterating over denom with prefix 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure separators aren't needed. If each part of a key is a known fixed length so there's no need to rely on a special :
byte to seprate them. The collateralDenomPrefix is fixed at one byte right?
Maybe just update comment, can't see any harm in leaving separators in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review changes look good. Willing to merge this into develop
now, as branch kd-update-liquidator
will be adding more functionality into the CDP module given the restructuring.
* 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
TODO