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 helper for fetching timebounds #249

Merged
merged 15 commits into from
Mar 18, 2019
Merged

Add helper for fetching timebounds #249

merged 15 commits into from
Mar 18, 2019

Conversation

morleyzhi
Copy link
Contributor

@morleyzhi morleyzhi commented Mar 13, 2019

Addresses #232.

(1) Add an axios interceptor that tracks the server's reported time from headers
(2) Add a function, ServerSdk.Server.prototype.fetchTimebounds(seconds: Number), that returns a timebounds object for TransactionBuilder based on the server time. If no server time is recorded locally, or the recorded time is older than 5 minutes, it'll attempt to go to the network; if THAT fails, use the local time.

@bartekn
Copy link
Contributor

bartekn commented Mar 14, 2019

The idea is great but I'm wondering if Horizon server is the best place to check the time. There is no guarantee that it has a correct time set (all timestamps during ingestion are actually taken from ledger headers so Horizon can work correctly even if time is set incorrectly). I think we should probably use Stellar-Core server time and expose it in Horizon root. Unfortunately, it doesn't seem to be exposed currently.

@morleyzhi
Copy link
Contributor Author

@bartekn That does sound better. How much does Horizon's timestamp vary?

IMO this feature only has to be better than user local time to be worth it, and user local time is a complete mess. I would guess it's right about 90% of time. If Horizon is right 91+% of the time, this seems better, and we can change the underlying data source to a 99% correct source in a later update.

@bartekn
Copy link
Contributor

bartekn commented Mar 14, 2019

How much does Horizon's timestamp vary?

What I wanted to say is that it'll be probably fine in most cases however it's not guaranteed whereas in core, if the timestamp is not correct, it will simply go out of sync with the network.

This is probably good as the first iteration but the final solution should use Stellar-Core's time, imho.

@morleyzhi
Copy link
Contributor Author

Yeah, I agree this should use Core within one or two releases.

@tomquisel
Copy link
Contributor

My take is that running a server with incorrect time and expecting services on it to work is pretty unrealistic. It's cool that Horizon (maybe) works now with a clock that's off, but it's safe to make it a requirement that the clock be correct (at least to within 1 sec). So I'd just leave it as is.

Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just minor comments.

src/horizon_axios_client.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
src/server.js Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
@morleyzhi morleyzhi merged commit 6cc54eb into stellar:master Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants