Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strategy to avoid "INVALID_TIMESTAMP" #80

Closed
bennycode opened this issue Apr 2, 2018 · 15 comments
Closed

Strategy to avoid "INVALID_TIMESTAMP" #80

bennycode opened this issue Apr 2, 2018 · 15 comments

Comments

@bennycode
Copy link
Contributor

Binance is pretty strict about the timestamp which is sent to the server on every request.

To avoid the "Request used an outdated timestamp" error, we could sync our own time with an online atomic clock service to determine our clock drift (which can then be used to adjust the timestamps).

@balthazar
Copy link
Member

Isn't the useServerTime option supposed to solve this issue?

@bennycode
Copy link
Contributor Author

@balthazar Good point! Haven't checked this one. 🙈

I was already investigating on how to determine the local clock drift by using Sntp. My idea was to detect the offset in milliseconds to then add it to all locally created timestamps:

const Sntp = require('sntp');

Sntp.time({
  host: 'pool.ntp.org',
  port: 123,
  resolveReference: true,
  timeout: 1000
})
.then((time) => {
  const offset = parseInt(time.t, 10);
  const timestamp = Date.now() + offset;
  console.log(`Offset: ${offset}ms`);
  console.log(`Adjusted timestamp: ${timestamp}`);
});

I will test the useServerTime option tonight and report back later!

@balthazar
Copy link
Member

Using sntp might worsen the compatibility of the library in the browser, which I think is still not ideal as someone using webpack previously reported.

I think the param is the way to go, but maybe it would be better to have it at the client level rather than a per-call basis

@bennycode
Copy link
Contributor Author

Yeah, we shouldn't break Browser compatibility, so I debugged further...

The INVALID_TIMESTAMP error (code -1021) pops up when calling client.prices.

The prices endpoint is a public one but useServerTime only seems to be implemented for endpoints which require authentication?!

My idea was to inject the timestamp offset from the calling application...

Demo Code:

const Binance = require('binance-api-node').default
const Sntp = require('sntp');

const client = Binance({
  apiKey: process.env.BINANCE_API_KEY,
  apiSecret: process.env.BINANCE_API_SECRET,
});

Sntp.time({
  host: 'pool.ntp.org',
  port: 123,
  resolveReference: true,
  timeout: 1000
})
.then((time) => {
  const offset = parseInt(time.t, 10);
  return client.prices({ timeOffset: offset });
});

This way we wouldn't need a dependency to Sntp within binance-api-node.

Do you maybe have another idea of how we can overcome the timestamp issue? Because the environment, where I am running "binance-api-node", has an offset of -2200ms and I cannot change the clock on that machine. :-/

@bennycode
Copy link
Contributor Author

bennycode commented Apr 5, 2018

I just had another idea...

Would it be possible to send the time offset already with the constructor?

Code:

const Binance = require('binance-api-node').default

const client = Binance({
  apiKey: process.env.BINANCE_API_KEY,
  apiSecret: process.env.BINANCE_API_SECRET,
  clockDrift: -2200 // 2200ms in the past
});

Edit: Because the time drift can change over time, it should also be possible to change clockDrift after instantiation. The idea is to catch an INVALID_TIMESTAMP error from the using application to then adjust the clock drift and reschedule the initial request.

@bennycode bennycode changed the title Strategy to avoid "Request used an outdated timestamp" Strategy to avoid "INVALID_TIMESTAMP" Apr 5, 2018
@balthazar
Copy link
Member

Instead of a constant like this, I think having a getter getTime would be better, where you can update the drift once it changed

@bennycode
Copy link
Contributor Author

Ok, I am fine with that as long as it is possible to update the clock drift during runtime. Can you please make a code proposal so that I can work on implementing that?

@balthazar
Copy link
Member

balthazar commented Apr 6, 2018

Following the docs, it looked to me that timestamp was only a parameter available for signed endpoints (I also remember trying having one for the public calls and got errors), are you saying it can also be provided for public routes too now? In that case, wouldn't having useServerTime available for all endpoints good?

The implementation would require to pass a getter in the client, that would be called instead of this

@bennycode
Copy link
Contributor Author

@balthazar What was the reason that useServerTime has been only implemented for signed endpoints? I would need it also to query public endpoints like prices. If this could be done, that would be perfect!

We could also make useServerTime to accept a boolean value or a function. Boolean value would turn on to query the server time from Binance and a function would act as callback parameter that could be use for custom time updates. What do you think?

@balthazar
Copy link
Member

I tried to specify a timestamp for public endpoints at some points and hit some weird issue, which was probably something else and thought that providing a timestamp was not available for public ones. If this is untrue, we can add it to public endpoints too.

I'm not really confident with having this confusing API, I would prefer having another parameter for the custom getTime function

@bennycode
Copy link
Contributor Author

Hey @balthazar, it has been some time and I would really like to be able to pass a function to "binance-api-node" which is used to calculate the "timestamp" which is sent to Binance.

What would be the easiest to achieve this? Can you give me some pseudo code to point me in the correct direction?

@balthazar
Copy link
Member

You would pass a getTime to the opts along with the apiKey that would return the time. Before that could you make sure that public endpoints do accept the timestamp extra parameter?

@emmtte
Copy link

emmtte commented Jul 10, 2018

Any news about this issue?
Have this error sometimes

(node:814) UnhandledPromiseRejectionWarning: Error: Timestamp for this request was 1000ms ahead of the server's time.

@bitcoinvsalts
Copy link

Any solution for this issue?

@balthazar
Copy link
Member

Closing thanks to #162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants