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.

Sign up for GitHub

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

Jump to bottom

fix: correct epoch execution with fees #1695

Merged
merged 5 commits into from
Jan 23, 2024
Merged

fix: correct epoch execution with fees #1695

merged 5 commits into from
Jan 23, 2024

Conversation

mustermeiszer
Copy link
Collaborator

Description

Adaption of the existing feature branch to do a correct epoch execution with the new fees.

Tasks

  • Adapt EpochExecutionInfo
  • Write migration for EpochExecutionInfo

@mustermeiszer
Copy link
Collaborator Author

Left over requirements:

  • From an UX perspective, as an issuer or public, I'd really like to see "1 week ago I charged 10k in fees to the pool. Of these 10k total, 6k, have already been executed, 4k are still pending."
    I am still a bit rusty from baking in 30C and confused by our wording, could we display the above based on the info we have?

  • Automatically create protocol-fees, when registering a pool

@wischli
Copy link
Contributor

wischli commented Jan 17, 2024

Automatically create protocol-fees, when registering a pool

This is already working on my branch. On registration, you can submit vec of PoolFeesInfo which is added without waiting the 7d Changeguard time.

@mustermeiszer
Copy link
Collaborator Author

This is already working on my branch. On registration, you can submit vec of PoolFeesInfo which is added without waiting the 7d Changeguard time

Not what I meant. I meant automatically creating a top level root fee for the protocol. With a fixed percentage that is determined by democracy

@mustermeiszer
Copy link
Collaborator Author

Must not go into this release. Can be a follow up

wischli
wischli approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! LGTM.

The one broken test rooted in the automatic epoch execution trigger during closing because of a healthy pool state. Beforehand, that was not the case. Since fees are paid during execution, assertions didn't match anymore. Fixed it by adding a senior tranche token redemption to the exisiting jr one. This way, the epoch needs to be closed manually such that all fee assertions hold true.

@mustermeiszer
Copy link
Collaborator Author

Not what I meant. I meant automatically creating a top level root fee for the protocol. With a fixed percentage that is determined by democracy

Nevermind that, we just ensure that the right fees are added when registering a pool via democracy. I think that will be easier, more flexible and fine. So not adding to force add the protoocl fee.

@wischli
Copy link
Contributor

wischli commented Jan 23, 2024

@mustermeiszer This PR is ready to be merged. I tested the migration against dev and centrifuge chain. It wasn't easy to find a block in which EpochExecution was not empty 😅

[2024-01-23T09:29:11Z INFO  runtime_common::migrations::epoch_execution] EpochExecutionMigration:  EpochExecution count pre migration is 1
[2024-01-23T09:29:11Z INFO  runtime_common::migrations::epoch_execution] EpochExecutionMigration:  EpochExecutionV2 count post migration is 1

@wischli wischli added D8-migration Pull request touches storage and needs migration code. D2-notify Pull request can be merged and notification about changes should be documented. labels Jan 23, 2024
mustermeiszer
mustermeiszer commented Jan 23, 2024
View reviewed changes
Copy link
Collaborator Author

@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.

Looks good! Can not approve but green light from my side.

@wischli wischli merged commit 276ed76 into feat/pool-fees Jan 23, 2024
@wischli wischli deleted the fix/pool-fees-epoch-execution branch January 23, 2024 09:59
@mustermeiszer mustermeiszer mentioned this pull request Jan 23, 2024
Merged
7 tasks
wischli added a commit that referenced this pull request Jan 23, 2024
@wischli @gpmayorga @mustermeiszer
feat: Add Pool Fees (#1633)
0426f76 
* 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>
wischli added a commit that referenced this pull request Jan 29, 2024
@wischli @gpmayorga @mustermeiszer
feat: Add Pool Fees (#1633)
aad505d 
* 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
Reviewers

@wischli wischli wischli approved these changes

Assignees
No one assigned
Labels
D2-notify Pull request can be merged and notification about changes should be documented. D8-migration Pull request touches storage and needs migration code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mustermeiszer @wischli