`PriceOracle`: Price Oracle (XLS-47d)

[gregtatcam]

High Level Overview of Change

Implement native support for Price Oracles

Add a new ledger object: Oracle

Add two new transactions:

• OracleSet: create or update the Oracle object.
• OracleDelete: delete the Oracle object.

Add one new API

• get_aggregate_price: get aggregate price for the token pair for the oracles.

Context of Change

Notes for reviewers:

• This should be reviewed by someone with transactor knowledge.
• Generally this is a straightforward feature, and it is similar to DID.
• Reading the spec is a prerequisite before reviewing #4789. XLS-47d specification

Type of Change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Refactor (non-breaking change that only restructures code)
• [x] Tests (you added tests for code that already exists, or your new feature included in this PR)
• [ ] Documentation update
• [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• [ ] Release

API Impact

• [x] Public API: New feature (new methods and/or new fields)
• [ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• [ ] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• [ ] Peer protocol change (must be backward compatible or bump the peer protocol version)

Test Plan

Added tests for the new features:

• Oracle
• GetAggregatePrice

[mvadari]

I get this error when I try to compile locally (Mac M1):

src/ripple/rpc/handlers/GetAggregatePrice.cpp:72:41: error: member access into incomplete type 'LedgerMaster'
                    context.ledgerMaster.getLedgerBySeq(prevSeq))
                                        ^
src/ripple/app/main/Application.h:79:7: note: forward declaration of 'ripple::LedgerMaster'
class LedgerMaster;

[gregtatcam]

I get this error when I try to compile locally (Mac M1):

src/ripple/rpc/handlers/GetAggregatePrice.cpp:72:41: error: member access into incomplete type 'LedgerMaster'
                    context.ledgerMaster.getLedgerBySeq(prevSeq))
                                        ^
src/ripple/app/main/Application.h:79:7: note: forward declaration of 'ripple::LedgerMaster'
class LedgerMaster;

Thanks. I built with the unity=ON; should have built with the unity=OFF as well. This file needs #include <ripple/app/ledger/LedgerMaster.h>. Will update.

[thejohnfreeman]

I find the description of the OracleID confusing. The spec says this:

We compute the PriceOracle object ID, a.k.a., OracleID, as the SHA-512Half of the following values, concatenated in order:

• The Oracle space key (0x52)
• The Owner Account ID
• The Oracle Sequence. This field must be passed to the transactions and it describes a unique Price Oracle sequence for the given account.

• OracleID is not mentioned in the description of the fields of PriceOracle, but it is included in the JSON of an example PriceOracle. Is it included in the ledger object or not?
• It is implied but not explicitly stated that the OracleID is used as the SHAMap key for a PriceOracle object. Is it? How are these objects supposed to be laid out in the ledger? The aggregation transaction suggests that it is possible to iterate PriceOracle objects for a given account. How?
• Oracle Sequence is not mentioned in the fields of PriceOracle or shown in the example. Is it possible to recover the OracleID for a PriceOracle using only the fields contained within a PriceOracle? If so, how?
• "Sequence" implies to me that it is a number that increments by one with each change, like a Ledger Sequence or an Account Sequence, but OracleID implies a constant identifier. Which is it? Are there any rules on the sequence number?

In the spec, the list following "The transaction fails if" includes:

• The URI field length exceeds 64 bytes.
• The Provider field length exceeds 64 bytes.

But the section describing the PriceOracle on-ledger object fields says:

• Provider identifies an Oracle Provider. It can be URI or any data, for instance chainlink. It is a string of up to 256 ASCII hex encoded characters (0x20-0x7E).
• URI is an optional URI field to reference price data off-chain. It is limited to 256 bytes.

Which is correct? Can we fix it in the spec?

I find this explanation unclear:

If an object with the OracleID Object ID already exists then the new token pairs are added to the Oracle instance PriceDataSeries array. Note that the order of the token pairs in the PriceDataSeries array is not important since the token pair uniquely identifies location in the PriceDataSeries array of the PriceOracle object. Also note that not every token pair price has to be updated. I.e., even though the PriceOracle may define ten token pairs, OracleSet transaction may contain only one token pair price update. In this case the missing token pair will not include AssetPrice and Scale fields. PreviousTxnID can be used to find the last updated Price Data for this token pair.

"If an object with the ID exists, then the new token pairs are added..." I'm assuming "new token pairs" are ones that do not appear in the PriceOracle. If so, then why no mention of "old token pairs"? They should be updating what appears in the PriceOracle, correct?

"In this case the missing token pair..." What missing token pair? The token pairs that are in the object, but not the transaction? If yes, then what is meant by "[they] will not include AssetPrice and Scale fields"? If they are missing, won't they "not include" every field?

I feel like there is an implied behavior for this transaction that is not well described by the existing language. Here is how I would describe that behavior. Is it correct?

An OracleSet transaction uniquely identifies a PriceOracle object with its Account and OracleSequence fields. If such an object does not yet exist in the ledger, it is created. Otherwise, the existing object is updated. The Provider, URI, and AssetClass fields are copied directly from the transaction, if present. Provider and AssetClass must be included in the transaction if the object is being created.

The PriceDataSeries of the transaction is copied to a newly created PriceOracle object, or updates an existing object, like so:

• PriceData objects for (BaseAsset, QuoteAsset) token pairs that appear in the transaction but not the object are copied to the object.
• PriceData objects for token pairs that appear in both the transaction and the object are overwritten in the object.
• PriceData objects for token pairs that appear only in the object are left unchanged.

The order of token pairs in the transaction is not important because the token pair uniquely identifies the location of the PriceData object in the PriceDataSeries array of the PriceOracle object.

LastUpdateTime, PreviousTxnID, and PreviousTxnLgrSeq are set in the same manner as for an AccountSet transaction.

The owner reserve of the account is updated according to the difference in the size of the PriceDataSeries before and after the transaction is applied: 0 for missing, 1 for 1 - 5 objects, 2 for 6 - 10 objects.

AssetClass is a required field on a PriceOracle object and an optional field in an OracleSet transaction, just like Provider, but its field description does not say that it must be included in the transaction when creating a new instance of PriceOracle, unlike Provider. It must be included in the transaction, correct?

A side note on formatting. I think the presentation in the section on PriceOracle fields is best: one table with all the fields, followed by a bulleted list of field descriptions. Not like the transaction sections with separate tables (repeating the same header row) for each field. I think the example should go directly after or before the table, not at the end of the section.

[gregtatcam]

@thejohnfreeman thanks for the feedback on the specs. I copied your comments and responded on https://github.com/XRPLF/XRPL-Standards/pull/138.

[intelliot]

@gregtatcam to confirm when, from his perspective, this PR is ready to be merged.

[gregtatcam]

@gregtatcam to confirm when, from his perspective, this PR is ready to be merged.

There are some issue discovered during QA testing that I have to look into. Has to do with the fields validation. Will update on Monday.

[intelliot]

note: @SaxenaKaustubh is testing this today

[intelliot]

note: this has been requested for consideration in the 2.1 release

Internal tracker: RPFC-90

[intelliot]

note: I believe the new Oracle objects should be retrievable with account_objects and ledger_entry. (This is relevant to Clio @kennyzlei @godexsoft )

[gregtatcam]

note: I believe the new Oracle objects should be retrievable with account_objects and ledger_entry. (This is relevant to Clio @kennyzlei @godexsoft )

Correct. Oracle objects are retrievable with account_objects and ledger_entry.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 77.06612% with 111 lines in your changes are missing coverage. Please review.

Project coverage is 61.60%. Comparing base (d7d15a9) to head (f839070).

Files	Patch %	Lines
src/ripple/rpc/handlers/GetAggregatePrice.cpp	79.64%	13 Missing and 21 partials :warning:
src/ripple/app/tx/impl/SetOracle.cpp	83.83%	9 Missing and 18 partials :warning:
src/ripple/app/tx/impl/DeleteOracle.cpp	50.00%	11 Missing and 10 partials :warning:
src/ripple/rpc/handlers/LedgerEntry.cpp	48.27%	8 Missing and 7 partials :warning:
src/ripple/protocol/impl/STCurrency.cpp	71.42%	6 Missing and 4 partials :warning:
src/ripple/protocol/impl/STParsedJSON.cpp	57.14%	3 Missing :warning:
src/ripple/protocol/impl/STObject.cpp	83.33%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4789      +/-   ##
===========================================
+ Coverage    61.53%   61.60%   +0.07%     
===========================================
  Files          797      804       +7     
  Lines        70158    70642     +484     
  Branches     36261    36534     +273     
