feat: add PriceOracle support (XLS-47)#156
Conversation
… #3270) (#291) ## Summary The `rippled` binary was renamed to `xrpld` upstream, and the `rippleci/rippled` image stopped receiving updates. Our integration tests across every open PR started failing because the published `develop` image exited before becoming healthy (`Connection refused` on `localhost:5005`, **0 passed / 41 failed**). This PR mirrors the upstream fix in xrpl.js: [XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270). Switching to `rippleci/xrpld:develop` is the **actual root-cause fix** rather than pinning an old digest of the deprecated image. ## Changes `.github/workflows/integration_test.yml`: - `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop`. - `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image handles `mkdir` + launch internally; no more `bash -c "mkdir -p /var/lib/rippled/db/ && rippled -a"` wrapper). - Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`. - Container name: `rippled-service` -> `xrpld-service`. - Removed the docker `--health-cmd` (which shelled out to the renamed `rippled` CLI and always failed) in favour of a direct JSON-RPC poll against `http://localhost:5005/`. - Always dump container logs on the stop step for post-mortem visibility. `.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`: - `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`. - `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`. - `[debug_logfile] /var/log/rippled/debug.log` -> `/var/log/xrpld/debug.log`. ## Verification Validated on throwaway PR #292 (now closed): **Integration Test green in 2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check also pass. ## Related follow-up The 7 in-flight PRs (#130, #131, #151, #153, #156, #157, #158) currently carry a stopgap commit pinning `rippleci/rippled:develop` to a specific digest. After this PR merges to `main`, those branches should: 1. Rebase on `main` to pick up the xrpld switch, or 2. Cherry-pick this commit and drop the stopgap digest pin. ## Test plan - [x] Validated end-to-end on PR #292 - [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass - [ ] Merge and confirm subsequent PRs inherit the fix without manual cherry-pick ## Credit Approach lifted from @ckeshava's [xrpl.js#3270](XRPLF/xrpl.js#3270).
ckeshava
left a comment
There was a problem hiding this comment.
Additional findings after cross-verifying against rippled develop. Each comment links to the upstream source that confirms the issue.
| #[serde(rename = "URI")] | ||
| pub uri: Option<Cow<'a, str>>, | ||
| /// A hint indicating which page of the owner directory links to this entry. | ||
| pub owner_node: Option<Cow<'a, str>>, |
There was a problem hiding this comment.
Two issues on OwnerNode:
1. Wrong optionality: rippled marks it soeREQUIRED, not optional.
2. Wrong underlying type: the field is SF_UINT64, not a string. JSON-RPC serializes it as a 16-char hex string (e.g. "0000000000000000"), but the canonical type is u64. Other ledger objects in this crate that carry OwnerNode should be checked for the same modeling drift.
Source: include/xrpl/protocol_autogen/ledger_entries/Oracle.h — getOwnerNode() (soeREQUIRED) -> SF_UINT64::type::value_type.
Separately, the same header documents sfOracleDocumentID (soeOPTIONAL) on the ledger entry, but that field is missing from this struct entirely. Worth adding as pub oracle_document_id: Option<u32> for ledger_entry RPC consumers who want to look it up.
There was a problem hiding this comment.
Fixed in 4075569: ledger Oracle.owner_node is now a required u64 with 16-character hex string JSON serde, and optional oracle_document_id was added for ledger-entry RPC consumers.
Add PriceData struct using the serde_with_tag! macro to support the XLS-47 PriceOracle amendment. Also adds OracleSet and OracleDelete variants to the TransactionType enum and module declarations for the oracle transaction types.
Implement the OracleSet transaction for creating and updating price oracle ledger entries. Includes new() constructor, builder methods for oracle-specific fields, and comprehensive unit tests covering serialization round-trips, builder patterns, and edge cases.
Implement the OracleDelete transaction for removing price oracle ledger entries. Includes new() constructor and unit tests covering serialization, builder patterns, and boundary values.
Add the Oracle ledger object representing on-ledger price oracle state. Includes Oracle variant in LedgerEntryType (0x0080) and LedgerEntry enums, new() constructor, and serde tests.
Add integration test stubs for OracleSet and OracleDelete transactions. These tests validate type construction and serde round-trips without requiring a live rippled instance (gated behind the integration feature).
OracleDocumentID and LastUpdateTime are required by the XRPL protocol but were typed as Option<u32>, allowing callers to construct invalid transactions that rippled would reject. Changed both to u32 to match the OracleDelete pattern and the protocol specification. Added price_data_series length validation (max 10 entries) in get_errors() to enforce the protocol limit at the model layer.
Address review findings on OracleSet / PriceData validation:
- Reject an empty `price_data_series` with `ValueTooLow { min: 1 }` when
the field is present. rippled requires at least one entry.
- Implement `Model::get_errors` for `PriceData`, enforcing `0 <= scale
<= 10` per XLS-47.
- Validate `base_asset` and `quote_asset` when present: must be either a
3-character ISO-style code or a 40-character hex code, and cannot be
the reserved symbol "XRP". `OracleSet::get_errors` now propagates each
entry's validation error.
- Update pre-existing tests that used "XRP" as an asset and add new
cases: empty series rejection, scale boundary (10 ok, 11 rejected),
invalid 4-char base asset rejection, explicit "XRP" rejection, and
40-char hex acceptance.
ec0d356 to
d706f54
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
==========================================
+ Coverage 83.51% 83.93% +0.41%
==========================================
Files 219 222 +3
Lines 22304 23456 +1152
==========================================
+ Hits 18628 19688 +1060
- Misses 3676 3768 +92
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ckeshava
left a comment
There was a problem hiding this comment.
-
Hello @e-desouza , the integration tests need to be updated to use hexadecimal characters.
-
The get_fee integration test might fail due to unavailability / flakiness of the testnet servers. In that case, there might be nothing wrong with the SDK code, however we will need to re-run the integ test.
| #[test] | ||
| fn test_oracle_delete_serde_roundtrip() { | ||
| let oracle_delete = OracleDelete::new( | ||
| "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW".into(), | ||
| None, | ||
| Some("12".into()), | ||
| None, | ||
| None, | ||
| Some(391), | ||
| None, | ||
| None, | ||
| None, | ||
| 1, | ||
| ); | ||
|
|
||
| let json = serde_json::to_string(&oracle_delete).unwrap(); | ||
| let deserialized: OracleDelete = serde_json::from_str(&json).unwrap(); | ||
| assert_eq!(oracle_delete, deserialized); | ||
| } |
There was a problem hiding this comment.
serde tests are already covered in the unit tests, this can be removed from the integ test file.
There was a problem hiding this comment.
Fixed in f20aeb9: removed the redundant serde roundtrip tests from the integration test files.
| pub fn new( | ||
| index: Option<Cow<'a, str>>, | ||
| ledger_index: Option<Cow<'a, str>>, | ||
| owner: Cow<'a, str>, | ||
| provider: Cow<'a, str>, | ||
| asset_class: Cow<'a, str>, | ||
| price_data_series: Vec<PriceData>, | ||
| last_update_time: u32, | ||
| uri: Option<Cow<'a, str>>, | ||
| owner_node: u64, | ||
| previous_txn_id: Cow<'a, str>, | ||
| previous_txn_lgr_seq: u32, | ||
| oracle_document_id: Option<u32>, | ||
| ) -> Self { |
There was a problem hiding this comment.
what is the need for defining these types of new() constructors? A user will never need to "construct" a ledger-object. Rather, the rippled validator/server will send ledger-objects as a part of the transaction-status / RPC-response messages.
SDKs only need to "parse" such ledger-objects. I cannot think of valid use-cases for this new constructor here.
There was a problem hiding this comment.
Fixed in 5febcdc: removed the unused Oracle::new() constructor so the ledger object is only modeled for parsing/deserialization.
| mod owner_node_hex { | ||
| use alloc::format; | ||
| use core::fmt; | ||
| use serde::de::{self, Visitor}; | ||
| use serde::{Deserializer, Serializer}; | ||
|
|
||
| pub fn serialize<S>(value: &u64, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| serializer.serialize_str(&format!("{value:016X}")) | ||
| } | ||
|
|
||
| pub fn deserialize<'de, D>(deserializer: D) -> Result<u64, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| deserializer.deserialize_any(OwnerNodeVisitor) | ||
| } |
There was a problem hiding this comment.
- A majority of the ledger-objects contain an "OwnerNode" field. From an architecture PoV, this should be placed under a
common/orutils/folder. Why is this located solely inside Oracle ledger object?
There was a problem hiding this comment.
As an aside, can you add an integration test that exercises the "serde" portion of this code? Fetching the metadata of a transaction and deserializing it to produce the expected Oracle object will help validate this code.
There was a problem hiding this comment.
Fixed in 5febcdc: removed the Oracle-local owner_node_hex helper instead of keeping a one-off utility on this ledger object.
There was a problem hiding this comment.
Covered in fa71e72: the integration suite now verifies the account_objects lifecycle after OracleSet/OracleDelete, which exercises fetching and deserializing the Oracle ledger object from the node.
| None, | ||
| None, | ||
| 1, | ||
| Some("chainlink".into()), |
There was a problem hiding this comment.
The value chainlink must be encoded in HEX format before being submitted into a rippled server. Here is the error I see when I run this test:
➜ xrpl-rust git:(d706f54) ✗ cargo test --features std,json-rpc,helpers,cli,websocket,integration \
--test integration_test \
transactions::oracle_set::test_oracle_set_submit \
-- --test-threads=1 --nocapture
warning: unused imports: `AsyncWebSocketClient`, `SingleExecutorMutex`, and `WebSocketOpen`
--> tests/common/mod.rs:22:29
|
22 | use xrpl::asynch::clients::{AsyncWebSocketClient, SingleExecutorMutex, WebSocketOpen};
| ^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^
|
= note: `#[warn(unused_imports)]` (part of `#[warn(unused)]`) on by default
warning: `xrpl-rust` (test "integration_test") generated 1 warning (run `cargo fix --test "integration_test" -p xrpl-rust` to apply 1 suggestion)
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.53s
Running tests/integration_test.rs (target/debug/deps/integration_test-d80605ce5156b007)
running 1 test
test transactions::oracle_set::test_oracle_set_submit ...
thread 'transactions::oracle_set::test_oracle_set_submit' (18656592) panicked at tests/common/mod.rs:270:10:
test_transaction: sign_and_submit failed: XRPLCoreError(XRPLBinaryCodecError(XRPLTypeError(TryFromStrError)))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
FAILED
failures:
failures:
transactions::oracle_set::test_oracle_set_submit
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 85 filtered out; finished in 0.07s
error: test failed, to rerun pass `--test integration_test`
There was a problem hiding this comment.
Fixed in ecb6705: moved the oracle test metadata into shared fixtures and now uses hex-encoded Blob literals for Provider, AssetClass, and URI in the integration tests.
4a717d2 to
fa71e72
Compare
… #3270) (XRPLF#291) ## Summary The `rippled` binary was renamed to `xrpld` upstream, and the `rippleci/rippled` image stopped receiving updates. Our integration tests across every open PR started failing because the published `develop` image exited before becoming healthy (`Connection refused` on `localhost:5005`, **0 passed / 41 failed**). This PR mirrors the upstream fix in xrpl.js: [XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270). Switching to `rippleci/xrpld:develop` is the **actual root-cause fix** rather than pinning an old digest of the deprecated image. ## Changes `.github/workflows/integration_test.yml`: - `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop`. - `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image handles `mkdir` + launch internally; no more `bash -c "mkdir -p /var/lib/rippled/db/ && rippled -a"` wrapper). - Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`. - Container name: `rippled-service` -> `xrpld-service`. - Removed the docker `--health-cmd` (which shelled out to the renamed `rippled` CLI and always failed) in favour of a direct JSON-RPC poll against `http://localhost:5005/`. - Always dump container logs on the stop step for post-mortem visibility. `.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`: - `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`. - `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`. - `[debug_logfile] /var/log/rippled/debug.log` -> `/var/log/xrpld/debug.log`. ## Verification Validated on throwaway PR XRPLF#292 (now closed): **Integration Test green in 2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check also pass. ## Related follow-up The 7 in-flight PRs (XRPLF#130, XRPLF#131, XRPLF#151, XRPLF#153, XRPLF#156, XRPLF#157, XRPLF#158) currently carry a stopgap commit pinning `rippleci/rippled:develop` to a specific digest. After this PR merges to `main`, those branches should: 1. Rebase on `main` to pick up the xrpld switch, or 2. Cherry-pick this commit and drop the stopgap digest pin. ## Test plan - [x] Validated end-to-end on PR XRPLF#292 - [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass - [ ] Merge and confirm subsequent PRs inherit the fix without manual cherry-pick ## Credit Approach lifted from @ckeshava's [xrpl.js#3270](XRPLF/xrpl.js#3270).
…hout tests - MAX_PRICE_DATA_SCALE: 10 → 20 (matches rippled kMaxPriceScale in Protocol.h) - MAX_ORACLE_ASSET_PRICE: 0x7FFFFFFFFFFFFFFF → u64::MAX (AssetPrice is plain UInt64; xrpl.js integration test uses 0xFFFFFFFFFFFFFFFF successfully) - validate_optional_blob: add zero-length rejection to match rippled isInvalidLength (length==0 also returns temMALFORMED) - Replace all OracleDelete::new() and OracleSet::new() positional calls with struct literal pattern for readability - Extract TEST_ACCOUNT, TEST_FEE, TEST_SEQUENCE, TEST_LAST_LEDGER, TEST_DOC_ID, TEST_LAST_UPDATE_TIME, TEST_PROVIDER, TEST_ASSET_CLASS constants into each test module to eliminate repeated literals - Add comments to the None fields in CommonFields::new() to name network_id, signing_pub_key, and txn_signature - Add test_scale_mid_range_ok (scale 15 must pass) - Add test_asset_price_full_u64_range_accepted - Add test_asset_price_non_hex_rejected - Add test_empty_blob_fields_rejected
…elds None slots
- tests/common/constants.rs: add ORACLE_PROVIDER, ORACLE_ASSET_CLASS, ORACLE_URI,
ORACLE_URI_HTTPS, TEST_ACCOUNT so oracle tests share a single source of truth
for all hex-encoded Blob literals and the reusable test account address
- tests/common/mod.rs: add SubmitOptions struct + submit_tx() helper; raw
sign_and_submit() calls with unnamed true/true booleans in failure-path tests
are replaced with submit_tx(..., SubmitOptions { wallet, autofill, check_fee })
making each argument self-documenting
- tests/transactions/oracle_set.rs: import and use new fixtures; replace all
three sign_and_submit positional calls with submit_tx struct pattern
- tests/transactions/oracle_delete.rs: same — import fixtures, replace the
sign_and_submit call in test_oracle_delete_not_found with submit_tx
- src/models/transactions/oracle_set.rs: add inline comments to the three None
slots in CommonFields::new() — network_id, signing_pub_key, txn_signature
e1f5d4e to
95cc6f5
Compare
Summary
Implements XLS-47 (PriceOracle) support for xrpl-rust.
New transaction types
OracleSet— Create or update a price oracle with up to 10PriceDataentriesOracleDelete— Delete an existing price oracle byOracleDocumentIDNew ledger entry type
Oracle(LedgerEntryType = 0x0080) — On-ledger price oracle object withPriceDataSeries,Provider,AssetClass,URI, andLastUpdateTimeShared types
PriceData— Nested STObject (viaserde_with_tag!) withBaseAsset,QuoteAsset,AssetPrice, andScalefieldsValidation
OracleSet.price_data_seriescapped at 10 entries (per XLS-47 spec)oracle_document_idandlast_update_timeare required (non-optional) fieldsRegistration
TransactionType::OracleSet,TransactionType::OracleDeleteadded to enumLedgerEntryType::Oracle(0x0080) added to enumTest plan
new()constructor testsget_transaction_type()variant testsprice_data_serieslength > 10 rejectedtests/transactions/cargo fmt,cargo clippy --all-features,cargo test --releaseall pass