feat(models): type ledger_entry response node using LedgerEntry enum - #361
Open
soloking1412 wants to merge 1 commit into
Open
feat(models): type ledger_entry response node using LedgerEntry enum#361soloking1412 wants to merge 1 commit into
soloking1412 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Types the
ledger_entryresult'snodefield using the existingLedgerEntryenum frommodels::ledger::objects, replacing the untypedOption<serde_json::Value>.Since the
models::ledger::objects::LedgerEntryenum already existed (used byLedger::account_statefor 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
LedgerEntryTypediscriminator ({"LedgerEntryType": "AccountRoot", "Account": ..., ...}), which doesn't fit serde's default externally-tagged enum representation, and doesn't fit#[serde(tag = "LedgerEntryType")]either without removingledger_entry_typefromCommonFields(a breaking change across every ledger object struct). This implements the issue's suggested workaround: a hand-writtenDeserializeforLedgerEntrythat buffers toserde_json::Value, readsLedgerEntryTypeto pick the variant, then delegates toserde_json::from_valuefor that variant's ownDeserialize.Serializeis 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 forLedger::account_statebefore this change (nothing previously exercised that path with real wire-format JSON).Unknown/unrecognized
LedgerEntryTypevalues 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
nodetyping 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 existingLedgerEntryenum instead of #160's parallelLedgerEntryNodetype, so it doesn't carry a duplicate 26-variant definition and doesn't need anUnknown(Value)catch-all. Flagging the overlap here so it's easy to reconcile with #160 either way.Type of Change
Before / After
Deserializing a non-
AccountRootentry — the exact scenario #308 says used to fail beforenodewas widened past a hardcodedAccountRootstruct — now dispatches correctly:Ledger::account_state(the other consumer ofmodels::ledger::objects::LedgerEntry) now round-trips against the real flat wire format too, as a side effect of fixingSerialize/Deserializeon the shared enum.Test Plan
cargo test --release: 1316 passed (up from 1313 onmain).models::ledger::objects: a flat-wire-format deserialize test for a non-AccountRootentry (DirectoryNode), an unknown-LedgerEntryTyperejection test, and a missing-LedgerEntryTyperejection test. Tightened the existing round-trip test to assert the wire format is no longer externally-tagged.models::results::ledger_entry's existing tests (deserialize, round-trip,DirectoryNode) to pattern-match the typed enum instead of indexing intoserde_json::Value.tests/requests/ledger_entry.rsthat inspectednodeas raw JSON (credential,vault_by_id) to match on the typed variant instead — compiled clean undercargo check --tests --features std,json-rpc,helpers,cli,websocket,integration(these need a livexrpldnode to actually execute, so compiled but not run end-to-end here).cargo clippy --all-targets --features std,json-rpc,helpers,cli,websocket,integrationandcargo fmt --check: clean on all changed files.Closes #308