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 sync cdp interest method #737

Merged
merged 17 commits into from
Dec 17, 2020
Merged

Add sync cdp interest method #737

merged 17 commits into from
Dec 17, 2020

Conversation

karzak
Copy link
Member

@karzak karzak commented Dec 9, 2020

Ready for initial review

  • adds SynchronizeInterest method that gets called whenever a user interacts with a CDP
  • adds CalculateNewInterest method that can be called by clients to calculate how much interest has accrued since SynchronizeInterest was last called on a CDP.

Open questions:

  • Because CDP fees are calculated on all principal in the begin blocker, there will be rounding errors between the sum of all CDP debt and the debt in the system. Are there any methods that could break because of this?

Base automatically changed from kd-add-cdp-accumulator-methods to cdp-accumulators December 10, 2020 00:31
Copy link
Contributor

@denalimarsh denalimarsh left a comment

Choose a reason for hiding this comment

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

LGTM. Functions DepositCollateral/WithdrawCollateral could use some comments to make the justification behind the current ordering a bit clearer.

x/cdp/keeper/cdp.go Outdated Show resolved Hide resolved
x/cdp/keeper/interest.go Show resolved Hide resolved
x/cdp/keeper/interest.go Show resolved Hide resolved
Copy link
Member

@rhuairahrighairidh rhuairahrighairidh left a comment

Choose a reason for hiding this comment

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

Looks good, left some questions. I'm still looking into the rounding a bit more.

InitGenesis and LiquidateCdps look they need updating for the new interest format. Are these for a follow up PR?

nit: In keep/interest.go in AccumulateInterest line 25, the time elapsed is calculated by subtracting unix timestamps. Looks like .Unix() truncates times rather than rounding so the elapsed time could be off by up to a second. https://play.golang.org/p/C0saCKvG3A_0 Could subtract the times first then round to second amount.

x/cdp/keeper/interest.go Outdated Show resolved Hide resolved
x/cdp/keeper/interest.go Outdated Show resolved Hide resolved
x/cdp/keeper/interest_test.go Outdated Show resolved Hide resolved
x/cdp/keeper/interest_test.go Outdated Show resolved Hide resolved
@rhuairahrighairidh
Copy link
Member

rhuairahrighairidh commented Dec 15, 2020

FYI looks like sdk.RPow is very slightly inaccurate.
In interest_test.go, TestCalculateInterestFactor, test "1 year: high interest rate": the calculated rate is different from what wolfram alpha gives https://www.wolframalpha.com/input/?i=%281000000055555%2F1000000000000%29%5E31536000

test result: 5.766022095987868825
  wa result: 5.766022096035290485
       diff: 0.000000000047422000

@karzak
Copy link
Member Author

karzak commented Dec 15, 2020

InitGenesis and LiquidateCdps look they need updating for the new interest format. Are these for a follow up PR?

Yes. InitGenesis is cleaned up in #738 and I'll address liquidations in a follow up.

Copy link
Member

@rhuairahrighairidh rhuairahrighairidh left a comment

Choose a reason for hiding this comment

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

LGTM, good catch with the index update, I missed that one

x/cdp/keeper/interest.go Outdated Show resolved Hide resolved
@karzak karzak merged commit f99ad37 into cdp-accumulators Dec 17, 2020
@karzak karzak deleted the kd-sync-cdp-interest branch December 17, 2020 19:36
karzak added a commit that referenced this pull request Jan 18, 2021
* Add 'InterestFactor' to CDP type (#734)

* update cdp type to include interest factor

* fix build

* Add cdp accumulator methods (#735)

* remame fees to interest

* add accumulate interest method

* add basic test

* add note

* address review comments

* update tests

* Add sync cdp interest method (#737)

* remame fees to interest

* add accumulate interest method

* add basic test

* add note

* address review comments

* update tests

* remove old fee functions

* add method to synchronize cdp interest

* add multi-cdp tests

* add test with many blocks

* add test for interest getter

* address review comments

* calculate time difference then convert to seconds

* fix: update collateral index when syncing interest

* fix: differentiate between case when apy is zero and all fees are being rounded to zero

* fix: round time difference properly

* update cdp genesis state and migrations (#738)

* remame fees to interest

* add accumulate interest method

* add basic test

* add note

* address review comments

* update tests

* remove old fee functions

* add method to synchronize cdp interest

* add multi-cdp tests

* add test with many blocks

* add test for interest getter

* update cdp genesis state and migrations

* address review comments

* calculate time difference then convert to seconds

* fix: update collateral index when syncing interest

* fix: differentiate between case when apy is zero and all fees are being rounded to zero

* fix: simplify add/remove/update collateral index

* update genesis state to include total principal amounts

* update migration

* Delete kava-4-cdp-state-block-500000.json

* Add cdp liquidations by external keeper (#750)

* feat: split liquidations between external keepers and automated begin blocker

* address review comments

* USDX incentive accumulators (#752)

* feat: split liquidations between external keepers and automated begin blocker

* wip: refactor usdx minting incentives to use accumulators/hooks

* wip: refactor usdx minting claim object

* feat: use accumulators/hooks for usdx minting rewards

* fix: get tests passing

* fix: don't create claim objects unless that cdp type is eligable for rewards

* add begin blocker

* update client

* cleanup comments/tests

* update querier

* address review comments

* fix: check for division by zero

* address review comments

* run hook before interest is synced

* Remove savings rate (#764)

* remove savings rate

* remove savings rate from debt param

* update migrations

* address review comments

* Add usdx incentives calculation test (#765)

* add usdx incentive calculation test

* update reward calculation

* add allowable error to test criteria

* Update x/incentive/keeper/rewards_test.go

Co-authored-by: Kevin Davis <karzak@users.noreply.github.com>

* fix: remove old fields from test genesis state

Co-authored-by: Ruaridh <rhuairahrighairidh@users.noreply.github.com>

Co-authored-by: Ruaridh <rhuairahrighairidh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants