Incorrect return value for getOrderbook method

tequdev commented 1 year ago

The return value of the getOrderbook method has a buy field and a sell field, but in the following, TakerGets = XRP and TakerGets = USD are mixed in the buy.

const { Client } = require('xrpl');
const client = new Client('wss://xrpl.ws');

const main = async () => {
  await client.connect();
  const orderbook = await client.getOrderbook(
    { issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B', currency: 'USD' },
    { currency: 'XRP' },
    { limit: 10, ledger_index: 78070258 }
  );
  console.log(orderbook);
};

main();

{
  buy: [
    ...
    {
      Account: 'rsFaWztpAqZKoBSLTuhrhYfBV4nwYg7TFP',
      BookDirectory: 'DFA3B6DDAB58C7E8E5D944E736DA4B7046C30E4F460FD9DE4E0DCE23664B3487',
      BookNode: '0',
      Flags: 0,
      LedgerEntryType: 'Offer',
      OwnerNode: '0',
      PreviousTxnID: 'F11D96C1BFF8DE0E6A731460747B97E77C453C1D874FA4ECC0186FFC3F0A6D7E',
      PreviousTxnLgrSeq: 78017755,
      Sequence: 75621130,
      TakerGets: '5236750',
      TakerPays: [Object],
      index: '5BF1EBD7C7DF6729C4EB6F3E8A12EE4721563F67E6DB29ACD1E81A38B9569111',
      quality: '0.0000003885826132620423'
    },
    {
      Account: 'rsgi7ENscrVaXC44JE2m94XzKQrmAVX2gV',
      BookDirectory: '4627DFFCFF8B5A265EDBD8AE8C14A52325DBFEDAF4F5C32E5B096C6EFAA91865',
      BookNode: '0',
      Expiration: 762310783,
      Flags: 0,
      LedgerEntryType: 'Offer',
      OwnerNode: '0',
      PreviousTxnID: '2B90439DA0A732E3CB1EF6E0D6DD8E39D378E17B750C7ED595B98798EACE097A',
      PreviousTxnLgrSeq: 78069539,
      Sequence: 72701643,
      TakerGets: [Object],
      TakerPays: '121568490',
      index: 'EC40BD275BF22AA9C40F024EDF6A642ABD3D73A4AA8CD7313330C45C6C4DA6EA',
      owner_funds: '45.79250358526066',
      quality: '2652498.697984101',
      taker_gets_funded: [Object],
      taker_pays_funded: '121464556'
    }
  ],
  sell: [
    ...
  ]
}

This is because lsfSell is used to determine buy/sell, which is a flag that trades the full amount of TakerGets if there is a better rate than the rate specified when the OfferCreate transaction is sent. should not be used.

https://github.com/XRPLF/xrpl.js/blob/5d34746f1241cc17d5ad08e0c1d6aa63d6743e42/packages/xrpl/src/sugar/getOrderbook.ts#L135

Also, the number of objects in the buy and sell fields is obviously different when limit is specified in option. In the following example, limit:10 is specified, but there is only one object in the sell field.

I think this is also due to the fact that lsfSell is used to make the decision.

pdp2121 commented 1 year ago

Thanks @develoQ for raising the issue! We are examining it at the moment, and will comment here once a solution has been found.

ckniffen commented 1 year ago

@develoQ This seems to be a question of intent vs action. The code calls the order_book twice, once in either direction (the cause of there being more than 10 results). I was not expecting this behavior based on the names of the parameters and the documentation which implies only a single direction is requested.

I investigated the behavior of ripple-lib, which this method was supposed to be based on, to derive intent. While it also called the order_book api twice it did much more parsing and took into account the direction and to normalize all offers. It then produced a single array.

This comes down to whether or not to update the docs (and parameters) to match the implementation or create a breaking change to break out the offers differently (or not at all).

Thoughts? @intelliot @mvadari @mukulljangid

Code snippets:

current implementation: https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/src/sugar/getOrderbook.ts#L105-L149
ripple-lib: https://github.com/XRPLF/xrpl.js/blob/1.x/src/ledger/orderbook.ts#L103-L126
ripple-lib parsing: https://github.com/XRPLF/xrpl.js/blob/1.x/src/ledger/parse/orderbook-order.ts#L24-L63

XRPLF / xrpl.js

Incorrect return value for getOrderbook method #2225