-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat: Add Pool Fees #1633
Conversation
There was a problem hiding this 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 😄
There was a problem hiding this 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], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Outdated
@@ -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)?, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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())?; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Outdated
@@ -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)?, |
There was a problem hiding this comment.
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)
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. |
* 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 <[email protected]>
There was a problem hiding this 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.
* 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 <[email protected]> --------- Co-authored-by: Guillermo Perez <[email protected]> Co-authored-by: Frederik Gartenmeister <[email protected]>
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 amountX
reduces the reserve byX
as well.Therefore, the epoch NAV now includes the pool reserve. During tranche rebalancing, we need to subtract the reserve. Based on this DAO Slack discussion.
TODO
PoolFeeAmount
)Checklist: