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

Fix default timestamp for portfolio valuation #1359

Closed
wants to merge 1 commit into from

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented May 23, 2023

Description

The last_updated field of portfolio valuation is only updating when the valuations are computed. This is ok, nevertheless, if a loan is added to a pool, and modified, it will affect the portfolio valuation without updating the last_updated, which by default will be created with 0 instead of now()

This means that if some entity call nav(), before computing once the portfolio valuation, it will obtain an incorrect time (?). Not sure if this is a possible case or if we always call the update_portfolio_valuation() at least once.

Until I see it, the pool-system is protected when closing the epoch giving an NAVTooOld error, but that means the epoch can not be closed (unless update_porfolio_valuation() is called).

If it is a security issue, I understand we need to deliver a fix soon. In case not, and we can live with it, this will be fixed properly in the Oracle Valuation PR

@lemunozm lemunozm added I2-bug The code fails to follow expected behaviour. P9-drop-everything Everyone should address this issue now. crcl-protocol Circle protocol related. labels May 23, 2023
@lemunozm lemunozm self-assigned this May 23, 2023
Comment on lines +94 to +101
impl<T: Config> Default for PortfolioValuation<T> {
fn default() -> Self {
Self {
value: T::Balance::zero(),
last_updated: T::Time::now().as_secs(),
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer created with 0 by default

@lemunozm
Copy link
Contributor Author

@mustermeiszer @offerijns It's not clear what should be the last_updated initial value.
In the original pallet-loans implementation, it's initialized in the initialise_pool() extrinsic, but we do not have such a call. That value is defined as "last time when nav is performed for the entire pool", but what if it has never been called? Should we return None instead or the time of first borrowing in the pool?

@mustermeiszer
Copy link
Collaborator

I agree it should be the created time.

@lemunozm
Copy link
Contributor Author

The intent of this commit is to add a fast fix, but if we can wait until the next release where Oracle Valuation fixes this well is better not to merge this PR

@lemunozm
Copy link
Contributor Author

Close it because the fix is already in #1311

@lemunozm lemunozm closed this May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. I2-bug The code fails to follow expected behaviour. P9-drop-everything Everyone should address this issue now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants