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

Inconsistent time type usage #1178

Open
leighmcculloch opened this issue Nov 5, 2023 · 3 comments · May be fixed by #1282
Open

Inconsistent time type usage #1178

leighmcculloch opened this issue Nov 5, 2023 · 3 comments · May be fixed by #1282
Labels
bug Something isn't working

Comments

@leighmcculloch
Copy link
Member

leighmcculloch commented Nov 5, 2023

What version are you using?

2674d86

What did you do?

Used get_ledger_timestamp to get timestamp.

{
"export": "4",
"name": "get_ledger_timestamp",
"args": [],
"return": "U64Val",
"docs": "Return the timestamp number of the current ledger as a u64."
},

What did you expect to see?

A Timepoint.

What did you see instead?

A u64.

Discussion

We seem to have these two types Timepoint and Duration that get exposed all the way up in the SDK, however they don't seem to get used anywhere. Even the places in the host interface where one might think a time would be exposed like the get_ledger_timestamp function, they just use a plain u64.

What are Timepoint and Duration for? Should we remove them if they aren't in use? Or are there places that aren't using them should be using them?

History

It looks like timepoint and duration were added in:

In response to this issue:

According to this comment it sounds like the intent was to use Timepoint for the ledger timestamp:

They can represent for example ledger timestamp and can specify its own display format

Ref: #866 (comment)

@jayz22 @graydon What should we do here?

@leighmcculloch leighmcculloch added the bug Something isn't working label Nov 5, 2023
@leighmcculloch leighmcculloch changed the title Inconsistent time types Inconsistent time type usage Nov 5, 2023
@graydon
Copy link
Contributor

graydon commented Nov 6, 2023

"What they're for" is explained in the comment you linked to: #866 (comment), they're numbers with absolute and relative time semantics implied (for formatting and, presumably, to dangle datetime-arithmetic functions on at the SDK level). They exist in the XDR already, we're just not returning the right type here. We should change the host interface.

@graydon
Copy link
Contributor

graydon commented Feb 14, 2024

Can the SDK move ahead with providing Timepoint to users? The env change doesn't seem to me a particularly high priority, is it? (We can of course change at some point, I am just trying to figure priority)

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Feb 28, 2024

It could but it'd require adding more host function calls. The SDK would get a u64, then need to convert it into a Timepoint, then give someone that Timepoint object. So we could do it in the SDK now, and do later in Env if the additional host fn call is bothering folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@graydon @leighmcculloch and others