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

Oracle valuation implementation #1311

Merged
merged 62 commits into from May 25, 2023
Merged

Oracle valuation implementation #1311

merged 62 commits into from May 25, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Apr 11, 2023 •
edited

Description

Structural changes: diagrams

Epic #1279

Changes and Descriptions

Prerequisites

@lemunozm lemunozm added D5-breaks-api Pull request changes public api. Document changes publicly. crcl-protocol Circle protocol related. D8-migration Pull request touches storage and needs migration code. labels Apr 11, 2023
@lemunozm lemunozm self-assigned this Apr 11, 2023
@lemunozm lemunozm closed this Apr 19, 2023
@lemunozm lemunozm reopened this May 3, 2023
@lemunozm lemunozm mentioned this pull request May 9, 2023
Closed
9 tasks
lemunozm
lemunozm commented May 9, 2023
View reviewed changes
Copy link
Contributor Author

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

Critical sections commented below:

pallets/loans-ref/src/types.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/types.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/lib.rs Show resolved Hide resolved
@lemunozm lemunozm marked this pull request as ready for review May 9, 2023 14:44
mustermeiszer
mustermeiszer reviewed May 12, 2023
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.

Just a quick overview.

Given we discussed restructuring this with OnChainPricing and OffChainPriing this will need to receive changes, right? Especially, the current forwarding of the PriceId through the valuation logic, right?

pallets/loans-ref/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/collection-data-feed/src/tests.rs Outdated Show resolved Hide resolved
pallets/collection-data-feed/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/config.rs Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

Given we discussed restructuring this with OnChainPricing and OffChainPriing this will need to receive changes, right? Especially, the current forwarding of the PriceId through the valuation logic, right?

Yes, and yes! Next week I'll have this 👍🏻

@mustermeiszer
Copy link
Collaborator

@lemunozm ping me when ypu need an intermediate review

lemunozm
lemunozm commented May 23, 2023
View reviewed changes
pallets/loans-ref/src/pricing/external.rs
Comment on lines +95 to +98
ensure!(
quantity.ensure_mul(price)? == amount,
Error::<T>::AmountNotMultipleOfPrice
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@offerijns, finally, I've opted for the deterministic path on this. If we see there is a rounding issue on the UI side, we can go with the solution you've proposed.

hieronx
hieronx reviewed May 24, 2023
View reviewed changes
pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/types/mod.rs Outdated Show resolved Hide resolved
mustermeiszer
mustermeiszer reviewed May 24, 2023
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.

At least from my point of view it looks sane. I have to admit that I did not cover all code paths, but tried to follow the from my pespective critical once. Mostly repayment and borrow flow for external pricing.

pallets/loans-ref/src/config.rs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name of the file is somhow really misleading for me...

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 wanted to express that it was a utility for configuration. In this case, it's used for configuring a pallet-loans without external pricing support. Maybe util mod is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to util.

pallets/loans-ref/src/lib.rs Show resolved Hide resolved
pallets/loans-ref/src/pricing/external.rs Outdated
/// Id of an external price
pub price_id: T::PriceId,

/// Maximum number of items associated to the price id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't a PriceId be reused by multiple loans? In that case, wouldn't this rather be "Maximum number of items associated with the loan of the pricing?" or "Initial number of items associated with the loan of the pricing"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't a PriceId be reused by multiple loans?

Yes! I like the first suggestion 👍🏻

mustermeiszer
mustermeiszer reviewed May 24, 2023
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.

At least from my point of view it looks sane. I have to admit that I did not cover all code paths, but tried to follow the from my pespective critical once. Mostly repayment and borrow flow for external pricing.

@lemunozm lemunozm mentioned this pull request May 24, 2023
Closed
branan
branan approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

Big change set to review in one sitting, but it looks clean. Code paths were relatively easy to follow, and the tests seem like they're validating what needs to be validated.

Needs a rebase or merge-up, but I think we should get this in and get it out the door 👍

@lemunozm lemunozm merged commit 5781da4 into main May 25, 2023
11 checks passed
@lemunozm lemunozm deleted the feature/oracle-valuation branch May 25, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustermeiszer mustermeiszer mustermeiszer left review comments

@branan branan branan approved these changes

@hieronx hieronx hieronx approved these changes

@mikiquantum mikiquantum Awaiting requested review from mikiquantum

@NunoAlexandre NunoAlexandre Awaiting requested review from NunoAlexandre

Assignees

@lemunozm lemunozm

Labels
crcl-protocol Circle protocol related. D5-breaks-api Pull request changes public api. Document changes publicly. 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.

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