Oracles: `on_new_data()` improvements

[lemunozm]

Hi,

As a user of orml_oracle, we've found some limitations regarding the on_new_data() callback when the oracle is fed:

1. on_new_data() should return a DispatchResult. A user could want to run custom logic that, if it fails, the oracle should not be fed. Right now, from the callback, we have no control over this situation.
2. on_new_data() should be called once with the whole list instead of one time per value. In our case, we use this callback as a trigger to make some expensive computations. We would save a lot of computation if we could combine this for all values.
3. on_new_data() should return the Weight. Because the user can run an expensive operation inside the callback, it will be awesome to reflect it to accurately the block time computation.

We can discuss any of the points. If you finally agree with them, I could also make a PR. Thanks!

[xlc]

All seems reasonable to me.

[lemunozm]

I want to add a new point that could be more polemic, but I think it would help a lot to reason about the code:

4. on_new_data() should have only account_id and key as params, without giving the value.

When things become complex with oracles, it's highly probable that the DataProvider/DataProviderExtended ends up somehow mapping the value coming from the oracle. This means that you must consider DataProvider as a source of truth for the values, and you must never read/trust the value from on_new_data() because, as a user of these traits, you do not know if the implementation of DataProvider is modifying it.

I have the strong opinion that on_new_data() should only be a trigger of a new value fed/updated, but without giving the proper value. It's the user who has to choose what to do with that trigger event.

[xlc]

We need solid use cases to determine exactly how we want to do the change. A feature that no one is using is just a maintenance burden.

Also (4) is not compatible with (1). We need to have two different hook to achive both goals.

[lemunozm]

Why (4) is not compatible with (1)? The resulting signature adding these two points will be:

/// Triggers an event saying that a member has updated the key with a new value.
/// If this method fails, the Oracle update is finally not applied.
/// If the user wants to know the value, they should use `DataProvider`.
fn on_new_data(account_id: &AccountId, key: &Key) -> DispatchResult;

If the problem is that most implementors of OnNewData could need to use a DataProvider, it could be fixed as follows:

fn on_new_data<Provider: DataProvider>(account_id: &AccountId, key: &Key) -> DispatchResult {
    // As an implementor of this:
    // 1. I can choose if I really want the oracle to be updated or not returning an Err.
    // 2. I can trigger some computation for this event.
    // 3. I can read the value as DataProvider::get(key)
}

In our use case, we use oracles to set prices. For oracle members, they feed with fiat values, but for Oracle users through DataProvider, we offer the stable coin value, and therefore DataProvider does a mapping converting values. I can not trust the value of on_new_data() because the source of truth is what DataProvider gives.

[xlc]

I see two use cases here:

1. Filter values before they are stored. This is used to reject invalid values.
2. Notify other pallets that new oracle is available.

1 should happen before the value is stored. 2 should happen after the value is stored.

[lemunozm]

I think both are the same case because the extrinsic is transactional, so:

1. Values are stored.
2. Notify new value is stored.
3. If the on_new_data() rejects, the whole transaction is down, and nothing is stored.

[xlc]

But a provider can feed multiple values at once. So we need to either

for new_data in values
  with tranactional
    save(new_data)
    if !on_new_data(new_data)
      revert

or

let updated = values.filter_map |new_data| =>
  if on_pre_new_data(new_data)
    save(new_data)
    Some(new_data)
  else
    None

for new_data in updated
  on_new_data(new_data)

Both works but the second way is less overhead.

[lemunozm]

Yeah, with several values we have that issue.

To avoid choosing between an overhead (example 1) or complexity (example 2) solution, could we agree directly on all values or reject all if some of the value is not correct? From my point of view, it's like if I agree directly on the whole extrinsic called by the oracle member, which could make sense.

This would also add point (2) into the game:

with transactional
  for new_data in values
    save(new_data)
  on_new_data(values) // Agree all or disagree all values

So, if there is an error, the member should call again with a correct list of values.

[xlc]

That could work. We just need to document it clearly.