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

feat: add NAV runtime API #1703

Merged
merged 3 commits into from Jan 30, 2024
Merged

feat: add NAV runtime API #1703

merged 3 commits into from Jan 30, 2024

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Jan 26, 2024 •
edited

Description

pub struct PoolNav<Balance> {
	pub nav_aum: Balance,
	pub nav_fees: Balance,
	pub reserve: Balance,
	pub total: Balance,
}

fn nav(pool_id: PoolId) -> Option<PoolNav<Balance>>;

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 self-assigned this Jan 26, 2024
@wischli wischli added D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature. labels Jan 26, 2024
wischli
wischli commented Jan 26, 2024
View reviewed changes
runtime/common/src/apis/pools.rs Outdated
@@ -46,5 +46,7 @@ decl_runtime_apis! {
fn tranche_id(pool_id: PoolId, tranche_index: TrancheIndex) -> Option<TrancheId>;

fn tranche_currency(pool_id: PoolId, tranche_loc: TrancheLoc<TrancheId>) -> Option<Currency>;

fn nav(pool_id: PoolId) -> Option<(Balance, Nav<Balance>)>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mustermeiszer Not sure if we would rather want to expose all data in a new type to be more explicit. Here we return (nav.total(pool.reserve), nav).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use a structured type for this? Just because it's quite opaque now what the data returned here is (hard to know that the first element is the reserve and the second element is the NAV)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all. I wanted to propose the most simple version and then iterate on that based on the feedback.

mustermeiszer
mustermeiszer reviewed Jan 26, 2024
View reviewed changes
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.

Looks good to me. @hieronx would the indexer be able to work with that?

lemunozm
lemunozm reviewed Jan 28, 2024
View reviewed changes
runtime/centrifuge/src/lib.rs

fn nav(pool_id: PoolId) -> Option<(Balance, pallet_pool_system::Nav<Balance>)> {
let pool = pallet_pool_system::Pool::<Runtime>::get(pool_id)?;
let nav_loans = Loans::update_nav(pool_id).ok()?;
Copy link
Contributor

@lemunozm lemunozm Jan 28, 2024 •
edited

Choose a reason for hiding this comment

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

Just to be sure about this. Do we want the NAV value used to compute the loan's portfolio, or the NAV value just for the moment we call the RuntimeAPI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure and asked @mustermeiszer who said that we want the current NAV, even if the onchain one is outdated.

Copy link
Contributor

@lemunozm lemunozm Jan 29, 2024 •
edited

Choose a reason for hiding this comment

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

Thanks for sharing the double check!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely current one, as the UI needs to have the latest data!

@wischli wischli added this to the Centrifuge 1025 milestone Jan 30, 2024
@wischli wischli marked this pull request as ready for review January 30, 2024 09:26
mustermeiszer
mustermeiszer approved these changes Jan 30, 2024
View reviewed changes
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.

LGTM!

@wischli wischli enabled auto-merge (squash) January 30, 2024 17:11
@wischli wischli merged commit 23daeeb into main Jan 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemunozm lemunozm lemunozm left review comments

@mustermeiszer mustermeiszer mustermeiszer approved these changes

@hieronx hieronx Awaiting requested review from hieronx

Assignees

@wischli wischli

Labels
D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature.
Projects
None yet
Milestone
Centrifuge 1025
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wischli @hieronx @lemunozm @mustermeiszer