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

Add CDP Spec #328

Merged
merged 24 commits into from
Jan 23, 2020
Merged

Add CDP Spec #328

merged 24 commits into from
Jan 23, 2020

Conversation

rhuairahrighairidh
Copy link
Member

@rhuairahrighairidh rhuairahrighairidh commented Jan 22, 2020

TODO

  • State
  • Msgs and associated state transistions
  • Begin Blocker
  • State transitions not from begin blocker or msgs
  • Understand how fees work and document
  • Document events

Notes:

  • The "Markdown Table Formatter" VSCode extension is useful for formatting tables for the events specs
  • This is a new branch with cherry-picked commits, old one was a git mess.

Proposal:

  • Add a broken url link checker to CI? It found a couple in the spec.

@rhuairahrighairidh rhuairahrighairidh mentioned this pull request Jan 22, 2020
6 tasks
@rhuairahrighairidh rhuairahrighairidh added the R4R When a PR is ready for review label Jan 22, 2020
Copy link
Member

@karzak karzak left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few comments regarding terms and concepts, then mostly typos/wording suggestions.

x/cdp/spec/01_concepts.md Outdated Show resolved Hide resolved
x/cdp/spec/01_concepts.md Outdated Show resolved Hide resolved

In the event of a decrease in the price of the collateral, the total value of all collateral in CDPs may drop below the value of all the issued pegged asset. This undesirable event is countered through two mechanisms.

**CDP Liquidations** Each CDP is monitored for ratio of collateral value to debt value. When this drops too low the collateral and debt is seized by the system. The collateral is sold off through an auction to bring in pegged asset which is burned against the seized debt.
Copy link
Member

Choose a reason for hiding this comment

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

Unclear use of 'pegged asset'. I think this could be confusing to outside readers. I would reccommend stable asset instead.

Listed here is the state stored by the module.
For details see the types package. In particular `keys.go` describes how state is stored in the key-value store.
<!--
Store key structures are not listed here - they seem like an implementation detail best documented by code comments.
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment. I'm fine with leaving the keys to code comments.

# State

Listed here is the state stored by the module.
For details see the types package. In particular `keys.go` describes how state is stored in the key-value store.
Copy link
Member

Choose a reason for hiding this comment

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

link to the file keys.go


## DrawDebt

DrawDebt creates debt in a CDP, minting new pegged asset which is sent to the sender.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DrawDebt creates debt in a CDP, minting new pegged asset which is sent to the sender.
DrawDebt creates debt in a CDP, minting new stable asset which is sent to the sender.

## DrawDebt

DrawDebt creates debt in a CDP, minting new pegged asset which is sent to the sender.
<!-- TODO Can the sender own have same collateral multiple CDPs? if so how do they choose between them. -->
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment. Not sure what's being asked... clarify if you still have questions.


- `Collateral` taken from depositor and sent to cdp module account
- the depositor's `Deposit` struct is updated or a new one created
- fees are updated (see below)
Copy link
Member

Choose a reason for hiding this comment

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

do we want to mention that the collateral index is updated? Same applies to other state changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be in favour of leaving it out. I'd prefer not to describe too much details

## Update Fees

- The total fees accumulated since the last block across all CDPs are calculated.
- An equal amount of debt coins are minted and sent to the system's liquidator module account.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- An equal amount of debt coins are minted and sent to the system's liquidator module account.
- An equal amount of debt coins are minted and sent to the system's cdp module account.
- An equal amount of stable asset coins are minted and sent to the system's liquidator module account


## Net Out System Debt, Re-Balance

- Burn the maximum possible equal amount of debt and pegged asset from the liquidator module account.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Burn the maximum possible equal amount of debt and pegged asset from the liquidator module account.
- Burn the maximum possible equal amount of debt and stable asset from the liquidator module account.

rhuairahrighairidh and others added 2 commits January 22, 2020 21:57
Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>
Copy link
Member

@karzak karzak left a comment

Choose a reason for hiding this comment

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

A few final suggestions, LGTM after those changes.

x/cdp/spec/06_params.md Outdated Show resolved Hide resolved
x/cdp/spec/README.md Outdated Show resolved Hide resolved
x/cdp/spec/README.md Outdated Show resolved Hide resolved
rhuairahrighairidh and others added 2 commits January 23, 2020 18:28
Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>
Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>
Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>
@karzak karzak merged commit e11b298 into develop Jan 23, 2020
@rhuairahrighairidh rhuairahrighairidh deleted the ro-add-cdp-module-spec branch January 24, 2020 00:41
denalimarsh pushed a commit that referenced this pull request Apr 3, 2020
* add overview and basic structure

* add state and params

* add basic messages

* add state transitions

* add begin block state transitions

* add missing titles

* add concepts

* add events

* update state and concepts

* update for liquidator changes

* update events

* mention module accounts

* update begin block

* update params

* update page numbering

* add fee descriptions

* add broken link linter

* add broken link linter to CI

* move link check to end of CI

* update typo

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

* address review comments

* Update x/cdp/spec/06_params.md

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

* Update x/cdp/spec/README.md

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

* Update x/cdp/spec/README.md

Co-Authored-By: Kevin Davis <karzak@users.noreply.github.com>

Co-authored-by: Kevin Davis <karzak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R4R When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants