Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| string interest_scalar = 4 [ | ||
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| string interest_accrued = 5 [ | ||
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; |
There was a problem hiding this comment.
I recommend adding comments to clarify these fields
For examples: to indicate if amout is before or after scalar is applied; if scalar stored here is the value of the scalar at the moment the debt was isolated or some other value; and if interest accrued is part of the amount field, separate from it, and before or after scalar.
| // GetBorrowerBorrowsWithoutInterest returns an sdk.Coins object containing all open borrows | ||
| // associated with an address. | ||
| func (k Keeper) GetBorrowerBorrowsWithoutInterest( | ||
| ctx sdk.Context, borrowerAddr sdk.AccAddress, | ||
| ) []types.BorrowWithInterest { | ||
| prefix := types.KeyAdjustedBorrowNoDenom(borrowerAddr) | ||
| totalBorrowed := []types.BorrowWithInterest{} | ||
| iterator := func(key, val []byte) error { | ||
| borrowDenom := types.DenomFromKeyWithAddress(key, types.KeyPrefixAdjustedBorrow) | ||
| var adjustedAmount sdk.Dec | ||
| if err := adjustedAmount.Unmarshal(val); err != nil { | ||
| // improperly marshaled borrow amount should never happen | ||
| return err | ||
| } | ||
|
|
||
| interestScalar := k.getInterestScalar(ctx, borrowDenom) | ||
| // interestScalar always >=1 | ||
| interestAccured := adjustedAmount.Mul(interestScalar.Sub(sdk.OneDec())).Ceil().TruncateInt() | ||
| b := types.NewBorrowWithInterest(borrowDenom, adjustedAmount.Ceil().TruncateInt(), interestScalar, interestAccured) | ||
| totalBorrowed = append(totalBorrowed, b) | ||
| return nil | ||
| } | ||
| util.Panic(k.iterate(ctx, prefix, iterator)) | ||
| return totalBorrowed | ||
| } |
There was a problem hiding this comment.
This function is where the danger is.
For context, InterestScalar for a given denom starts at 1.0 the first time that denom is registered with the leverage module, and then begins increasing as borrowers accrue interest. New borrows created after that do not start at InterestScalar=1.0 but at a higher value, even before they accrue interest.
Therefore, this function incorrectly separates principal and interest.
Example scenario:
On
Feb 1, we register denomCoinAwith the leverage module
- At that moment,
InterestScalar(CoinA) = 1.0Unspecified borrowing and lending activity happens between
Feb 1 and Jul 31
- Due to borrow interest,
InterestScalar(CoinA) = 1.2nowOn
Aug 1,borrower Zborrows180 CoinA
- This is happening at a time when
InterestScalar(CoinA) = 1.2- At this instant, the leverage module stores
adjustedAmount=150because 150*1.2 =180 CoinAUnspecified borrowing and lending activity happens between
Aug 1 and Dec 31
- Due to borrow interest,
InterestScalar(CoinA) = 1.4now- The leverage module still stores
adjustedAmount=150assumingborrower Zdid nothing else- Therefore the current debt owed is 150*1.4 =
210 CoinA. Principal is 180 and interest is 30.If we applied this
GetBorrowerBorrowsWithoutInteresttoborrower ZonDec 31it would incorrectly state that principal is 150 and interest is 60.
toteki
left a comment
toteki
left a comment
There was a problem hiding this comment.
See the problem with interest scalar.
We would need to crawl the chain with an archive node to separate interest from principal accurately. A migration couldn't do that.
We should clarify that what we're storing here is a snapshot of the bad debts with the current interest scalar so it stops increasing for them. The real values of (principal,interest) will be (snapshotPrincipal+X,snapshotInterest-X) for some X >= 0 for each borrower.
2 participants
Description
closes: #2795
moving bad debts to isolate bad debts
repay-isolated-bad-debtisolated-bad-debts,last-interest-time,interest-scalarsAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...