Oracle V2 implementation #1629

lemunozm · 2023-11-28T08:26:36Z

Description

Main new requirements:

Different feeders for different prices in different collections.
Feeders are permissionless, and the admin chooses the feeders for a pool.
The collection is updated under demand.
We prioritize low PoV against DB rate access.

See the specs here

Changes and Descriptions

This PR not:

Attach pallet-loans logic to these new pallets
Remove old oracle infrastructure
Modify oracle integration tests
Perform any migration

lemunozm

Highlighting the most important things:

pallets/oracle-data-collection/src/lib.rs

pallets/oracle-feed/src/lib.rs

pallets/oracle-data-collection/src/lib.rs

mustermeiszer

First round. Sorry for stalling. Looks goood, but need to look at the details again.

runtime/centrifuge/src/lib.rs

pallets/oracle-feed/src/lib.rs

lemunozm · 2023-12-15T11:12:26Z

@mustermeiszer here is the root support: 29bfc7d

I've needed to propagate that to pallet-oracle-data-collection to be able to fetch fed value by root

pallets/oracle-feed/src/lib.rs

lemunozm · 2023-12-19T06:50:27Z

@mustermeiszer when you say Origin, do you mean something like:

#[pallet::origin]
pub type Origin = FeederOrigin;

?

lemunozm · 2023-12-19T06:58:36Z

Ok, I think we do not want that, just reuse the current RuntimeOrigin.

wischli

First round with a lot of nits and some questions from my side. Feel free to ignore all the nits. Looks really good!

pallets/oracle-data-collection/src/benchmarking.rs

pallets/oracle-data-collection/src/lib.rs

pallets/oracle-feed/src/tests.rs

runtime/altair/src/lib.rs

wischli · 2023-12-19T08:40:00Z

runtime/altair/src/lib.rs

+impl pallet_oracle_feed::Config for Runtime {
+	type FirstValuePayFee = FeeToTreasury<Fees, FirstValueFee>;
+	type OracleKey = OracleKey;
+	type OracleValue = Quantity;


Is it really Quantity? I would have imagined Rate for loans or Ratio for the orderbook

Rate sure not because it has 27 decimals with a few integer parts. It could be a Ratio which is exactly the same type as Quantity (both alias the same exact type). We should probably unify these types to avoid confusion.

Regarding the name, I think you're right, and Ratio fits better to specify a "price"

@lemunozm can you make this an enum already? I think we will have non ratio types in the future. WDYT?

Make sense to avoid future migrations

@mustermeiszer thinking about this. Do you think it would be interesting to have different pallet instances for pallet-oracle-feed in case of different values instead of an OracleValue enum?

I say this to avoid, for example, a feeder feeding with a Key as Isin and a Value as a non-ratio type, mixing keys with non-related values.

After another thought, definitely I think it's better to use different pallet instances to maintain always well-formed key-value correlations on-chain. If some consumer needs different value types, we can always compound them into an enum using a runtime type that implements ValueProvider in the runtime. Similar to what we already do with OracleConverterBridge.

I am really undecided here.

PRO:

strong typed relationship

cleaner from a technical perspective

CON:

We have 70 pallets right now, the limit is 255 - not really close, but we are already having more pallets than most parachains IIRC

External integrations - It is just harder to look at the right place but I might over-value that

I have even thought about allowing this API

pub fn feed( lemunozm marked this conversation as resolved. origin: OriginFor<T>, key: BoundedVec<T::MaxScaleSizeKey>, value: BoundedVec<T::MaxScaleSizeValue>, ) -> DispatchResultWithPostInfo {

and upon fetching/aggregating we decode into the needed type and fail if it is not correct. That would be the most flexible.

For example with currently we have needs for

ISIN -> Price

(Currency, Currency) -> Price

Should we really have an instance of this pallet for each combination that we need on-chain? There will be more, I can promise that ^^

🤔

Should we really have an instance of this pallet for each combination that we need on-chain?

I think both relations above can be in the same pallet instance. The pallet instance can have the Key as an enum but the Value as Price. That way, any Key enum variant will map always to a correct Value type.

The number of instances will depend only on the number of different kinds of values, not keys. Mostly, I can see Price, Balance, and maybe some id or similar (?).

The API with encoding values seems pretty scary to me 😅

WDYT?

The encoding is always ensured when fetching the keys. Basically anywas what we are already doing when accceotinbg an extrinsic, but just during a runtime call. It is then an invalid key.

But lets go with instances if you think it is better. Should we already make the current key then an enum?

Should we already make the current key then an enum?

It currently is!

runtime/altair/src/lib.rs

runtime/common/src/changes.rs

lemunozm · 2023-12-19T09:57:04Z

Thanks for your deep review @wischli, I'll go into detail with your comments once I give the support for RuntimeOrigin to the PR

lemunozm

@mustermeiszer latest changes for the RuntimeOrigin support: 72f3e09

runtime/common/src/oracle.rs

wischli

Thanks for adhering to many of my nits and arguing against the other. AFAICS, the last open discussion is about instantiation vs. OracleValue enum. As I am voting for the former, I am approving this PR already. We must not merge before Frederik's approval though.

pallets/oracle-feed/src/lib.rs

wischli · 2023-12-20T14:25:36Z

runtime/altair/src/lib.rs

+impl pallet_oracle_feed::Config for Runtime {
+	type FirstValuePayFee = FeeToTreasury<Fees, FirstValueFee>;
+	type OracleKey = OracleKey;
+	type OracleValue = Quantity;


I think instantiating the pallet-oracle-feed makes more sense for the mentioned reason. That can be done in a follow up PR. But @mustermeiszer should have the stronger say here.

mustermeiszer

Look great, just need to decide on the instance vs. enum vs. generic scale for key-value in oracle.

mustermeiszer · 2023-12-21T08:52:06Z

runtime/altair/src/lib.rs

+impl pallet_oracle_feed::Config for Runtime {
+	type FirstValuePayFee = FeeToTreasury<Fees, FirstValueFee>;
+	type OracleKey = OracleKey;
+	type OracleValue = Quantity;


I am really undecided here.

PRO:

strong typed relationship

cleaner from a technical perspective

CON:

We have 70 pallets right now, the limit is 255 - not really close, but we are already having more pallets than most parachains IIRC

External integrations - It is just harder to look at the right place but I might over-value that

I have even thought about allowing this API

pub fn feed( lemunozm marked this conversation as resolved. origin: OriginFor<T>, key: BoundedVec<T::MaxScaleSizeKey>, value: BoundedVec<T::MaxScaleSizeValue>, ) -> DispatchResultWithPostInfo {

and upon fetching/aggregating we decode into the needed type and fail if it is not correct. That would be the most flexible.

For example with currently we have needs for

ISIN -> Price

(Currency, Currency) -> Price

Should we really have an instance of this pallet for each combination that we need on-chain? There will be more, I can promise that ^^

mustermeiszer · 2023-12-21T08:57:02Z

runtime/common/src/oracle.rs

+pub struct OracleConverterBridge<Origin, Provider, Pools, AssetRegistry>(
+	PhantomData<(Origin, Provider, Pools, AssetRegistry)>,
+);


Don't we also need a converter for the CachedCollection, or is it implicitly using this one?

yes it is implicitly using this one

mustermeiszer · 2023-12-21T08:58:14Z

runtime/centrifuge/src/lib.rs

+parameter_types! {
+	#[derive(Clone, PartialEq, Eq, Debug, TypeInfo, Encode, Decode, MaxEncodedLen)]
+	pub const MaxFeedersPerKey: u32 = 10;
+	pub const FirstValueFee: Fee = Fee::Balance(deposit(1, pallet_oracle_feed::util::size_of_feed::<Runtime>()));


This means 1 CFG per value?

1.65 CFG for each first value, yes. See here for context: #1629 (comment)

mustermeiszer

Thanks! All thinks sort out from my side.

lemunozm added Q5-hard Can be done by an experienced coder with a good knowledge of the codebase. P5-soon Issue should be addressed soon. D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature. labels Nov 28, 2023

lemunozm requested review from hieronx and mustermeiszer November 28, 2023 08:26

lemunozm self-assigned this Nov 28, 2023

lemunozm commented Nov 28, 2023

View reviewed changes

pallets/oracle-data-collection/src/lib.rs Outdated Show resolved Hide resolved

mustermeiszer reviewed Nov 29, 2023

View reviewed changes

This was referenced Nov 30, 2023

Fast runtime change handle by pool #1634

Closed

Modify RuntimeChange to support easily new additions #1637

Merged

lemunozm force-pushed the oracle-v2 branch from 9383f6f to a3abd60 Compare December 1, 2023 08:20

lemunozm requested review from NunoAlexandre and wischli as code owners December 1, 2023 08:20

lemunozm force-pushed the oracle-v2 branch from dae6ea7 to a1dd284 Compare December 4, 2023 11:53

lemunozm added 14 commits December 5, 2023 07:59

impl pallet-oracle-feed

83af50a

impl pallet-oracle-data-collection

6c46806

tech debt: PayFee docs

60895be

attach both pallets to development runtime

b6d0b3d

runtime change support for changing feeders

85e2a6b

add AggregationProvider

bd03704

fix clippy

02cf2a9

mock for pallet-oracle-feed

e86d845

add pallet-oracle-feed tests

9a7acfe

add mock for pallet-oracle-data-collection

8d299ee

prepare testing for pallet-oracle-data-collection

fab6f3c

add pallet-oracle-data-collection testing

eda7e3c

fix docs

d9811ec

add benchmarking for pallet_oracle_feed

83d6d28

lemunozm requested a review from mustermeiszer December 12, 2023 08:09

fix docs

3f0e856

mustermeiszer reviewed Dec 14, 2023

View reviewed changes

runtime/centrifuge/src/lib.rs Outdated Show resolved Hide resolved

pallets/oracle-feed/src/lib.rs Outdated Show resolved Hide resolved

pallets/oracle-feed/src/lib.rs Outdated Show resolved Hide resolved

add root support for feeders

29bfc7d

lemunozm added 2 commits December 15, 2023 12:15

Merge remote-tracking branch 'origin/main' into oracle-v2

d2ae695

Merge remote-tracking branch 'origin/main' into oracle-v2

e8f17b4

mustermeiszer reviewed Dec 18, 2023

View reviewed changes

pallets/oracle-feed/src/lib.rs Show resolved Hide resolved

pallets/oracle-feed/src/lib.rs Outdated Show resolved Hide resolved

pallets/oracle-feed/src/lib.rs Outdated Show resolved Hide resolved

pallets/oracle-feed/src/lib.rs Outdated Show resolved Hide resolved

wischli reviewed Dec 19, 2023

View reviewed changes

lemunozm added 2 commits December 19, 2023 13:46

Merge remote-tracking branch 'origin/main' into oracle-v2

5286d30

RuntimeOrigin as a feeder source

72f3e09

lemunozm force-pushed the oracle-v2 branch from 9daa76d to 72f3e09 Compare December 19, 2023 14:27

lemunozm commented Dec 19, 2023

View reviewed changes

runtime/common/src/oracle.rs Show resolved Hide resolved

lemunozm added 4 commits December 19, 2023 15:38

last_change_id_for out of benchmark line

0d1e178

doc Willian suggestions

f2fe532

testing events

29aabc2

deposit taken from storage size

15e36b3

lemunozm force-pushed the oracle-v2 branch from 37bb3ce to 15e36b3 Compare December 20, 2023 11:06

lemunozm added 3 commits December 20, 2023 12:30

add sp_std dependency to oracle feed

dddeac3

size deposit to all runtimes

7714e23

remove TODO comment

f5492a1

wischli approved these changes Dec 20, 2023

View reviewed changes

mustermeiszer reviewed Dec 21, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into oracle-v2

591e198

mustermeiszer approved these changes Dec 21, 2023

View reviewed changes

lemunozm merged commit 1552ccf into main Dec 21, 2023
9 checks passed

lemunozm deleted the oracle-v2 branch December 21, 2023 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oracle V2 implementation #1629

Oracle V2 implementation #1629

lemunozm commented Nov 28, 2023 •

edited

lemunozm left a comment

mustermeiszer left a comment

lemunozm commented Dec 15, 2023 •

edited

lemunozm commented Dec 19, 2023 •

edited

lemunozm commented Dec 19, 2023

wischli left a comment

wischli Dec 19, 2023

lemunozm Dec 19, 2023 •

edited

mustermeiszer Dec 19, 2023

lemunozm Dec 19, 2023

lemunozm Dec 20, 2023 •

edited

lemunozm Dec 21, 2023

mustermeiszer Dec 21, 2023 •

edited

lemunozm Dec 21, 2023 •

edited

mustermeiszer Dec 21, 2023

lemunozm Dec 21, 2023

lemunozm commented Dec 19, 2023

lemunozm left a comment

wischli left a comment

wischli Dec 20, 2023

mustermeiszer left a comment

mustermeiszer Dec 21, 2023 •

edited

mustermeiszer Dec 21, 2023

lemunozm Dec 21, 2023

mustermeiszer Dec 21, 2023

wischli Dec 21, 2023

mustermeiszer left a comment

Oracle V2 implementation #1629

Oracle V2 implementation #1629

Conversation

lemunozm commented Nov 28, 2023 • edited

Description

Changes and Descriptions

This PR not:

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Dec 15, 2023 • edited

lemunozm commented Dec 19, 2023 • edited

lemunozm commented Dec 19, 2023

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Dec 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Dec 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer Dec 21, 2023 • edited

Choose a reason for hiding this comment

lemunozm Dec 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Dec 19, 2023

lemunozm left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer Dec 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Nov 28, 2023 •

edited

lemunozm commented Dec 15, 2023 •

edited

lemunozm commented Dec 19, 2023 •

edited

lemunozm Dec 19, 2023 •

edited

lemunozm Dec 20, 2023 •

edited

mustermeiszer Dec 21, 2023 •

edited

lemunozm Dec 21, 2023 •

edited

mustermeiszer Dec 21, 2023 •

edited