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.

Sign up for GitHub

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

Jump to bottom

services/horizon: expose date header #2316

Merged
merged 1 commit into from
Feb 26, 2020
Merged

services/horizon: expose date header #2316

merged 1 commit into from
Feb 26, 2020

Conversation

tomerweller
Copy link
Contributor

@tomerweller tomerweller commented Feb 22, 2020 •
edited
Loading

closes #2315
do not merge yet

@cla-bot cla-bot bot added the cla: yes label Feb 22, 2020
@tomerweller
Copy link
Contributor Author

I've tested and rebased

leighmcculloch
leighmcculloch approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

🎉 I think it is good for us to give Horizon clients visibility into what time that Horizon says the data was accurate at, which is all this field will do accurately.

@tomerweller In #2315 you mentioned:

Horizon clients might be interested in obtaining an accurate timestamp.

I don't think clients should use Horizon as a time server which the statement could be interpreted to mean. Do we have any plans like that, or is the intended use just what I stated at the top?

@tomerweller
Copy link
Contributor Author

The JS sdk already uses horizon server time but this functionality doesn't work inside of the browser due to Date not being exposed to CORS clients.

I definitely don't think that horizon should be used as a time server but if a client is already communicating with a horizon server I don't find it offensive to use that value as a sanity test.

ire-and-curses
ire-and-curses approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

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

LGTM

@tomerweller tomerweller merged commit 142df5b into stellar:master Feb 26, 2020
@tomerweller tomerweller deleted the horizon-date-header branch February 26, 2020 01:13
@abuiles abuiles mentioned this pull request Mar 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ire-and-curses ire-and-curses ire-and-curses approved these changes

@leighmcculloch leighmcculloch leighmcculloch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

horizon: expose date header to CORS clients
3 participants
@tomerweller @leighmcculloch @ire-and-curses