stellar / rs-soroban-sdk

Rust SDK for Soroban contracts.
Apache License 2.0
126 stars 67 forks source link

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

Closed leighmcculloch closed 10 months ago

leighmcculloch commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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).