Open gefjon opened 1 month ago
Having discussed this more with the team, there's an additional wrinkle: while C#'s DateTime
is semantically correct as a translation for AlgebraicType::timestamp()
, use of that type is fraught because of its locale-dependent printing. I have been encouraged to prefer DateTimeOffset
, which maintains locale-independent printing by encoding a time zone, but that's no good because we don't store the time zone in AlgebraicType::timestamp()
and so would have to discard it when serializing. We would still be representing the same point in time, but it would be, say, 12:00 UTC rather than 7:00 EST. Implicitly losing information in this way seems undesirable.
My disposition is to avoid these tough questions by retaining spacetimedb::Timestamp
as a user-facing type (adding it to the client SDKs) and providing methods on it to convert to and from SystemTime
or DateTimeOffset
.
I'm also interested in providing a special SATS type analogous to Rust Duration
or C# TimeSpan
, for storage in ScheduleAt::Interval
. I'm not sure what to name this type - I would prefer not to use either Duration
or TimeSpan
to avoid confusion when writing Rust and C# respectively. I'm considering TimeDuration
, which is redundant but clear and unambiguous.
TODO: Switch back to micros: u64
, rather than nanos: i64
. The latter is likely more correct, but the former is BSATN- and BFLATN-compatible with our old code, which is more valuable than the incremental improvement in precision or the ability to represent negative durations or pre-Unix timestamps.
EDIT: micros: i64
, but otherwise done.
Description of Changes
AlgebraicType
.Timestamp
to the SATS crate, where previously it was in bothbindings
andclient-api-messages
.lib
, which seems wrong to me.Timestamp
rather thanu64
.Timestamp
storemicros_since_unix_epoch: i64
, where previously it hadmicros_since_unix_epoch: u64
.SystemTime
and C#DateTime(Offset)
can do.TimeDuration
, analogous toDuration
andTimeSpan
, as another new "special"AlgebraicType
.TimeDuration
ismicros: i64
, to matchTimestamp
.TimeSpan
but Rust has unsignedDuration
.ScheduleAt
storesTimestamp
orTimeDuration
in its two variants.ScheduleAt::Interval
means arguably we want unsignedTimeDuration
, as a negative interval seems meaningless.ScheduleAt::Interval(-10 minutes)
is the same asScheduleAt::Interval(10 minutes)
.ScheduleAt
s viaDuration::into
, I hope.u64
s.Companion PRs:
Still TODO:
client-api-messages
bindings.API and ABI breaking changes
Timestamp
type (e.g. inReducerContext
) is now defined in the SATS crate, re-exported from lib and bindings, and has slightly different methods.Timestamp::now
is deprecated and stubbed in the WASM target.Timestamp
is changed fromU64
toProduct { __timestamp_micros_since_unix_epoch__: I64 }
. This is layout-equivalent in BSATN and I think BFLATN, but changes the JSON format. It will also make oldModuleDef
s / system tables detect a migration.Expected complexity level and risk
2 - Changing the SATS definitions of types has been shown recently to be potentially scary. On the other hand, this redefinition is layout-compatible, so should be much less breaking than when we redefined
Identity
.Testing
Describe any testing you've done, and any testing you'd like your reviewers to do, so that you're confident that all the changes work as expected!
Timestamp
is well-behaved and compatible withSystemTime
.