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

docs: market params design_doc #1152

Merged
merged 8 commits into from
Jul 27, 2022
Merged

docs: market params design_doc #1152

merged 8 commits into from
Jul 27, 2022

Conversation

toteki
Copy link
Member

@toteki toteki commented Jul 21, 2022

New ADR (as folder is not yet moved/renamed) absorbs some previous changes and proposes three similarly themed parameters:

  • MaxSupplyUtilization
  • MaxCollateralShare
  • MinCollateralLiquidity (alternative: MaxCollateralUtilization)

This has been split off from #1141 into a docs-only PR for process.


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 ! to the type prefix if API or client breaking change
  • added appropriate labels to the PR
  • 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 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)

Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Is there anything new in this PR?

@@ -47,69 +47,6 @@ The calculated borrow limit, which weighs collateral uTokens against borrowed as

Note also that as a consequence of uToken interest, the asset value of uToken collateral increases over time, meaning a user who repays positions in full and redeems collateral uTokens will receive back more base assets than they deposited originally, reducing the effective interest.

### Collateral utilization
Copy link
Member

Choose a reason for hiding this comment

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

Why moving this definitions?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not part of the original decision - since ADR-003 will be renamed to a design doc (and already functioned as one), this is an attempt to apply the rule "don't modify old design docs, but make a new one instead."

Since multiple parameters are being added in the same timeframe, it made sense to group the proposals.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but they are related to the borrowing as well.
Hence, if we are moving this then let's already organize it in a documentation, rather than scatter it around decision docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

What specifically should I do to this PR before merging? As it stands, it is a completed design doc representing parameters which were added together on a single theme.

Moving the collateral utilization definition back to 003 violates "don't modify old design docs" rule. Removing this document altogether and putting it in the spec goes against our decision to use design doc PRs.

As I see it, this should either be merged unchanged, or moved to the design docs folder first then merged. Spec updates are a separate issue and do not happen during the initial design PR.

docs/architecture/ADR-010-market-params.md Outdated Show resolved Hide resolved
docs/architecture/ADR-010-market-params.md Outdated Show resolved Hide resolved
@toteki
Copy link
Member Author

toteki commented Jul 22, 2022

Is there anything new in this PR?

  • Adds two new parameters MaxCollateralShare and MaxSupplyUtilization
  • Renames + redefines MaxCollateralUtilization to MinCollateralLiquidity
  • Moves the latter to a new doc with the others

@toteki
Copy link
Member Author

toteki commented Jul 25, 2022

This is the first PR that was split off from #1141, and should ideally be merged before #1159.

@robert-zaremba
Copy link
Member

  1. It would be great if we already start moving towards a nice documentation flow, and link definitions and whatever is needed into the design docs. If we desided that the design docs are going to be static (won't change after the design is settled), then we end up with scattered pieces distributed among decision discussed in desig docs.
  2. For completeness, let's already place this document in docs/design_docs

@toteki toteki changed the title docs: market params ADR docs: market params design_doc Jul 27, 2022
docs/design_docs/010-market-params.md Show resolved Hide resolved
docs/design_docs/010-market-params.md Show resolved Hide resolved
docs/design_docs/010-market-params.md Show resolved Hide resolved

This quantity has the property of increasing rapidly (as 1/N -> 0) when available supply is under stress.

Stable assets will have high max collateral utilization (can go even to 50). Volatile assets should have significantly smaller collateral utilization, and assets with high risk should have max collateral utilization close to 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Stable assets will have high max collateral utilization (can go even to 50). Volatile assets should have significantly smaller collateral utilization, and assets with high risk should have max collateral utilization close to 1.
Stable assets will have high max collateral utilization (up to to 50). Volatile assets should have significantly smaller collateral utilization, and assets with high risk should have max collateral utilization close to 1.

@toteki toteki merged commit 372a8cb into main Jul 27, 2022
@toteki toteki deleted the adam/adr10 branch July 27, 2022 16:08
@robert-zaremba
Copy link
Member

I hoped we won't merge PRs without updated documentation (spec dir)

@toteki
Copy link
Member Author

toteki commented Jul 27, 2022

I hoped we won't merge PRs without updated documentation (spec dir)

Current practice is to update spec simultaneous with implementation, so it is always exactly up to date (as opposed to containing unimplemented features).

As an example, the proto update with these parameters also added them to the Token struct in the spec.

We will likely update the descriptions (specifically failure conditions) of Msg types in the spec, in the same PR as those limits are introduced in code.

Also since spec is scheduled for overhaul (to be fleshed out to be an adequate source of truth) we can wait until v3 release candidate is out to do the large spec updates.

If the above turns out to be bad practice, we can revisit it - for now, I want to rush v3.

@robert-zaremba
Copy link
Member

Current practice is to update spec simultaneous with implementation, so it is always exactly up to date (as opposed to containing unimplemented features).

I have never seen that. The whole motivation to redo the adr process and put more pressure to the documentation was that both were and still are very outdated.

We will likely update the descriptions (specifically failure conditions) of Msg types in the spec,

We should really link it to the code / or proto file rather than copying it. This way we are sure the we will be up to date.

@robert-zaremba
Copy link
Member

True - we don't need to update the docs now, we don't have that task. But if in any current tasks we are updating something then we should already slowly construct the updated docs.

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.

3 participants