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

Timepoint and Duration types are difficult to construct in tests #1197

Closed
leighmcculloch opened this issue Jan 7, 2024 · 5 comments · Fixed by #1201
Closed

Timepoint and Duration types are difficult to construct in tests #1197

leighmcculloch opened this issue Jan 7, 2024 · 5 comments · Fixed by #1201
Assignees
Labels
bug Something isn't working

Comments

@leighmcculloch
Copy link
Member

What version are you using?

v20.0.3

What did you do?

Tried to create a Timepoint or Duration using functions on the tests.

What did you expect to see?

Some way to do that.

What did you see instead?

No way to do that.

Other than building the XDR and converting that way:

use soroban_sdk::{xdr, IntoVal, Timepoint};

let t: Timepoint = xdr::ScVal::Timepoint(xdr::TimePoint(1)).into_val(&e);
@wildework
Copy link

Thanks for adding this! I personally didn't even think to use XDR to initialize a Timepoint value. If it's a UNIX timestamp (as you can find mentions of in some random places in the repositories) then I would expect, as a consumer of the SDK, that the Env should have a function to return a valid Timepoint.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jan 7, 2024

All the SDK types, even custom types, are constructible via XDR ScVal's and convertible using into_val. The XDR conversions support other functionality like type generation in fuzz tests, etc. But it isn't intended to be the only way to construct types, and not intended to be the default way folks in tests or contracts to construct values.

@leighmcculloch
Copy link
Member Author

There are already private functions on Timepoint and Duration named from_u64, but I think it would be helpful if we add public functions with names that communicate intent better. I'm thinking of APIs like this:

impl Duration {
    pub fn from_secs(secs: u64) {

which is the same as: https://doc.rust-lang.org/std/time/struct.Duration.html#method.from_secs

impl Timepoint {
    pub fn from_unix(secs: u64) {

which doesn't mirror the Rust stdlib because the way the Rust stdlib supports creating unix times is not particularly user friendly (see this)and given that unix time is the main way to represent time on Stellar I think we need something friendly.

@wildework
Copy link

wildework commented Jan 7, 2024

Definitely on board for from_secs even though I do think from_seconds would be more accessible. I think from_unix is good.

What about something like this in Env to return the unix timestamp of the block it's executing in.

impl Env {
    pub fn now() -> Timepoint {

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jan 7, 2024

It is possible to get a now-like Timepoint out of the env already with the following code. The reason for not providing now specifically is intentional because it could be misleading. The environment is only able to provide one time, which is the ledger close time, and so all calls to now would continue to return the same value within a single invocation.

env.ledger().timestamp()

Unfortunately the timestamp function returns a u64 though, not a Timepoint. That's because the host environment's get_ledger_timestamp function returns the value as a U64Val, not a TimepointVal (ref).

Timepoint and Duration haven't really been adopted by the Host environment either... and that has the run on affect of the types being difficult to adopt in contracts. If the SDK is going to encourage their use the host environment should probably too. We might be able to add a get_ledger_timestamp_timepoint in the future and switch the SDK to using that (cc @graydon).

github-merge-queue bot pushed a commit that referenced this issue Jan 17, 2024
### What
Export functions for creating and accessing Timepoint and Duration.

### Why
The types are not creatable inside a contract, and outside a contract in
tests are only creatable using the XDR ScVal type. If developers are to
use the Timepoint and Duration types, they need to be able to construct
the types from primitive values.

The `Timepoint` `from_unix` and `to_unix` functions were added using the
unix terminologgy because that's the type of time values the Stellar
network uses.

The `Duration` `from_seconds` and `to_seconds` functions were added
using the seconds terminology because it seemed like the most direct way
to communicate intent. I originally intended to use `secs` which I think
would also be reasonable and feedback on the issue suggested `seconds`
which also seems reasonable to me.

While for both types functions were repurposed/removed, those functions
were private only and should have no impact on compatibility.

Close #1197
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.

2 participants