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 collateral type to cdp #629

Merged
merged 8 commits into from Aug 21, 2020
Merged

Add collateral type to cdp #629

merged 8 commits into from Aug 21, 2020

Conversation

karzak
Copy link
Member

@karzak karzak commented Aug 19, 2020

Adds a 'Type' field to CDP and 'CollateralParam` structs. This enables one denom to have two different cdp types, for example, 'busd-a' and 'busd-b'. EDIT: The reason for this is to enable a two-tiered system of cdps for stablecoins. During normal times, the debt limit for stablecoins should be low, and the interest charged should be modest - call this 'busd-a'. However, at times when a lot of liquidations are happening, it's important that auction bidders are able to mint USDX efficiently. To accommodate this, we will have a second stable coin CDP ('busd-b'), that has a much higher debt limit, but a higher interest rate. This way, auction bidders are encouraged to crate USDX using stable coins only at times of elevated liquidations. See the 'USDC-A' and 'USDC-B' vaults in maker for example.

For messages, the denom of a CDP is no longer implied by the coins that are being used as collateral. CollateralType is now a required field for cdp messages. For example,

kvcli tx cdp create 1000000000bnb 10000000usdx --from key

becomes

kvcli tx cdp create 1000000000bnb 10000000usdx bnb-a --from key

The store didn't undergo any changes, as the prefix for each collateral was used to index cdps by collateral denom, rather than the denom itself. Instead, getter methods that previously assumed each CollateralParam's 'Denom' was unique now assume each CollateralParam's 'Type' is unique. The params validation logic has been updated to reflect this change.

Feedback wanted:

  • I opted not to change the API of the current 'incentive' implementation, but its semantic meaning has changed. Previously, the Reward.Denom param referred to the denom of CDPs that rewards would be given for. Now, Reward.Denom would refer to the Type of CDPs that rewards would be given for. Would anyone argue in favor of changing the Reward.Denom field to be Reward.CollateralType or something similar?
  • Similarly, I've left CDP method names such as 'IterateCDPsByDenom` as is, although again, Denom now refers to collateral type.

Notes

  • I'll include migration logic in a follow-on PR, as these changes should be given thorough review since they are breaking changes to the core CDP logic.

@karzak karzak added R4R When a PR is ready for review cdp issues for cdp features v0.10+ labels Aug 19, 2020
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I have a few questions about the motivation of the changes:

This enables one denom to have two different cdp types, for example, 'busd-a' and 'busd-b'.

What does it mean to have different CDP types? What's the benefit of allowing for more than one type (or what was wrong with having a single one)? Should the <denom>-<suffix> be validated too for consistency (eg with a regex)?

I opted not to change the API of the current 'incentive' implementation, but its semantic meaning has changed. Previously, the Reward.Denom param referred to the denom of CDPs that rewards would be given for. Now, Reward.Denom would refer to the Type of CDPs that rewards would be given for. Would anyone argue in favor of changing the Reward.Denom field to be Reward.CollateralType or something similar?
Similarly, I've left CDP method names such as 'IterateCDPsByDenom` as is, although again, Denom now refers to collateral type.

I would vote for renaming the Denom to CollateralType, I realize that some of my comments were due to this change.

x/cdp/types/cdp.go Outdated Show resolved Hide resolved
x/incentive/types/genesis.go Show resolved Hide resolved
x/incentive/types/msg.go Show resolved Hide resolved
x/incentive/types/params.go Show resolved Hide resolved
x/incentive/types/rewards.go Show resolved Hide resolved
@rhuairahrighairidh
Copy link
Member

rhuairahrighairidh commented Aug 20, 2020

Thoughts on Type/Denom naming:
I reckon maker went with separate collateral denoms USDC-A and USDC-B because it was easiest to implement. But they're the same underlying collateral, just used in different types of CDP with different parameters.

Before, each of our CDP types had a unique collateral, so we used the denom to identify them. But now we’re using an arbitrary string to identify CDP types.

I’d vote to make a distinction between cdp type and collateral denoms.
eg
reward.Denom -> reward.CDPType
GetCdpByOwnerAndDenom -> GetCDPByOwnerAndType
CollateralParams -> CDPTypeParams (or CDPParams?)
k.GetCollateral -> k.GetCDPTypeParams
collateralType -> cdpType

x/cdp/types/keys.go Outdated Show resolved Hide resolved
@rhuairahrighairidh
Copy link
Member

Looks good. I haven't tested it out manually, but I'm thinking we'll be doing that next week as we improve our tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cdp issues for cdp features R4R When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants