XRPLF / xrpl.js

A JavaScript/TypeScript API for interacting with the XRP Ledger in Node.js and the browser
https://xrpl.org/
1.2k stars 512 forks source link

GetLedger does not include ledgerHash option #979

Closed alexchiriac closed 5 years ago

alexchiriac commented 5 years ago

Hi!

Another minor thing that I thought would be very useful is to include a ledgerHash option for the getLedger method.

In the Specifying Ledgers section of the rippled docs, it's stated that if both ledger_hash and ledger_index are included in the request, ledger_hash will be considered first. Then the additional field wouldn't clash with the current logic for the request in ledger.ts of taking 'validated` by default:

{
    ledger_index: options.ledgerVersion || 'validated'
    expand: options.includeAllData,
    transactions: options.includeTransactions,
    accounts: options.includeState
}

The new rippled request would look like this:

{
    ledger_hash: options.ledgerHash,
    ledger_index: options.ledgerVersion || 'validated',
    expand: options.includeAllData,
    transactions: options.includeTransactions,
    accounts: options.includeState
}

If the change is welcome, then I can submit a PR as I've already worked on the fix!

intelliot commented 5 years ago

Sounds good to me. Out of curiosity, what's your use case for this? I usually refer to ledgers by index.

alexchiriac commented 5 years ago

Thanks for the reply! On the XRP Ledger, I never refer to hashes either. My use case is a multiple chain block explorer where users can search for an address/transaction hash/block number/block hash.

I'm thinking that if a user enters an integer in the search box to search for a block/ledger, he might get back multiple results from different chains. I would like to allow the option for block hashes to make the search more deterministic.

intelliot commented 5 years ago

👍Sounds like a good addition!

intelliot commented 5 years ago

Released in ripple-lib 1.1.2 (2018-12-12)