feat: price-oracle pallet

[Roznovjak]

Description

Implement on-chain oracles providing price estimates for asset pairs based on previous trading data.
Aggregate data of different length periods( e.g. 1, 10, 100 and 1000 blocks ) and calculate average prices.
Values tracked: trading prices and volumes, total pool liquidity.

Related Issue

Fixes: #106

[codecov]

Codecov Report

Merging #112 (17f54ed) into master (edec51c) will increase coverage by 0.04%.
The diff coverage is 85.28%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   85.22%   85.26%   +0.04%     
==========================================
  Files          21       24       +3     
  Lines        2132     2362     +230     
==========================================
+ Hits         1817     2014     +197     
- Misses        315      348      +33     

Impacted Files	Coverage Δ
pallets/lbp/src/benchmarking.rs	100.00% <ø> (ø)
pallets/lbp/src/lib.rs	86.00% <ø> (ø)
pallets/price-oracle/src/types.rs	57.40% <57.40%> (ø)
primitives/src/traits.rs	74.07% <76.92%> (+2.64%)	⬆️
pallets/xyk/src/lib.rs	83.09% <83.33%> (+0.25%)	⬆️
pallets/price-oracle/src/lib.rs	89.23% <89.23%> (ø)
pallets/price-oracle/src/benchmarking.rs	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edec51c...17f54ed. Read the comment docs.

[Roznovjak]

Our use case is to allow another pallets to call on_trade() handler and populate the PriceBuffer with trading data. At the end (or at the beginning) of a block, this pallet processes the data from the price buffer. There is no upper limit on how many entries the buffer can have. If the buffer contains multiple trades of the same asset, we calculate average value.
We need to figure out how to benchmark on_trade() method and whether to process the data from the PriceBuffer in on_initialize or in on_finalize. It should be ok to update prices in the next block ( in the case of on_initialize).

@jak-pan @lumir-mrkva @apopiak your expertise would be greatly appreciated.

[green-jay]

polkadotJS types definition seems to be missing

[green-jay]

did you? :)

[Roznovjak]

Any idea how this can be done?

[green-jay]

This is probably going to be configured from time to time so might be better as a pallet const
Also - maybe NonZeroU32 type can be used instead?

[Roznovjak]

We can't use NonZeroU32 because its constructor is not const. The same problem applies also for pallet config parameters.

[green-jay]

Our use case is to allow another pallets to call on_trade() handler and populate the PriceBuffer with trading data. At the end (or at the beginning) of a block, this pallet processes the data from the price buffer. There is no upper limit on how many entries the buffer can have. If the buffer contains multiple trades of the same asset, we calculate average value.
We need to figure out how to benchmark on_trade() method and whether to process the data from the PriceBuffer in on_initialize or in on_finalize. It should be ok to update prices in the next block ( in the case of on_initialize).

@jak-pan @lumir-mrkva @apopiak your expertise would be greatly appreciated.

afaik rule of thumb is keep the load on on_finalize minimized, so all the heavier operations should always be processed in on_inialize

[jak-pan]

Our use case is to allow another pallets to call on_trade() handler and populate the PriceBuffer with trading data. At the end (or at the beginning) of a block, this pallet processes the data from the price buffer. There is no upper limit on how many entries the buffer can have. If the buffer contains multiple trades of the same asset, we calculate average value.
We need to figure out how to benchmark on_trade() method and whether to process the data from the PriceBuffer in on_initialize or in on_finalize. It should be ok to update prices in the next block ( in the case of on_initialize).
@jak-pan @lumir-mrkva @apopiak your expertise would be greatly appreciated.

afaik rule of thumb is keep the load on on_finalize minimized, so all the heavier operations should always be processed in on_inialize

Did this change? We were told the exact opposite before when we were doing exchange. The only thing you need to make sure is to reserve weight in on-initialize -> That's why the exchange order matching is done in on-finalize. (The data structures can be still cleaned for that block so we don't need to save unnecessary data to blockchain)

[apopiak]

The most important thing is to make sure that the weight used is predictable. As long as all weight consumed in the hooks is tracked it does not matter whether you execute in on_finalize, on_initialize or on_idle from that perspective.
So a thing to avoid is to execute arbitrary logic in on_finalize e.g. iterating a storage map whose number of keys you don't know.
If you do something where you don't know the weight ahead of time, you're better of doing it in on_initialize and keeping track of the weight as you're executing (and limiting it to the total block weight). If you can estimate/limit the weight ahead of time you can do it in on_finalize.
As Jakub mentioned there are, of course, also other considerations why you might choose one over the other.

[apopiak]

There is no upper limit on how many entries the buffer can have.

This line makes me uncomfortable xD
You might want to
a) argue why it's still safe (even though it might lead to unbounded execution time)
or b) change that

[enthusiastmartin]

Few more comments. I haven't got through it all yet.

[enthusiastmartin]

Is the iteration necessary ? I am not sure how many there can be but i'd rather try to avoid.

Perhaps different storage ?

[Roznovjak]

Yes it is necessary. We need to check the existence of an asset pair and increase the asset_count if the asset pair exists because it is used in weigh calculation here

Basilisk-node/pallets/price-oracle/src/weights.rs

Line 82 in 5344652

fn on_initialize_one_entry_multiple_tokens(t: u32, ) -> Weight {

[enthusiastmartin]

yes, i know it is necessary now. I really meant if this could not be done in a way where there is no need to iterate over it just to find the pair

[Roznovjak]

I'm not aware of a better solution. We need to use an iteration because we use a vector. The reason behind this is that we need to iterate over all values at the end of each block, and because it is advised to use vectors over maps (faster iterations). https://substrate.dev/docs/en/knowledgebase/runtime/storage#iterating-over-storage-maps

[mrq1911]

why not to just use a map where the check for existence could be O(1) ?

[Roznovjak]

As I stated above, we use a vector because we iterate over all values at the end of each block. For me, it makes sense to prioritize iterations, because a lookup is a "one time thing" (occurs only when a new pool is created), but iterating takes place at the end of each block. There's a plan to modify the algorithm for price calculations, so this implementation might change in the future.