Oracles: Move feeders to collection level #1672

lemunozm · 2024-01-04T15:59:42Z

Description

Move the feeder concept by collection instead of by key.

lemunozm · 2024-01-04T16:02:15Z

Already compiling with cargo check -p pallet-oracle-collection

lemunozm · 2024-01-05T12:37:08Z

It's now quite simple, and I would say it fits our requirements (by now).

If update_collection() is called and there is a key under use that has no associated feeder value for it, the method fails.
Only registered values are added to the collection when the collection is updated.

lemunozm · 2024-01-05T14:17:05Z

This is ready @mustermeiszer 🙌🏻

mustermeiszer · 2024-01-08T22:21:44Z

pallets/oracle-collection/src/lib.rs

 				.try_into()
 				.map_err(|_| Error::<T>::NoOracleCollectionChangeId)?;

-			Self::update_feeders(collection_id, key, feeders.clone())?;
+			CollectionInfo::<T>::insert(collection_id, info.clone());


I think we should forbid any non-allowed feeder to feed any further values at this point. Basically, iterate over all keys in the storage and remove unknown feeders.

This is a very good point

Basically, iterate over all keys in the storage and remove unknown feeders.

I would say this could be more complex because some existing keys could have lost some of their feeders (but not all), and then the aggregated values could have changed or not been validated.

Could we consider that if we call apply_update_collection_info(), then you should call later update_collection() to reflect those changes? We can understand the current collection as the values the collection had at the moment of updating. If we see this in such a way, then there is no problem if feeders are modified because the collection at that point contained such previous feeders and values.

If you agree with me, it could be interesting to add a last_updated field to the collection to know if the content of the collection reflects a moment in the past.

to add a last_updated field to the collection to know if the content of the collection reflects a moment in the past.

I thought that is already the case?

Could we consider that if we call apply_update_collection_info(), then you should call later update_collection() to reflect those changes? We can understand the current collection as the values the collection had at the moment of updating. If we see this in such a way, then there is no problem if feeders are modified because the collection at that point contained such previous feeders and values.

Yes, that is a good idea. update_collection will filter them already, right?

Maybe, apply_update_collection_info should just purge the CachedCollection storage?

I think it makes more sense to purge it 👍🏻

I thought that is already the case?

The timestamp stored in the collection is the timestamp of the older Oracle value, not the timestamp of when the collection was outdated. So now we will store both

So purge or store outdated timestamp?

I think we can use both. Store the last_updated each time we call update_collection() and purge when apply.

mustermeiszer

I have thought about the iteration of the Keys storage during this review and this is a possibly unbounded iteration.

We should use CountedStorageMap instead OR restrict register_id to not store anymore keys if MaxKeysPerCollection is reached.

lemunozm · 2024-01-09T07:22:11Z

That is a very good point! Thanks for your profound review.

I think I'll need to do it by myself in register_id, because the map is a DoubleStorageMap, and the CountedStorageNMap counts all elements. I need a counter per collection, not a counter per key in all collections.

…lection-level

mustermeiszer · 2024-01-09T09:40:30Z

pallets/oracle-collection/src/lib.rs

 				.try_into()
 				.map_err(|_| Error::<T>::NoOracleCollectionChangeId)?;

-			Self::update_feeders(collection_id, key, feeders.clone())?;
+			CollectionInfo::<T>::insert(collection_id, info.clone());


to add a last_updated field to the collection to know if the content of the collection reflects a moment in the past.

I thought that is already the case?

mustermeiszer · 2024-01-09T09:41:11Z

pallets/oracle-collection/src/lib.rs

 				.try_into()
 				.map_err(|_| Error::<T>::NoOracleCollectionChangeId)?;

-			Self::update_feeders(collection_id, key, feeders.clone())?;
+			CollectionInfo::<T>::insert(collection_id, info.clone());


Could we consider that if we call apply_update_collection_info(), then you should call later update_collection() to reflect those changes? We can understand the current collection as the values the collection had at the moment of updating. If we see this in such a way, then there is no problem if feeders are modified because the collection at that point contained such previous feeders and values.