===========================================
+ Hits         43170    43522     +352     
- Misses       19753    19812      +59     
- Partials      7235     7308      +73     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[gregtatcam]

@gregtatcam to confirm when, from his perspective, this PR is ready to be merged.

I think it's good to go and it's good to be merged from @manojsdoshi perspective. But there are a few commits in January that I assume have to be reviewed (@thejohnfreeman , @mvadari )

[intelliot]

• awaiting perf team sign-off
• Internal tracker: RPFC-90

[shortthefomo]

1. can someone explain how this is even useful?
2. is there one transactor that has the ability to even act on this data (with in the ledger).
3. why is it even considered valuable data if nothing can be done with it?

my review of this past that ability to act on the data with in the ledger its self it should never be added to it.

[gregtatcam]

1. can someone explain how this is even useful?
2. is there one transactor that has the ability to even act on this data (with in the ledger).
3. why is it even considered valuable data if nothing can be done with it?

my review of this past that ability to act on the data with in the ledger its self it should never be added to it.

It's useful in that it enables Oracle Providers to bring market pricing data to on-chain. This data then can be used by dApps. For instance, a lending protocol can use the data to decide on the value of assets held as collateral and if the market value of the collateral falls below a certain point, the lending protocol may automatically initiate the liquidation of the collateral to cover the outstanding loan amount.

[shortthefomo]

A decentralized application (DApp) is a type of distributed, open source software application that runs on a peer-to-peer (P2P) blockchain network rather than on a single computer.

Where does this lending protocol sit, this solution needs to be build with that "lending protocol" is that present here?

[gregtatcam]

A decentralized application (DApp) is a type of distributed, open source software application that runs on a peer-to-peer (P2P) blockchain network rather than on a single computer.

Where does this lending protocol sit, this solution needs to be build with that "lending protocol" is that present here?

It's a native landing protocol - feature of XRPL.

[shortthefomo]

It's a native landing protocol - feature of XRPL.

Are you referring to the AMM? Because the code submitted here does not interface with the AMM in any way.

[gregtatcam]

It's a native landing protocol - feature of XRPL.

Are you referring to the AMM? Because the code submitted here does not interface with the AMM in any way.

Not AMM. It's a new feature, not yet implemented.

[intelliot]

• Price oracles haven't been been partner tested or Devnet released yet
• Let's put this in 2.2 (or later)

[gregtatcam]

• Price oracles haven't been been partner tested or Devnet released yet
• Let's put this in 2.2 (or later)

Is it available on devnet?

[intelliot]

No. I would suggest running it in a new Devnet (similar to what was done with amm-devnet). If that isn't possible, then we can do a custom build with this branch and give it a special version number, like 2.1.0+PriceOracle or the like.

[gregtatcam]

No. I would suggest running it in a new Devnet (similar to what was done with amm-devnet). If that isn't possible, then we can do a custom build with this branch and give it a special version number, like 2.1.0+PriceOracle or the like.

Is someone scheduled to work on this? It would be good if the partners could start testing this.

[gregtatcam]

@thejohnfreeman could you please review when you get a chance? There is a minor patch to fix trim validation in get_aggregate_price. I also merged with the latest develop and resolved the merge conflicts with AMM. Thanks.

[intelliot]

per the request for a quick re-review (see above), this is awaiting @thejohnfreeman

[q73zhao]

Performance sign-off

The Price Oracle feature testing on XRPL confirmed its good performance and reliability under simulated MainNet conditions.

The get_aggregate_price API efficiently processed queries on token pairs with the oldest updates among 200 oracle objects, averaging a response time of 6.24 ms under medium payment loads.

Additionally, running oracleset transactions at around 15 per second, with a concurrent payment load of around 100 TPS, showed no significant ledger validation delays or fee escalations, with average response time of 5.71ms

More detailed report can be found here