Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify JSON serialization format of transactions (Version: 2.0.0-b2) #4727

Closed
intelliot opened this issue Sep 25, 2023 · 2 comments · Fixed by #4759 or #4775
Closed

Unify JSON serialization format of transactions (Version: 2.0.0-b2) #4727

intelliot opened this issue Sep 25, 2023 · 2 comments · Fixed by #4759 or #4775
Assignees
Labels
API Change Clio Reviewed Feature Request Used to indicate requests to add new features
Milestone

Comments

@intelliot
Copy link
Collaborator

intelliot commented Sep 25, 2023

There are many API methods that can return transactions. Currently, the return formats are inconsistent. The following table shows where fields are located as of rippled v1.12.0 (unchanged since v1.0.1).

Note: In the following table, the "top" level is with respect to the individual object representing the transaction; for methods that return arrays of transactions, the "top" level is per element of the array. "Transaction Instructions" refer to the canonical fields of the transaction as defined in the transaction format, such as the Account and Flags fields.

Method Transaction Instructions Transaction Metadata hash ledger_index inLedger validated ledger_hash Binary blob date
ledger (with transactions expanded) Top level metaData field (JSON)
or in meta field¹ (binary)
Top level Above transaction level None Above transaction level None tx_blob field¹ None
tx Top level meta field Top level Top level Top level Top level None tx field¹ Top level
tx_history (deprecated) Top level None Top level Top level Top level None None Not supported None
account_tx tx field meta field tx field tx field tx field Top level None tx_blob field¹ None
transaction_entry (would like to deprecate) tx_json field metadata field tx_json field Top level None Top level Top level Not supported None
sign, sign_for, submit, and submit_multisigned tx_json field (N/A) tx_json field (N/A) (N/A) (N/A) (N/A) tx_blob field None
Streams from subscribe transaction field meta field² Top level Top level² None Top level² Top level² Not supported transaction field
Data API v2 methods tx field meta field Top level Top level None None (Data API only reports validated transaction data) None tx¹ Top level (as ISO8601 timestamp, not Ripple time)

¹ Only if the request asked for binary data
² The transactions_proposed stream omits these fields because the transactions' outcomes are not yet final.

Recommendation

Change all of rippled's methods to serialize transactions to JSON in a single consistent format. The format I suggest is essentially the same format the Data API uses, with modifications to accommodate for some formats only the rippled APIs handle. Specifically:

  1. Always use tx_blob for binary format. Always use tx_json for JSON format transaction instructions.
  2. Only include the canonical transaction instructions ("uppercase fields") and signing fields in the tx_json field with transaction instructions. Move all "lowercase" fields like hash and ledger_index out to the top level of the transaction. (Note: the issuer/currency/value sub-fields of amounts are canonical fields even though they are lowercase.)
  3. Always use meta for JSON metadata. Use meta_blob for binary metadata.
  4. Remove the inLedger field entirely. (It was a deprecated alias for ledger_index, which is the ledger index of the ledger that includes this transaction.)
  5. Add the close_time_iso field, as a sibling to tx_json, to all validated transactions that are from a closed ledger. Most of the time this would be the close time of the parent ledger. Make this a UTC ISO8601 timestamp with whole-seconds resolution, for example 2018-07-22T16:37:55Z. Omit this field from transactions that are not in closed ledgers.
  6. Add the ledger_hash field to any transactions when pulling them from a closed ledger. Omit this field from transactions that are not in closed ledgers.
  7. Add the validated boolean field to all transactions, even when this information is redundant because of context (for example, listing transactions in a validated ledger).
@intelliot intelliot added API Change Feature Request Used to indicate requests to add new features labels Sep 25, 2023
@intelliot intelliot added this to the 2.0 milestone Sep 25, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Core Ledger Sep 25, 2023
@pkcs8
Copy link

pkcs8 commented Sep 26, 2023

Great to see this initiative to unify output formats. meta, metadata and metaData has been one of my longest pet peeves.

@intelliot intelliot changed the title Unify JSON serialization format of transactions (Version: 2.0.0) Unify JSON serialization format of transactions (Version: 2.0.0-b2) Sep 27, 2023
@intelliot
Copy link
Collaborator Author

tx_history is deprecated and I haven't seen any usage of it for many years. I think that if you specify "command": "tx_history" and "api_version": 2, then you should get an error similar to:

{
  "error": "unknownCmd",
  "error_code": 32,
  "error_message": "Unknown method.",
  "status": "error",
  "type": "response",
  "id": 1,
  "request": {
    "id": 1,
    "command": "tx_history",
    "api_version": 2
  }
}

If we want to be friendlier, I think it's also OK to add an informational field that says tx_history is deprecated and was removed in api_version 2.

intelliot pushed a commit that referenced this issue Oct 24, 2023
Remove `tx_history` and `ledger_header` methods from API version 2.

Update `RPC::Handler` to allow for methods (or method implementations)
to be API version specific. This partially resolves #4727. We can now
store multiple handlers with the same name, as long as they belong to
different (non-overlapping) API versions. This necessarily impacts the
handler lookup algorithm and its complexity; however, there is no
performance loss on x86_64 architecture, and only minimal performance
loss on arm64 (around 10ns). This design change gives us extra
flexibility evolving the API in the future, including other parts of
#4727.

In API version 2, `tx_history` and `ledger_header` are no longer
recognised; if they are called, `rippled` will return error
`unknownCmd`

Resolve #3638

Resolve #3539
@intelliot intelliot reopened this Oct 25, 2023
@intelliot intelliot moved this from 📋 Backlog to 🏗 In progress in Core Ledger Oct 25, 2023
@intelliot intelliot linked a pull request Oct 27, 2023 that will close this issue
12 tasks
intelliot added a commit that referenced this issue Nov 3, 2023
Fix #4727

Squashed commit of the following:

commit 53e384f
Author: Bronek Kozicki <[email protected]>
Date:   Fri Nov 3 13:37:29 2023 +0000

    Minor improvements

commit c1f2ea4
Merge: 942e209 056255e
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 21:43:11 2023 +0000

    Merge branch 'develop' into feature/unify_transaction_json

commit 942e209
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 21:26:05 2023 +0000

    Unconditionally set validated in account_tx output

commit b5b1118
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 21:14:37 2023 +0000

    Remove obsolete comments

commit 7770bc5
Author: Chenna Keshava <[email protected]>
Date:   Thu Nov 2 13:16:44 2023 -0700

    simplify the extraction of transactionID from Transaction object

commit 94c16d8
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 20:26:09 2023 +0000

    Replace class enum JsonOptions with struct

    We may consider turning this into a general-purpose template and using it elsewhere

commit b2a8a2d
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 17:46:54 2023 +0000

    Minor improvements

commit 95b6055
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 17:14:53 2023 +0000

    Improve getJson for Transaction and STTx

commit 17e6588
Author: Bronek Kozicki <[email protected]>
Date:   Thu Nov 2 12:06:30 2023 +0000

    Fix typos

commit 5973390
Author: Bronek Kozicki <[email protected]>
Date:   Wed Nov 1 15:55:02 2023 +0000

    Fix validated and close_time_iso in account_tx

commit cdb3384
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 31 17:27:57 2023 +0000

    Add ledger_hash, ledger_index to transaction_entry

commit 0d8673c
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 31 17:12:53 2023 +0000

    Update API-CHANGELOG.md

commit 625c812
Merge: 54f1d60 3b624d8
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 21:32:35 2023 +0000

    Merge branch 'develop' into feature/unify_transaction_json

commit 54f1d60
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 21:19:40 2023 +0000

    Set ledger_hash on closed ledger, even if not validated

commit ef1276c
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 20:59:58 2023 +0000

    Minor fixes

commit c81bece
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 17:45:49 2023 +0000

    Rename mInLedger to mLedgerIndex

commit c477eb4
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 17:35:33 2023 +0000

    Improved comments

commit e386093
Author: Chenna Keshava <[email protected]>
Date:   Thu Oct 26 17:51:41 2023 -0700

    additional tests for Subscribe unit tests

commit 9f1d544
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 30 16:09:17 2023 +0000

    Time formatting fix

commit 4e92ee4
Author: Bronek Kozicki <[email protected]>
Date:   Thu Oct 26 14:04:40 2023 +0000

    Store closeTime in LedgerFill

commit 1115cef
Author: Bronek Kozicki <[email protected]>
Date:   Thu Oct 26 13:22:26 2023 +0100

    Move isValidated from RPCHelpers to LedgerMaster

commit 2035bbc
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 24 19:15:50 2023 +0000

    Set ledger_hash and ledger_index

commit bdf90e5
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 24 18:24:30 2023 +0000

    Remove inLedger from API version 2

commit c2a3b52
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 24 17:39:45 2023 +0000

    Add unit test for tx

commit 1a96800
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 24 16:53:31 2023 +0000

    Add unit test for transaction_entry

commit 2b397c9
Author: Bronek Kozicki <[email protected]>
Date:   Tue Oct 24 15:45:41 2023 +0000

    Add small APIv2 unit test for subscribe

commit 4d517e2
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 23 15:27:35 2023 +0000

    Store close_time_iso in API v2 output

commit 910f125
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 18 17:24:42 2023 +0100

    Output from tx

commit 0fb0517
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 18 17:24:26 2023 +0100

    Output from transaction_entry

commit e4ce8b1
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 18 17:24:08 2023 +0100

    Output from account_tx

commit 92635aa
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 18 17:23:49 2023 +0100

    Output from ledger

commit 76eee5a
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 18 17:23:16 2023 +0100

    Output from sign, submit etc.

commit aa6de60
Author: Bronek Kozicki <[email protected]>
Date:   Mon Oct 23 18:20:21 2023 +0000

    Output for subscriptions

commit ee53c5d
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 25 18:17:10 2023 +0000

    Formatting fix

commit 65346dc
Author: Bronek Kozicki <[email protected]>
Date:   Wed Oct 25 18:07:18 2023 +0000

    Remove include <ranges>
@intelliot intelliot moved this from 🏗 In progress to Ready to release in Core Ledger Jan 8, 2024
sophiax851 pushed a commit to sophiax851/rippled that referenced this issue Jun 12, 2024
Remove `tx_history` and `ledger_header` methods from API version 2.

Update `RPC::Handler` to allow for methods (or method implementations)
to be API version specific. This partially resolves XRPLF#4727. We can now
store multiple handlers with the same name, as long as they belong to
different (non-overlapping) API versions. This necessarily impacts the
handler lookup algorithm and its complexity; however, there is no
performance loss on x86_64 architecture, and only minimal performance
loss on arm64 (around 10ns). This design change gives us extra
flexibility evolving the API in the future, including other parts of
XRPLF#4727.

In API version 2, `tx_history` and `ledger_header` are no longer
recognised; if they are called, `rippled` will return error
`unknownCmd`

Resolve XRPLF#3638

Resolve XRPLF#3539
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Clio Reviewed Feature Request Used to indicate requests to add new features
Projects
Archived in project
4 participants