Yes, that is a good idea. update_collection will filter them already, right?

mustermeiszer · 2024-01-09T09:41:43Z

pallets/oracle-collection/src/lib.rs

 				.try_into()
 				.map_err(|_| Error::<T>::NoOracleCollectionChangeId)?;

-			Self::update_feeders(collection_id, key, feeders.clone())?;
+			CollectionInfo::<T>::insert(collection_id, info.clone());


Maybe, apply_update_collection_info should just purge the CachedCollection storage?

pallets/oracle-collection/src/lib.rs

mustermeiszer

Thanks for your patience! Thats looks like a good iteration to the final version!

lemunozm · 2024-01-09T12:51:34Z

Thanks for all your feedback! Very appreciated and accurate! 🙌🏻

lemunozm requested review from hieronx and mustermeiszer January 4, 2024 15:59

lemunozm self-assigned this Jan 4, 2024

move feeders to collection level

dfb8aa2

lemunozm force-pushed the oracle/feeders-by-collection-level branch from ce58e28 to dfb8aa2 Compare January 5, 2024 11:16

lemunozm added 3 commits January 5, 2024 12:31

update tests

a0a9e67

fix benchmarks

14e1b9d

fix weights

c21fb69

lemunozm marked this pull request as ready for review January 5, 2024 12:40

lemunozm added 3 commits January 5, 2024 13:44

fix runtime-common

0c8b02b

modify docs

bc4e87f

fix integration-tests

c880656

wischli added this to the Centrifuge 1025 milestone Jan 8, 2024

mustermeiszer reviewed Jan 8, 2024

View reviewed changes

lemunozm added 2 commits January 9, 2024 08:38

add a key counter per collection

d4ac613

Merge remote-tracking branch 'origin/main' into oracle/feeders-by-col…

dea3c2f

…lection-level

lemunozm requested a review from mustermeiszer January 9, 2024 07:40

mustermeiszer reviewed Jan 9, 2024

View reviewed changes

lemunozm added 2 commits January 9, 2024 10:47

add last_updated to collection

32a8655

purge collection after apply

a41b1b1

lemunozm requested a review from mustermeiszer January 9, 2024 09:55

mustermeiszer approved these changes Jan 9, 2024

View reviewed changes

lemunozm merged commit 433211e into main Jan 9, 2024
9 checks passed

lemunozm deleted the oracle/feeders-by-collection-level branch January 9, 2024 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oracles: Move feeders to collection level #1672

Oracles: Move feeders to collection level #1672

lemunozm commented Jan 4, 2024 •

edited

Loading

lemunozm commented Jan 4, 2024

lemunozm commented Jan 5, 2024 •

edited

Loading

lemunozm commented Jan 5, 2024

mustermeiszer Jan 8, 2024

lemunozm Jan 9, 2024

lemunozm Jan 9, 2024

mustermeiszer Jan 9, 2024

mustermeiszer Jan 9, 2024

mustermeiszer Jan 9, 2024

lemunozm Jan 9, 2024

lemunozm Jan 9, 2024

mustermeiszer Jan 9, 2024

lemunozm Jan 9, 2024 •

edited

Loading

mustermeiszer left a comment

lemunozm commented Jan 9, 2024

mustermeiszer Jan 9, 2024

mustermeiszer Jan 9, 2024

mustermeiszer Jan 9, 2024

mustermeiszer left a comment

lemunozm commented Jan 9, 2024

Oracles: Move feeders to collection level #1672

Oracles: Move feeders to collection level #1672

Conversation

lemunozm commented Jan 4, 2024 • edited Loading

Description

lemunozm commented Jan 4, 2024

lemunozm commented Jan 5, 2024 • edited Loading

lemunozm commented Jan 5, 2024

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

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

lemunozm Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Jan 9, 2024

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 Jan 9, 2024

lemunozm commented Jan 4, 2024 •

edited

Loading

lemunozm commented Jan 5, 2024 •

edited

Loading

lemunozm Jan 9, 2024 •

edited

Loading