Skip to content
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

leverage: Move complex leverage calculations from keeper to separate package #1168

Open
toteki opened this issue Jul 26, 2022 · 5 comments
Open

Comments

@toteki
Copy link
Member

toteki commented Jul 26, 2022

In #1118 I tried moving parts of function ComputeLiquidation to the x/leverage/types package. The result was a more pure function which did not read or write state and was much more efficient to write tests for.

However (see discussion), types was the wrong place to put it.

Instead, a sub-package for example x/leverage/math or x/leverage/keeper/math might be created for ComputeLiquidation and other leverage computations which are sufficiently complex, especially if they

  • Can be refactored to only input / output sdk.Int and sdk.Dec with no ctx, keeper, coin, err, or address involvement.
  • Are easily separated from their keeper reads/writes
  • Require more thorough test cases (especially regarding rounding)
@robert-zaremba
Copy link
Member

We should not do that. types package should be strictly minimal, to potentially release it as a stable, independent module, containing only proto / api code. This will make it much easier to 3rd party developers to have more stable packages.
BTW, we are taking the same direction already in core Cosmos SDK.

@toteki
Copy link
Member Author

toteki commented Jul 26, 2022

Do you think taking the approach I mentioned but with a package like x/leverage/math would be worth doing?

In the meantime, I'll move back the stuff in #1118

@robert-zaremba
Copy link
Member

math is too ambiguous. Which functions would like to move? We can consider updating this task, and create a keeper sub package. Not sure what will be the best name.

@toteki
Copy link
Member Author

toteki commented Jul 26, 2022

I've moved them back to keeper in the PR now by the way -

The functions I'm interested in moving are the ones with pure math or financial logic but strictly sdk.Dec and sdk.Int inputs and outputs. For example:

  • Dynamic interest rate (the kink model)
  • Dynamic close factor (a liquidation parameter)
  • Compute liquidation (including rounding, prices, uToken exchange, and limiting factors)
  • Exchange token <-> uToken (small function, includes rounding down)
  • AdjustedBorrow <-> Borrow (includes rounding up)

There would also be internal pure mathematical functions, like the ApproxExponential and Interpolate currently seen in x/leverage/keeper/math.go

Overall, whether it ends up in its own package or not, I would like to get the module's unique math in one place, separate from the keeper's operations.

The Dec and Int inputs and outputs (without any ctx, Keeper, AccAddress or error) would allow for more efficient testing and a better isolation of concepts. In the meantime, the remaining keeper code would look cleaner, as it would focus on getting/setting/validating inputs instead of larger calculations.

@robert-zaremba
Copy link
Member

ACK. Let's think about the package name. Can you update the description of the task?

These functions are strictly related to the state machine, so I think package nested inside keeper makes sense.

@toteki toteki changed the title leverage: Move complex leverage calculations from keeper to types package leverage: Move complex leverage calculations from keeper to separate package Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants