[Request] Remove (or make optional) dependency on time (Timestamp::now_utc())

[frederikrothenberger]

Description

We'd like to contribute to these crates a PR that removes (or makes optional via feature flag) the dependency on a specific way to obtain the current time (currently provided by Timestamp::now_utc()).

Motivation

The reason for this feature request / contribution is that we'd like to make use of this library in ICP smart contracts. These smart contracts have a custom way of accessing the current time. As such, it would be great if the current time could be provided externally by the library user.

The ultimate goal is to make the library wasm32-unknown-unknown compatible without requiring WASI or JS bindings.

Requirements

Write a list of what you want this feature to do.

1. Remove the dependency on now_utc or allow it's implementation to be provided by the library user

Open questions

I have the following open questions:

• Do I need to implement this without breaking changes, or are breaking changes an option?
  • If it must be non-breaking: The only option I see there is adding a default feature time that can be explicitly disabled by clients. Having that will add some complexity toggling all the code paths to now_utc off. Of course, I'll try to minimize the impact.
• Currently, we only use the identity_core, identity_credential and identity_jose crates. Should I / is it desired to remove the dependency on a specific time provider from all other crates too or should I scope it to only exactly what we need?

Request for feedback on a possible approach

The need to access time seems to mostly come from builders and options that allow to not specify the time which results in defaulting to now_utc somewhere deeply nested in the library code.

The simplest approach I see is to move the point at which now_utc is being called to an earlier point in time. One way of doing that is to change the interfaces of the functions that take these options from fn foo(options: &OptionsWithOptionalTime) -> ... to something like fn foo(options: &impl Into<OptionsWithTime> -> ... and then provide an implementation From<OptionsWithOptionalTime> for OptionsWithTime and toggle OptionsWithOptionalTime behind the time feature flag.

Similarly for the Credential::from_builder, we could introduce a CredentialBuilderWithMandatoryIssuance and put the issuance_date into the conversion from the current builder to the new one.

Do you see this as a feasible approach? Happy to hear any type of feedback on this. Especially open to suggestions that make the implementation easier. 😉

One disadvantage of that approach I see is that it leads to potentially many conversions with cloning behind the scenes for existing users of OptionsWithOptionalTime. Or we make the conversion zero-copy (i.e. OptionsWithTimeReferencingOptionsWithOptionalTime) which will introduce a lot more complexity in terms of lifetimes, etc. I'd rather avoid that.
Should I be concerned about the costs of conversions or not?

Are you planning to do it yourself in a pull request?

Yes

[eike-hass]

Hi there 👋 After some internal discussion we would prefer if we can introduce the needed extensibility in a non-breaking way. We are in the process of designing the next mayor release, where we would revisit the topic and provide a more complete interface, but for now the quickest way to release the requested extensibility is to add a new cfg for the wasm32-unknown-unknown target.

My colleague @itsyaasir will support the effort 💪

[itsyaasir]

Hi there 👋🏾 , expounding on what @eike-hass has said, currently the Timestamp::now_utc() has a cfg check for wasi and non-wasi compilation, we can have additional for wasm32-unknown-unknown target. This is a very minimal approach with limited moving parts. What do you think @frederikrothenberger ?

[itsyaasir]

Hi @frederikrothenberger , Is it possible to import and call an external function from a WebAssembly module in an Internet Computer (IC) canister?

One of the approaches suggested by the team involves implementing a now_utc function by importing an external function from a Wasm module. While this approach would involve using the unsafe keyword in Rust, it is considered acceptable within this context. Here is a rough outline of the implementation:

#[link(wasm_import_module = "time")]
extern "C" {
    fn now() -> u64;
}

fn utc_now() {
	unsafe { now() }
}

This method allows us to use the external function to get the current time. Would this approach be feasible ?

[frederikrothenberger]

@itsyaasir: We actually do something similar in our SDK:

#[link(wasm_import_module = "ic0")]
extern "C" {
    pub fn time() -> i64;
}

But I can't assume the existence of that module for generic wasm32-unknown-unknown. Or is the idea to add a feature flag for ICP specifically?

[itsyaasir]

Hey @frederikrothenberger, to keep this project as general purpose as possible we won't depend on ICP's SDK nor we will add a feature flag specifically for ICP.

We think the approach we showcased in our previous messages should hopefully solve your issue as well as allowing other users to work with their own concept of time.

Basically, the approach consists on allowing the user to provide their definition of time through a foreign function fn now() -> i64 defined in a WASM module time.

Here is the code that deals with what I just discussed:

#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
pub mod wasm_time {
  #[link(wasm_import_module = "time")]
  extern "C" {
    /// Returns the timestamp
    pub fn now() -> i64;
  }
}

#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
#[no_mangle]
pub fn now_utc() -> Self {
  let ts = { unsafe { wasm_time::now() } };
  Self::from_unix(ts).expect("Timestamp failed to convert system datetime")
}

From your previous message it seems this approach could easily be applied to your specific use case: if you create a time::now WASM function that simply forward its implementation to your ic0::time function everything should work as intended - given the right modules are linked out.

By default we still want to depend on js_sys to get the current time in WASM targets, thus we will lock the aforementioned code behind an extern-time feature (which shall be disabled by default).

Could this work for you?

[frederikrothenberger]

@itsyaasir: Sorry, this task got pushed back on my side due to other priorities. And I'll be on PTO for the next two weeks. I'll report back after.

[frederikrothenberger]

Hi @itsyaasir

From your previous message it seems this approach could easily be applied to your specific use case: if you create a time::now WASM function that simply forward its implementation to your ic0::time function everything should work as intended - given the right modules are linked out.

Can this forwarding function be implemented in a library that has a dependency on identity.rs? We don't want our devs needing to go through additional / custom linking steps. They should simply be able to pull in the library, compile to wasm and deploy (as with any other functionality they pull in).

[frederikrothenberger]

@itsyaasir: So after thinking more about this, I don't think importing a function from some generically named wasm module is convenient for users of that feature, as it would impose additional steps in the build process.

However, what we could do is take a similar approach to the custom feature of getrandom. See here for the details: https://docs.rs/getrandom/latest/getrandom/macro.register_custom_getrandom.html

What do you think? This would allow the registration of a custom time function regardless of the platform without going through additional build steps and we could also gate the macro behind a feature flag.

[itsyaasir]

@frederikrothenberger

After liaising with the team, I think this sounds like a great approach. You are welcome to contribute to this by opening a pull request (PR), and you can invite me and the team to review it. Please let me know if you have any more questions or concerns.

Thank you.

[frederikrothenberger]

@itsyaasir: I opened #1397.