bitfinexcom / bitfinex-api-node

BITFINEX NodeJS trading API - Bitcoin, Litecoin, and Ether exchange
https://www.bitfinex.com/
MIT License
461 stars 214 forks source link

Bug: public funding trades mapping is wrong. #555

Closed michael34435 closed 2 years ago

michael34435 commented 4 years ago

Issue type

Brief description

Public funding trade mapping is wrong. There is no maker column in public funding trades.

Steps to reproduce

const BFX = require('bitfinex-api-node');
const bfx = new BFX(
  {
    apiKey: 'xxxxx',
    apiSecret: 'xxxxx',
    ws: {
      autoReconnect: true,
    },
  },
);
const ws = bfx.ws(2, { transform: true });

ws.once('open', () => {
  ws.subscribeTrades(`fUSD`);
  ws.on('trades', console.log);
});

ws.open();

output:

fUSD [
  {
    id: 161367901,
    symbol: 1594289406000,
    mtsCreate: -51.4457011,
    offerID: 0.00020119,
    amount: 3,
    rate: undefined,
    period: undefined,
    maker: undefined
  }
]
Additional Notes:

I had found the same issue in 4.0.2 - 4.0.15 ;(

crazyguitar commented 4 years ago

Hi,

I fixed this issue and am waiting for reviewing now.

iccicci commented 2 years ago

Hi all, (and @crazyguitar )

I can read comment about 1 year and half ago stating the issue was fixed, but in v5.0.3 the issue is still alive.

Could I please have an estimation of when the fix will be merged and published?

Thank you

vigan-abd commented 2 years ago

Hi @iccicci @crazyguitar , we'll take a look at the issue asap, thanks for the info

iccicci commented 2 years ago

Hi @vigan-abd,

I've checked the funding_trade.js file in the bfx-api-node-models package:

  1. the const fields mapping respects exactly what the API documentation says;
  2. with a console.log in the unserialize method, I saw the data is [ [ 267665633, 1644877838000, -0.1, 0.00002297, 2 ] ] (I would say: ID, MTS_CREATE, AMOUNT, RATE, PERIOD);

I also checked the ws2.js file in this package, and it seems the set of data is not configurable; it is as is when it arrives from the websocket: I'm afraid this needs to be escalated on BitFinex

ghost commented 2 years ago

There is a simple fix: to use PublicTrade instead of FundingTrade, since it is already designed for this, but this will be a breaking change

Public Funding trade has format

 [
    ID,
    MTS,
    AMOUNT,
    RATE,
    PERIOD
  ]

while finding trades

    [
    ID,
    CURRENCY,
    MTS_CREATE,
    OFFER_ID,
    AMOUNT,
    RATE,
    PERIOD,
    MAKER
  ]

and as far as I found out correctly, _handleTradeMessage receives only data from public trades channel, so FundingTrade should not be used in this case

@vigan-abd wdyt?

ghost commented 2 years ago

fixed in https://github.com/bitfinexcom/bitfinex-api-node/pull/578

vigan-abd commented 2 years ago

thanks @vitaliystoliarovcc for investigating and providing a fix :) @iccicci can you confirm that we solved the issue so we can close the ticket? latest package release that includes the fix is https://www.npmjs.com/package/bitfinex-api-node/v/5.0.4

iccicci commented 2 years ago

Sorry for my late reply @vigan-abd , I'm so busy in this period... Yes, it seems now the fields are correct. Thank you

vigan-abd commented 2 years ago

thanks for confirming @iccicci, we're glad we could help :)