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: Enable Runtime-Apis to update nav with estimates #1791

Merged
merged 10 commits into from
Apr 3, 2024
Merged

fix: Enable Runtime-Apis to update nav with estimates #1791

merged 10 commits into from
Apr 3, 2024

Conversation

mustermeiszer
Copy link
Collaborator

@mustermeiszer mustermeiszer commented Apr 2, 2024 •
edited
Loading

Description

See slack threads

Changes and Descriptions

Allows to call the runtime apis with either the latest prices or if outdated using estimates with linear accrual based on the latest settlement price.

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

@mustermeiszer
Copy link
Collaborator Author

We can also think about removing the update_nav forever and make the calls to loans and fees more explicit. Would be easier to control which oracle values we might want to fake for some calls.

@lemunozm
Copy link
Contributor

lemunozm commented Apr 3, 2024 •
edited
Loading

I do not understand why this fixes the issue. Why using no prices for RuntimeApi would fix it?
EDIT: Ok 👍🏻

@mustermeiszer mustermeiszer marked this pull request as ready for review April 3, 2024 08:15
@mustermeiszer mustermeiszer changed the title wip: Make runtime apis work fix: Enable Runtime-Apis to update nav with estimates Apr 3, 2024
lemunozm
lemunozm reviewed Apr 3, 2024
View reviewed changes
pallets/pool-system/src/impls.rs Outdated
.then(|| {
NotedChange::<T>::remove(pool_id, change_id);
change
})
.ok_or(Error::<T>::ChangeNotReady.into())
.ok_or(Error::<T>::ChangeNotReady.into())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already notified in the consumer side of released(): https://github.com/centrifuge/centrifuge-chain/blob/main/pallets/oracle-collection/src/lib.rs#L244

Not sure if we want another event here.

Copy link
Contributor

@wischli wischli Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICS, we are not emitting such an event for pool fees or for loans. IMO, it makes sense to add this here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also the Events are quite confusing without showing that this came from a change that was released:

And if we are noting a change we are also emitting an event. I would keep it.

runtime/common/src/apis/loans.rs Outdated
@@ -32,3 +34,17 @@ decl_runtime_apis! {
fn portfolio_valuation(pool_id: PoolId, input_prices: PriceCollectionInput) -> Result<Balance, DispatchError>;
}
}

pub fn update_nav_api_call<T: pallet_loans::Config>(
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT. We're using this for the API, but it seems like a common method that can be used for other purposes. I would move to runtime/common/src/lib.rs instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same name?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion. Maybe infallible_update_nav() or similar?

wischli
wischli reviewed Apr 3, 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.

Changes seem reasonable to me, thanks for the quick fix!

@lemunozm Could you elaborate on the the difference between older_value_timestamp and last_updated entries of a CachedCollection? On Centrifuge chain, both values are the same right now. IIUC, now - older_value_timestamp needs to be less than the value_lifetime entry of the CollectionInfo, correct?

pallets/pool-system/src/impls.rs Outdated
.then(|| {
NotedChange::<T>::remove(pool_id, change_id);
change
})
.ok_or(Error::<T>::ChangeNotReady.into())
.ok_or(Error::<T>::ChangeNotReady.into())?;
Copy link
Contributor

@wischli wischli Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICS, we are not emitting such an event for pool fees or for loans. IMO, it makes sense to add this here.

@lemunozm
Copy link
Contributor

lemunozm commented Apr 3, 2024 •
edited
Loading

@lemunozm Could you elaborate on the the difference between older_value_timestamp and last_updated entries of a CachedCollection? On Centrifuge chain, both values are the same right now. IIUC, now - older_value_timestamp needs to be less than the value_lifetime entry of the CollectionInfo, correct?

Yes:

  • older_value_timestamp: contains the timestamp of the older price of the collection. That timestamp comes from when the feeder fed the value.
  • last_updated: contains the timestamp when the collection was updated (when we call update_collection()).

Note in the code that older_value_timestamp is mutable and is overwriten with older values.

@wischli wischli added I3-annoyance The code behaves as expected, but "expected" is an issue. D2-notify Pull request can be merged and notification about changes should be documented. I2-bug The code fails to follow expected behaviour. and removed I3-annoyance The code behaves as expected, but "expected" is an issue. labels Apr 3, 2024
@wischli wischli added this to the Centrifuge 1029 milestone Apr 3, 2024
lemunozm
lemunozm requested changes Apr 3, 2024
View reviewed changes
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.

Looks good! Just some lines that can be removed because they do not affect to the computation

runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/development/src/lib.rs Outdated Show resolved Hide resolved
runtime/altair/src/lib.rs Outdated Show resolved Hide resolved
runtime/altair/src/lib.rs Outdated Show resolved Hide resolved
runtime/altair/src/lib.rs Outdated Show resolved Hide resolved
@mustermeiszer mustermeiszer enabled auto-merge (squash) April 3, 2024 14:21
lemunozm
lemunozm approved these changes Apr 3, 2024
View reviewed changes
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!

Thanks for this

wischli
wischli approved these changes Apr 3, 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.

LGTM!

@mustermeiszer mustermeiszer merged commit b73e1dd into main Apr 3, 2024
10 checks passed
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

@lemunozm lemunozm lemunozm approved these changes

@cdamian cdamian Awaiting requested review from cdamian

Assignees

@mustermeiszer mustermeiszer

Labels
D2-notify Pull request can be merged and notification about changes should be documented. I2-bug The code fails to follow expected behaviour.
Projects
None yet
Milestone
Centrifuge 1029
Development

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