Update ledger command to accept string as ledger_index

[carlhua]

Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well.

[gregtatcam]

The ledger command does take strings: current, closed, validated. Here is the doc: https://xrpl.org/ledger.html. And the code handling the parsing: https:/ripple/rippled/blob/develop/src/ripple/net/impl/RPCCall.cpp#L102.

[cjcobb23]

@gregtatcam I think we want to be able to pass strings as numbers, like:

"ledger_index" : "2"

Currently, this errors out. You have to do this:

"ledger_index" : 2

[gregtatcam]

Got it, thanks.

[scottschurr]

@carlhua, could you clarify the bug report? As @gregtatcam reported the three usual strings ("current", "closed", and "validated") are supported. As far as I can see the documentation does not say that, for example, "5" would be an acceptable replacement for 5.

Do you want to extend the current functionality to include support for "5"? Would that be a feature request, not a bug report?

Just looking for clarification. Thanks.

[mDuo13]

The main reason I think we'd want to extend the API to handle string integers for ledger indexes as input is that the API in some cases outputs them in that format. The validations subscription stream for example returns ledger indexes as strings. (The ledger and ledger_data commands do too, when representing ledger headers.) Example response from the ledger command:

{
  "id": 14,
  "result": {
    "ledger": {
      "accepted": true,
      "account_hash": "696E78E40668C722CC3BA25002595F4B809FFE96B6A7F6A7EAC1C88B653CE475",
      "close_flags": 0,
      "close_time": 651093681,
      "close_time_human": "2020-Aug-18 19:21:21.000000000 UTC",
      "close_time_resolution": 10,
      "closed": true,
      "hash": "7907E089DCE916EE490073EF2E07C2B214F4BD9B762606EB624BA502916F25CE",
      "ledger_hash": "7907E089DCE916EE490073EF2E07C2B214F4BD9B762606EB624BA502916F25CE",
      "ledger_index": "57596705",
      "parent_close_time": 651093680,
      "parent_hash": "BED01C6D5115996A1B584340154712959E91FD67D2D14D34E5B48861F4B2655E",
      "seqNum": "57596705",
      "totalCoins": "99990895180923243",
      "total_coins": "99990895180923243",
      "transaction_hash": "2467C3F33BCE1C7B74CBDA10AD748FEEB235FA1B122603D16D60D73EAD7E705A"
    },
    "ledger_hash": "7907E089DCE916EE490073EF2E07C2B214F4BD9B762606EB624BA502916F25CE",
    "ledger_index": 57596705,
    "validated": true
  },
  "status": "success",
  "type": "response"
}

(Note the result.ledger.ledger_index is a string while result.ledger_index is a number. Ugh.)

I think it makes sense to support copying the ledger_index from this response directly into follow-up queries. As of today, you have to remember to convert the data type first.

I would like to change the API to always use the same data type for ledger_index but that definitely is a breaking change, whereas supporting an additional input type is not. And there's no reason we couldn't do both: there shouldn't be any parsing ambiguity between ledger indexes as quoted integers vs the shortcut strings, so this is a case where we can relieve the user of the cognitive load of remembering which type to use, at little cost to the server.

An ancillary benefit is that string numbers would be immune from JSON precision worries if we ever migrated to 64-bit ledger indexes. But then again, that would be such a fundamental change that we probably shouldn't worry about it. After about 8 years of ledger closes at the current rate, we've iterated through ~1.3% of the 32-bit space (per my quick calculations) so unless the rate of ledgers over time dramatically increases, we're centuries away from needing to make that change. If JSON precision worries are still a problem by then, I'll be very disappointed in our future.