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

feat: Add Pool Fees #1633

Merged
merged 67 commits into from
Jan 23, 2024
Merged

feat: Add Pool Fees #1633

merged 67 commits into from
Jan 23, 2024

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Nov 29, 2023

Description

Changes and Descriptions

NAV

This PR adds a "negative" NAV part to the existing "positive" part from Loans which is renamed to AssetsUnderManagement (AUM). The PoolFees NAV is the sum of pending pool fees. It does not include amounts which can be paid during epoch execution (calculated at epoch closing) because any disbursement of amount X reduces the reserve by X as well.

- NAV = NAV(Loans)
+ NAV = min(PoolReserve + NAV(Loans) - NAV(PoolFees), 0)

Therefore, the epoch NAV now includes the pool reserve. During tranche rebalancing, we need to subtract the reserve. Based on this DAO Slack discussion.

  • Changes calculation and definition of the NAV:

TODO

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli added D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature. labels Nov 29, 2023
@wischli wischli self-assigned this Nov 29, 2023
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Question: Does this pallet overwrite pallet-fees?

I left some non-asked comments over a fast review to catch up last changes for myself 😄

pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
pallets/pool-system/src/lib.rs Outdated Show resolved Hide resolved
libs/types/src/fixed_point.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Need to go into more detail in the epoch closing part. Otherwise, nothing big from my side.

@@ -58,6 +66,9 @@ impl<T: Changeable, Options: Clone> RuntimeChange<T, Options> {
RuntimeChange::OracleCollection(change) => match change {
OracleCollectionChange::Feeders(_, _) => vec![],
},
RuntimeChange::PoolFee(pool_fees_change) => match pool_fees_change {
PoolFeesChange::AppendFee(_, _) => vec![week],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the requirement for week come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's arbitrarily taken and matching the other change guard timelines. What would be your proposal? I have an open TODO to decide on this.

pallets/pool-system/src/lib.rs Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Show resolved Hide resolved
@@ -1178,7 +1239,7 @@ pub mod pallet {
pool.tranches.rebalance_tranches(
T::Time::now(),
pool.reserve.total,
epoch.nav,
epoch.nav.ensure_sub(epoch.reserve)?,
Copy link
Contributor Author

@wischli wischli Jan 11, 2024

Choose a reason for hiding this comment

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

NOTE: Before (red), now (green)

- epoch.nav = NAV(Loans)
+ epoch.nav = epoch.reserve + NAV(Loans) - NAV(PoolFees)

Copy link
Collaborator

Choose a reason for hiding this comment

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

CR
epoch.nav.ensure_sub(pool.reserve.total)

let tranches = build_update_tranches::<T>(n);
prepare_asset_registry::<T>();
create_pool::<T>(n, admin.clone())?;
create_pool::<T>(n, m, admin.clone())?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: The number of pool fees impacts the POV size but not the weight of all pool-registry calls except for the pool registration.

@wischli wischli marked this pull request as ready for review January 11, 2024 11:17
@wischli wischli requested a review from hieronx as a code owner January 11, 2024 11:17
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

I have 2 change requests. Otherwise, the code looks super good! Some nits, but overall ready from a logical perspective.

pallets/pool-system/src/lib.rs Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
libs/types/src/pools.rs Outdated Show resolved Hide resolved
pallets/pool-system/src/lib.rs Show resolved Hide resolved
pallets/pool-system/src/lib.rs Outdated Show resolved Hide resolved
@@ -1178,7 +1239,7 @@ pub mod pallet {
pool.tranches.rebalance_tranches(
T::Time::now(),
pool.reserve.total,
epoch.nav,
epoch.nav.ensure_sub(epoch.reserve)?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

CR
epoch.nav.ensure_sub(pool.reserve.total)

pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
pallets/pool-fees/src/lib.rs Show resolved Hide resolved
@mustermeiszer
Copy link
Collaborator

mustermeiszer commented Jan 23, 2024

We need to do a RU in dev/demp after merging this and inform apps beforehand, so that they can work on the UI for this.

mustermeiszer and others added 2 commits January 23, 2024 10:59
* fix: correct epoch execution with fees

* refactor: use new nav syntax

* tests: fix auto epoch closing

* feat: epoch execution migration

* chore: add epoch migration to altair

---------

Co-authored-by: William Freudenberger <w.freude@icloud.com>
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Good to go and to test in dev/demo.

@wischli wischli merged commit 0426f76 into main Jan 23, 2024
9 checks passed
@wischli wischli deleted the feat/pool-fees branch January 23, 2024 14:35
@wischli wischli mentioned this pull request Jan 26, 2024
4 tasks
wischli added a commit that referenced this pull request Jan 29, 2024
* chore: init blank pool fees pallet

* chore: apply workspace #1609 to pool-fees dummy

* fix: docker-compose

* feat: add changeguard pool fees support

* wip: prepare payment logic

* refactor: use reserve instead of nav

* chore: add extrinsics

* feat: prep + pay disbursements

* refactor: Apply fees ChangeGuard to #1637

* feat: add pool fees to all runtimes

* feat: add fees pool registration

* fix: existing pool unit tests

* fix: existing integration tests

* docs: add pool fee types

* wip: init fees unit tests

* tests: extrinsics wip

* chore: add events

* tests: add pool fees unit tests

* fix: support retroactive disbursements

* refactor: add epoch transition hook

* refactor: add pool fee prefix to types

* refactor: remove redundand trait bounds

* wip: pool system integration tests

* refactor: move portfolio valuation from loans to cfg-types

* chore: add pool fee account id

* wip: pool fee nav

* wip: fix uts

* wip: fix apply review by @lemunozm

* fix: issues after rebase

* tests: add saturated_proration

* refactor: simplify pool fee amounts

* chore: aum + fix fees UTs

* chore: apply AUM to pool-system

* fix: remove AUM coupling in PoolFees

* fix: transfer on close, unit tests

* fix: use total nav

* fix: taplo

* fix: fee calc on nav closing

* feat: impl TimeAsSecs for timestamp mock

* fix: test on_closing instead of update_active_fees

* fix: clippy

* tests: fix + add missing pool fees

* refactor: make update fees result instead of void

* tests: add insufficient resource in p-system

* bench: add pool fees, apply to system + registry

* fix: tests

* refactor: explicitly use Seconds in FeeAmountProration impl

* docs: add PoolFeeAmount and NAV update

* refactor: update NAV, total assets after review from @mustermeiszer

* fix: clippy

* refactor: Add PoolFeePayable

* fix: clippy

* fix: correct epoch execution with fees (#1695)

* fix: correct epoch execution with fees

* refactor: use new nav syntax

* tests: fix auto epoch closing

* feat: epoch execution migration

* chore: add epoch migration to altair

---------

Co-authored-by: William Freudenberger <w.freude@icloud.com>

---------

Co-authored-by: Guillermo Perez <gpmayorga@users.noreply.github.com>
Co-authored-by: Frederik Gartenmeister <mustermeiszer@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol fees
4 participants