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 PoolFees RtApi & AccruedFees event #1728

Merged
merged 5 commits into from Feb 12, 2024
Merged

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Feb 9, 2024

Description

  • Adds Accrued event for fixed fees when they are updated (either manually during NAV update or during epoch
    closing)
Accrued {
	pool_id: T::PoolId,
	fee_id: T::FeeId,
	pending: T::Balance,
	disbursement: T::Balance,
},
  • Adds PoolFees Runtime API list_fees which simulates a PoolFees update and returns the list of pool fees divided by the bucket
fn list_fees(pool_id: PoolId) -> Option<PoolFeesList>

type PoolFeesList<...> = Vec<PoolFeesOfBucket<...>>;

pub struct PoolFeesOfBucket<FeeId, AccountId, Balance, Rate> {
	/// The corresponding pool fee bucket
	pub bucket: PoolFeeBucket,
	/// The list of active fees for the bucket
	pub fees: Vec<PoolFee<AccountId, FeeId, PoolFeeAmounts<Balance, Rate>>>,
}
  • Adds Pool Fees update to execution of previous epoch

Runtime API

Apps needs to add type decorations for the new PoolFeesApi similar to the existing ones.

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 Feb 9, 2024
@wischli wischli self-assigned this Feb 9, 2024
lemunozm
lemunozm previously approved these changes Feb 9, 2024
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.

Clean and straignforward!

Just a minor question and NIT

pallets/pool-fees/src/lib.rs Outdated Show resolved Hide resolved
@@ -2241,6 +2241,15 @@ impl_runtime_apis! {
}
}

// PoolFeesApi
impl runtime_common::apis::PoolFeesApi<Block, PoolId, PoolFeeId, AccountId, Balance, Rate> for Runtime {
fn list_fees(pool_id: PoolId) -> Option<Vec<(cfg_traits::fee::PoolFeeBucket, Vec<cfg_types::pools::PoolFee<AccountId, PoolFeeId, cfg_types::pools::PoolFeeAmounts<Balance, Rate>>>)>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT. This type is very long. Could maybe get_pool_fees() return a type which simplifies this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since clippy is complaining, I should do that yes. I dislike introducing types specifically for runtime API responses because these have to be typed manually in the JS side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know that. And with tuples, they do not need to specify any type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can get the best of both worlds with a type alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In JS, tuples are just arrays. So you have to specify them as tuples still. WDYT about 731aaa1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect!

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.

LGTM

@wischli wischli enabled auto-merge (squash) February 12, 2024 10:55
@wischli wischli merged commit 7f2e1c9 into main Feb 12, 2024
9 checks passed
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.

None yet

2 participants