Skip to content

feat(models): type ledger_entry response node using LedgerEntry enum - #361

Open
soloking1412 wants to merge 1 commit into
XRPLF:mainfrom
soloking1412:feat/type-ledger-entry-node
Open

feat(models): type ledger_entry response node using LedgerEntry enum#361
soloking1412 wants to merge 1 commit into
XRPLF:mainfrom
soloking1412:feat/type-ledger-entry-node

Conversation

@soloking1412

Copy link
Copy Markdown

High Level Overview of Change

Types the ledger_entry result's node field using the existing LedgerEntry enum from models::ledger::objects, replacing the untyped Option<serde_json::Value>.

// before
pub node: Option<serde_json::Value>,
// after
pub node: Option<crate::models::ledger::objects::LedgerEntry<'a>>,

Since the models::ledger::objects::LedgerEntry enum already existed (used by Ledger::account_state for full-ledger state dumps), this reuses it rather than introducing a second, parallel discriminated-union type — one enum, two call sites.

Blockers noted in #308

The issue calls out that the real obstacle is XRPL's wire format: a flat object carrying its own LedgerEntryType discriminator ({"LedgerEntryType": "AccountRoot", "Account": ..., ...}), which doesn't fit serde's default externally-tagged enum representation, and doesn't fit #[serde(tag = "LedgerEntryType")] either without removing ledger_entry_type from CommonFields (a breaking change across every ledger object struct). This implements the issue's suggested workaround: a hand-written Deserialize for LedgerEntry that buffers to serde_json::Value, reads LedgerEntryType to pick the variant, then delegates to serde_json::from_value for that variant's own Deserialize. Serialize is likewise hand-written to flatten straight through to the inner type instead of wrapping it — the enum's default derive was serializing as {"AccountRoot": {...}}, which doesn't match real rippled output either, and was already silently wrong for Ledger::account_state before this change (nothing previously exercised that path with real wire-format JSON).

Unknown/unrecognized LedgerEntryType values are a deserialize error (unknown_variant) rather than silently dropped, so a future ledger object type rippled adds before this crate models it fails loudly instead of losing data.

Context of Change

Filed as #308. Overlaps in idea with the node typing included in #160 (open since April, part of a much larger integration-tests PR) — this is a standalone implementation of just that piece, reusing the existing LedgerEntry enum instead of #160's parallel LedgerEntryNode type, so it doesn't carry a duplicate 26-variant definition and doesn't need an Unknown(Value) catch-all. Flagging the overlap here so it's easy to reconcile with #160 either way.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Tests

Before / After

Deserializing a non-AccountRoot entry — the exact scenario #308 says used to fail before node was widened past a hardcoded AccountRoot struct — now dispatches correctly:

let entry: LedgerEntry = serde_json::from_str(r#"{
    "LedgerEntryType": "DirectoryNode",
    "Owner": "rN7n...",
    ...
}"#)?;
match entry.node.unwrap() {
    LedgerEntry::DirectoryNode(directory_node) => { /* typed access */ }
    _ => unreachable!(),
}

Ledger::account_state (the other consumer of models::ledger::objects::LedgerEntry) now round-trips against the real flat wire format too, as a side effect of fixing Serialize/Deserialize on the shared enum.

Test Plan

  • cargo test --release: 1316 passed (up from 1313 on main).
  • Added to models::ledger::objects: a flat-wire-format deserialize test for a non-AccountRoot entry (DirectoryNode), an unknown-LedgerEntryType rejection test, and a missing-LedgerEntryType rejection test. Tightened the existing round-trip test to assert the wire format is no longer externally-tagged.
  • Rewrote models::results::ledger_entry's existing tests (deserialize, round-trip, DirectoryNode) to pattern-match the typed enum instead of indexing into serde_json::Value.
  • Updated the two integration tests in tests/requests/ledger_entry.rs that inspected node as raw JSON (credential, vault_by_id) to match on the typed variant instead — compiled clean under cargo check --tests --features std,json-rpc,helpers,cli,websocket,integration (these need a live xrpld node to actually execute, so compiled but not run end-to-end here).
  • cargo clippy --all-targets --features std,json-rpc,helpers,cli,websocket,integration and cargo fmt --check: clean on all changed files.

Closes #308

Replaces the untyped node: Option<serde_json::Value> with the existing
models::ledger::objects::LedgerEntry enum. Adds hand-written
Serialize/Deserialize for that enum matching XRPL's actual flat wire format
(an inline LedgerEntryType discriminator), since serde's default
externally-tagged representation doesn't match rippled's output and
#[serde(tag = "LedgerEntryType")] would require removing the duplicate
field from CommonFields across every ledger object. Fixing Serialize
alongside Deserialize also corrects Ledger::account_state, the enum's
other consumer.

Closes XRPLF#308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Type ledger_entry response node field using ledger object models instead of serde_json::Value

1 participant