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

Oracles: Time validation by a max age threshold #1670

Merged
merged 10 commits into from
Jan 3, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jan 2, 2024

Description

We want, as a requirement, to have oracle values validated with a "max age" threshold. Older oracles values must be considered outdated and it's an error to use them.

This error is generated when:

  • Getting a collection where one oracle is outdated.
  • Getting just an oracle value outdated.
  • Updating a collection when an oracle value is outdated.

The threshold is optionally set by the collection by the admin

Tasks

  • Implementation
  • Testing
  • Benchmarks
  • Updated runtimes

@lemunozm lemunozm added D0-ready Pull request can be merged without special precaution and notification. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. P7-asap Issue should be addressed in the next days. I8-enhancement An additional feature. labels Jan 2, 2024
@lemunozm lemunozm self-assigned this Jan 2, 2024
Comment on lines 355 to 378
let (value, timestamp) = T::AggregationProvider::aggregate(fed_values)
.ok_or(Error::<T>::KeyNotInCollection)?;

Self::ensure_valid_timestamp(collection_id, timestamp)?;

Ok((value, timestamp))
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'm aggregating both values and timestamps and from the aggregated timestamp, I'm checking that it's not outdated.

Another option could be to filter outdated timestamps and aggregate them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option 3:

  • Final timestamp is the median of the timestamps,
  • If the median is not outdated then the final value is the median of the non-outdated Oracle values.

I see this option quite accurate, but the timestamp probably would be older than the average of used values.

@mustermeiszer

Copy link
Contributor Author

@lemunozm lemunozm Jan 3, 2024

Choose a reason for hiding this comment

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

Final solution (after Frederic sync):

  • Filter outdated oracles until a limit N.
    • If the limit is overpassed, then OracleOutdatedError.
  • Aggregating values & timestamps of the non-outdated oracles

@lemunozm lemunozm force-pushed the oracle/max-age-validation branch 2 times, most recently from fae22cb to 77d0cbe Compare January 3, 2024 10:57
Comment on lines +549 to +557
pub struct CollectionInfo<T: Config> {
/// Maximum duration to consider an oracle value non-outdated.
/// An oracle value is consider updated if its timestamp is higher
/// than `now() - value_lifetime`
pub value_lifetime: Option<T::Timestamp>,

/// Minimun number of feeders to succesfully aggregate a value.
pub min_feeders: u32,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To catch all casuistics well, I've done a new type that is used as a parameter for the new extrinsic set_collection_info() to configure a collection with some properties.

min_feeders force to have at least min_feeders per key in a collection. That means that the feeder should have fed and should have fed recently if value_lifetime is different than None

@lemunozm
Copy link
Contributor Author

lemunozm commented Jan 3, 2024

Should be ready then! Thanks for your fast review @mustermeiszer 🙌🏻

@lemunozm lemunozm force-pushed the oracle/max-age-validation branch from 194f346 to 1f7ac52 Compare January 3, 2024 13:21
@lemunozm lemunozm requested a review from mustermeiszer January 3, 2024 13:21
@lemunozm lemunozm enabled auto-merge (squash) January 3, 2024 14:03
@lemunozm lemunozm merged commit f967ff6 into main Jan 3, 2024
9 checks passed
@lemunozm lemunozm deleted the oracle/max-age-validation branch January 3, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. I8-enhancement An additional feature. P7-asap Issue should be addressed in the next days. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants