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

Add support for custom getTime #162

Merged
merged 5 commits into from
Mar 5, 2019
Merged

Add support for custom getTime #162

merged 5 commits into from
Mar 5, 2019

Conversation

barhun
Copy link
Contributor

@barhun barhun commented Mar 5, 2019

This support for the optional field timeOffset allows consumers of the API to avoid unnecessary calls to the /api/v1/time endpoint for each private call. One can call the time method only once at startup and then calculate an offset between the local time and the returned value which, if provided as an option, will be used for any future private call.

@barhun barhun changed the title Add support for the optional field 'timeOffset' for private calls Add support for the optional field 'timeOffset' to be used in private calls Mar 5, 2019
@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage decreased (-26.4%) to 64.0% when pulling 2174ba2 on barhun:master into aa54224 on HyperCubeProject:master.

@balthazar
Copy link
Member

Thanks! Rather than having a timeOffset, I think it would be better to instead allow a custom function to be passed, returning the time integer. If not provided it would simply be: () => Date.now() and we resolve the promise with this value. This would allow for more complex operations to be made if we need it

What do you think?

@barhun
Copy link
Contributor Author

barhun commented Mar 5, 2019

Well, indeed, it would be a more generic solution. Yet, since both the server time and the local time are monotonically increasing and the offset between them is always a constant value, this one also appears to solve almost all cases.

Now, I think providing a function returning a promise would be the best choice as it would also let us later remove the option useServerTime at some point in future. One could say Binance({apiKey, apiSecret, timePromise: Binance().time}) instead of using useServerTime: true, but we need to keep it for now 'cause backward-compatibility must be conserved. For the ones who want to make use of an offset like me, it would be something similar to the following: Binance({apiKey, apiSecret, timePromise: () => Promise.resolve(Date.now() - timeOffset)})

This promise-based solution will allow people to query their own time server if they ever need to, thus seems a great solution. Deal? (:

@balthazar
Copy link
Member

useServerTime is a convenient way for people to get the time added to the request without having to do it on their side, I don't really see it going away.

Because the underlying code is using Promise.resolve, we do not need to force using a promise with the library, although it is possible for example:

getTime: () => Date.now() + TIME_OFFSET

getTime: async () => {
  const time = await fetch('mycustom-ntpserver.com')
  return time + TIME_OFFSET
}

This gives the most customization capabilities while keeping it simple

@barhun barhun closed this Mar 5, 2019
@barhun barhun reopened this Mar 5, 2019
@balthazar
Copy link
Member

Looks good! Do you mind putting the default getTime function out of the privateCall so it's not being recreated everytime? Merging after this!

Also about typings, will it be an issue if the provided getTime returns a promise?

@balthazar
Copy link
Member

Almost perfect :D Rather than having the ternary checking the typeof, why not keep it a default like you were doing? getTime = defaultGetTime?

@barhun
Copy link
Contributor Author

barhun commented Mar 5, 2019

'cause NOT everyone uses TypeScript. (: Just kidding, pushing it now one last time! Thanks for all your feedbacks.

@balthazar
Copy link
Member

❤️

@balthazar balthazar merged commit c92bafb into ViewBlock:master Mar 5, 2019
@balthazar balthazar changed the title Add support for the optional field 'timeOffset' to be used in private calls Add support for custom getTime Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants