XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.5k stars 1.45k forks source link

Suggestion: Change "index" to "LedgerEntryID" for ledger objects #3649

Open mDuo13 opened 3 years ago

mDuo13 commented 3 years ago

Summary

Change the fields index and LedgerIndex to LedgerEntryID to reduce confusion about what a "ledger index" means.

Motivation

"Index" is one of the most confusing terms in understanding the XRP Ledger:

The ledger_index and LedgerIndex fields often appear very close to one another in a single API response despite having different data types and referring to totally different things. Example:

API response with "ledger_index" and "LedgerIndex" 6 lines apart

Meanwhile, in the most egregious cases (e.g. the ledger command with "full": true) a single API response may use both LedgerIndex and index to refer to the same thing, on top of using ledger_index to mean something different.

As an aside, a "transaction index" and a "transaction sequence" are unrelated things and neither of them is the transaction's ID:

Term Transaction Ledger
Sequence The account sequence number this transaction consumed. The sequence number of the ledger. (Deprecated usage.)
Index The order a transaction was executed within a ledger. The ID of an object within the ledger; or the sequence number of the ledger.

Solution

We can change the names of fields in the JSON formats without changing those fields' canonical binary meanings or values. I suggest making the following changes in APIv2 or later:

Old name New Name Used In
index LedgerEntryID Serialization of ledger entry objects, including from API responses from account_info, account_objects, ledger (in the state data returned with accounts: true or full: true), ledger_data¹, ledger_entry, and book_offers.
LedgerIndex LedgerEntryID Serialization of transaction metadata in all places that occurs.
index² entry_id In ledger_entry requests

Some relevant source code links:

¹ In the case of ledger_data with "binary": true, it may be aesthetically displeasing to see lowercase data next to upper-CamelCase LedgerEntryID but I think it's OK. I would also accept entry_id there. ² The field index is a shared definition with index as used in the download_shard request to refer to a Shard Index. I suggest leaving download_shard's usage of index unchanged or perhaps changing it to shard_index.

Paths Not Taken

cjcobb23 commented 3 years ago

What about using key instead of id? We could use LedgerEntryKey, or something like that. key-value is a widely used terminology, is easily understood and accurately describes what's going on here. The ledger is just a map, and maps use the key-value terminology; in the code, we call the ledger a "state map". We also use key in the codebase in various places, as well as Keylet: https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/app/ledger/Ledger.h#L176 https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/app/ledger/Ledger.h#L198 https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/app/ledger/Ledger.h#L173

https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/protocol/Keylet.h#L43 https://github.com/ripple/rippled/blob/0b4e34b03b72196510998375fd28622c8b63aaa7/src/ripple/ledger/ReadView.h#L198

I find the use of key to be clearer than index or id.

mDuo13 commented 3 years ago

"Key" kind of makes sense but I worry it would be confused for a cryptographic key...

intelliot commented 3 years ago

I like both options. Either would be a good improvement over the status quo. Just for fun, let's try taking a (very) informal poll right here.

Term Reaction
LedgerEntryID ❤️
LedgerEntryKey 🚀

React to this comment with the emoji of your preferred term.

sublimator commented 3 years ago

AccountStateIndex

sublimator commented 3 years ago

LedgerEntryID Yeah ^