feat!: convert LiquidationIncentive from Dec to uint32 #1149

robert-zaremba · 2022-07-21T00:05:07Z

Description

Following discussion about using fixed point integers to represent small decimal values (percentage, basis points), this PR demonstrates how we can do it without adding more complexity.

See #1107 for discussion and motivation.

Author 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...

Reviewers 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...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

robert-zaremba · 2022-07-21T01:00:58Z

NOTE: I didn't update all the tests and CLI. Let's have initial review before moving it further.

robert-zaremba · 2022-07-21T01:29:54Z

Another thing to consider is the size of small decimal numbers. Is basis points too precise or per-mile will need good enough?

toteki · 2022-07-21T02:20:00Z

Percent should be okay in my opinion, but it might help to have the extra precision in case it's needed. Make sure we're capable of handling values like MaxBorrowRate: 120% for APY though if we're including those fields.

robert-zaremba · 2022-07-21T18:34:07Z

I have added a new type for package: BP to represent operations beyond "1" (10'000bp)

robert-zaremba · 2022-07-21T18:35:01Z

I will add MaxBorrowRate to this PR to cover two examples and have better taste of how this will work.

robert-zaremba · 2022-07-22T15:20:08Z

I've added MaxBorrowRate to this PR. In the keeper code there is almost no change.

RafilxTenfen · 2022-07-26T20:22:20Z

Changing everything from sdk.Dec to uint32 would be API breaking right?
Would also affect the structures on the rust side

robert-zaremba · 2022-07-26T22:39:58Z

Changing everything from sdk.Dec to uint32 would be API breaking right?

These are not parameters to the messages. Except the general gov param update, for which we don't have any additional interface yet. Token is part of the QueryRegisteredTokensResponse - so that one will indeed be the only change seeing externally.

adamewozniak

utack 👍 just has to pass e2e

toteki

Let's do these params all at once, and without the custom math types at first (so uint32 only in proto)

robert-zaremba · 2022-07-27T23:00:20Z

Let's do these params all at once, and without the custom math types at first (so uint32 only in proto)

Why? I don't see the reason why not using the custom type. Custom type gives us more control in fact and provides the essence of the solution.
Otherwise it's like using string without using sdk.Dec. Doesn't make much sense.

proto/umee/leverage/v1/leverage.proto

toteki · 2022-07-28T19:26:31Z

x/leverage/types/token.go

 )

 const (
 	// UTokenPrefix defines the uToken denomination prefix for all uToken types.
-	UTokenPrefix = "u/"
+	UTokenPrefix       = "u/"
+	maxBorrowRateLimit = bpmath.ONE * 10_000


This limit isn't necessary to hard code

toteki · 2022-07-28T19:31:45Z

x/leverage/keeper/keeper.go

@@ -432,7 +433,7 @@ func (k Keeper) LiquidateBorrow(
 	}

 	// apply liquidation incentive
-	reward.Amount = reward.Amount.ToDec().Mul(sdk.OneDec().Add(liquidationIncentive)).TruncateInt()
+	reward.Amount = bpmath.Mul(reward.Amount, liquidationIncentive).Add(reward.Amount)


This is the only use of Mul (or Quo) I could find in the current diff

Interestingly enough, the original line had to be removed in my liquidation PR #1118 , because even the small sdk.Dec early rounding was contributing to dust issues. Now liquidation incentive is multiplied to a Dec without rounding back to Int.

I think it is possible to avoid 4-precision Mul and Quo altogether

toteki · 2022-07-28T19:36:08Z

Preferences:

Convert FixedBP -> BP
See if we can avoid Mul and Quo on BP

I did the merge conflict cleanup on this, so it should be ready after that. We can merge it before #1118 if you're worried about merge conflicts.

edit: Oh, but we need to fix tests here too

toteki · 2022-07-28T19:37:31Z

I can also handle conflicts here if #1118 merges first - as far as I know it's being blocked not because it has problems but just to keep this one first.

stale · 2022-09-03T16:33:49Z