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

pallet-collection-data-feed implementation #1324

Merged
merged 16 commits into from
May 2, 2023
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Apr 19, 2023

Description

This PR adds a new pallet to organize Oracle inputs into collections in order to read them better from consumers as pallet-loans.

Epic #1279

Changes and Descriptions

  • Added Collection traits (and mocks)
  • Implements pallet-collection-data-feed
  • Mock
  • Tests

@lemunozm lemunozm added D0-ready Pull request can be merged without special precaution and notification. crcl-protocol Circle protocol related. labels Apr 19, 2023
@lemunozm lemunozm self-assigned this Apr 19, 2023
@lemunozm lemunozm mentioned this pull request Apr 19, 2023
4 tasks
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 so far. But I need some explanations to understand that properly.

  • DataId in our case would be the loans provided value for getting the price, right?
  • CollectionId in our case would be a PoolId, right?

}

/// Abstration to represent a collection of datas in memory
pub trait DataCollection<DataId, Data, Moment> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the same for interest rate right?

Would it make sense to merge them into a common collection trait?

Copy link
Contributor Author

@lemunozm lemunozm Apr 20, 2023

Choose a reason for hiding this comment

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

That would be great, but the parameters differ a little bit, the RateCollection uses two: interest_rate and normalized_debt, and in this case we only need a data_id. Maybe in a future refactor once #1310 is used instead.

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've simplified the trait to allow in the future mixing DataCollection for this and interest accrual.

pallets/collection-data-feed/src/lib.rs Show resolved Hide resolved
pallets/collection-data-feed/src/lib.rs Show resolved Hide resolved
pallets/collection-data-feed/src/lib.rs Show resolved Hide resolved
pallets/collection-data-feed/src/lib.rs Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

  • DataId in our case would be the loans provided value for getting the price, right?
  • CollectionId in our case would be a PoolId, right?
  • DataId will be the PriceId to identify an oracle price. Could match a LoanId or not. I think is more or less what you say.
  • CollectionId is a PoolId, yeah 🎸

BTW, I'm thinking in removing Moment from DataRegistry/DataCollection traits and letting the user (here, pallet-collection-data-feed) define Data as (Data, Moment) for its use case.

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

@lemunozm lemunozm merged commit a6e0717 into main May 2, 2023
11 checks passed
@lemunozm lemunozm deleted the feature/collection-data-feed branch May 2, 2023 20:00
@lemunozm lemunozm mentioned this pull request May 9, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. D0-ready Pull request can be merged without special precaution and notification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants