Skip to content

docs: ADR-4: max collateral utilization#940

Merged
robert-zaremba merged 36 commits intomainfrom
robert/adr5-max-utilization
Jul 4, 2022
Merged

docs: ADR-4: max collateral utilization#940
robert-zaremba merged 36 commits intomainfrom
robert/adr5-max-utilization

Conversation

@robert-zaremba
Copy link
Copy Markdown
Contributor

@robert-zaremba robert-zaremba commented May 23, 2022

Description

closes: #926

  • adding max collateral utilization spec
  • cleaning the structure and language of other ADRs (done it as I was reviewing places which are impacted by max collateral utilization)

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

  • included the correct type prefix in the PR title
  • added appropriate labels to the PR
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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 ! in the type prefix if API or client breaking change
  • 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)

toteki and others added 4 commits May 24, 2022 12:51
Co-authored-by: toteki <63419657+toteki@users.noreply.github.com>
Co-authored-by: toteki <63419657+toteki@users.noreply.github.com>
@robert-zaremba robert-zaremba marked this pull request as ready for review May 25, 2022 15:47
@robert-zaremba robert-zaremba requested a review from a team as a code owner May 25, 2022 15:47
@robert-zaremba robert-zaremba requested a review from brentxu May 25, 2022 15:47
Copy link
Copy Markdown
Contributor

@toteki toteki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a naming issue between Collateral Utilization and Borrow Utilization.

Imagine 3 users:

  • Alice lends 100 ATOM, does not enable atom as collateral
  • Bob lends 200 ATOM, enables atom as collateral
  • Charlie borrows 100 ATOM

In this case, borrow utilization is 100/300. (Equal to the amounts borrowed / lent)

Would collateral utilization be the same? Or would it be 100/200 (equal to borrowed / collateral)?

If we desire the first option, we should stick to the existing concept Borrow Utilization.

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jun 29, 2022

I think it might be more intuitive to define utilization as a percent (98 collateral / 100 supply = 0.98)

than a 1/N (98 collateral / 2 non-collateral = 49.0).

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

I think it might be more intuitive to define utilization as a percent (98 collateral / 100 supply = 0.98)
than a 1/N (98 collateral / 2 non-collateral = 49.0).

We want to know how much more there is collateral over available supply. Simple supply doesn't take into account what's available (how much was borrowed). So it won't give us what we want.

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 1, 2022

In

collateral_utilization(tokenA) = total_collateral(tokenA) / available_supply(tokenA)

Does that mean available_supply(tokenA) = ModuleBalance(tokenA) - Reserved(tokenA)?


Edit:

Also total_collateral(tokenA) doesn't exist - rather total_collateral(UtokenA) does, so we would have to multiply by uToken exchange rate.


If the above is accurate, I can start looking over the math again.

Copy link
Copy Markdown
Member

@brentxu brentxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

@toteki available supply is defined, see adr3.
Yes, total_collateral(tokenA) will use uToken balance under the hood.

Copy link
Copy Markdown
Contributor

@toteki toteki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that ADR-004 was modified to use change interest rate to depend on collateral utilization. This needs to be reverted.

Dynamic interest rates work on the ratio of supply vs borrow - they need to increase when borrows are high, even when the Token is disabled as collateral.

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

This is intended. If we block new borrows based on collateral_utilization_ (the model presented in this ADR update) rather supply_utilization, then we should update the interest rate algorithm. In fact we can use both: select the max of two indices.

BTW, base on the definition, the current model is base on supply_utilization.

@robert-zaremba robert-zaremba requested a review from toteki July 3, 2022 19:19
@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 3, 2022

If proposing a change to interest rate algorithm, please separate it into a different PR.

Renaming to supply_utilization is fine of course, but switching interest rate to collateral utilization (which is what we'd get if we merged now) has not been discussed with anyone besides us and doesn't work for non-collateral assets anyway.

Also for future design PRs: It is possible to make a new ADR document rather than adding to old ones when modifying a feature - see how ADR-008 changed how borrows are stored, which was first defined in ADR-003. This allowed for a cleaner review process and reduced individual document size.

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 3, 2022

To reduce the diff size here, I will try extracting the borrow_utilization -> supply_utilization renames into a chore PR.

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

I don't see a reason of splitting this PR. We could argue that some renames can go into other PRs, but most of the changes are logically connected:

  1. borrow_utilization -> supply_utilization rename in this PR comes from the inspection of the current algorithm and definitions updates, which are correlated to the other logical changes: the meaning of utilization and the new definition of borrow limits.
  2. interest rate algorithm is not clearly defined and was never defined in this PR. Today, we refer to an external source. We can refer to a code or add more specifics in a following up PR.
  3. This PR updates definitions and requirements for the borrowing mechanism and adds few minor cosmetic changes related to the updated paragraphs. Again - rolling back the minor changes (which are in fact connected to this PR) and moving to separate PR is additional work which doesn't bring any benefit, and adds work-time to many people in the team.

Let's focus on logical changes, unless something is really not related to this PR. We have lot of work to do.

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 4, 2022

I don't see a reason of splitting this PR.

Our previous design PRs have only modified the .md file until they are agreed upon. As a result, they didn't block changes to .proto and .go files in other branches while they were debated.

A source of frustration for me is that a bunch of things have been combined in this PR

  • supply_utilization rename, which I approve and affects a bunch of files
  • collateral_utilization definition, which looks good to merge and is the main feature
  • cosmetic changes to other ADRs, which would be good to merge on their own but currently block changes to other ADRs in different branches
  • interest rate changes, which I currently disagree with and are blocking this PR until they can be discussed more

I know the PR splitting creates merge conflicts and extra work for you, but I want to unravel this before it keeps growing, or at least confine it to .md files while it's under discussion.

@robert-zaremba
Copy link
Copy Markdown
Contributor Author

robert-zaremba commented Jul 4, 2022

Like I said, I want to be efficient. All changes in this PR are related to the major problem I outlined 2 months ago.

I can move interest changes to other PR, but don't want to debate about cosmetic changes and double the work.

Copy link
Copy Markdown
Contributor

@toteki toteki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the rest of this is good to merge

@robert-zaremba robert-zaremba merged commit 1401e8d into main Jul 4, 2022
@robert-zaremba robert-zaremba deleted the robert/adr5-max-utilization branch July 4, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update ADRs for borrow utilization guard

4 participants