Add feature to support custom `now_utc` implementations #1397

frederikrothenberger · 2024-08-23T11:43:53Z

Description of change

This PR adds a feature to identity_core to allow specifying a custom function to get the current time (Timestamp::now_utc). The feature is disabled by default.

Fixes #1391.

Type of change

Add an x to the boxes that are relevant to your changes.

Bug fix (a non-breaking change which fixes an issue)
Enhancement (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Fix

How the change has been tested

Added an integration test to identity_core that uses the custom hook.

Change checklist

Add an x to the boxes that are relevant to your changes.

I have followed the contribution guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
@itsyaasir: does this feature require more documentation?
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

frederikrothenberger · 2024-08-23T11:59:16Z

@itsyaasir @UMR1352: There will need to be some CI changes as cargo test --workspace --all-features no longer compiles (since the test binaries are missing the custom time implementation but have custom_time enabled).

I.e. testing will need to be split into cargo test --test custom_time --features="custom_time" and running all other tests with custom_time disabled.

I did not touch CI (yet). How would you want me to approach that issue?

identity_core/src/custom_time.rs

itsyaasir · 2024-08-23T12:17:24Z

@itsyaasir @UMR1352: There will need to be some CI changes as cargo test --workspace --all-features no longer compiles (since the test binaries are missing the custom time implementation but have custom_time enabled).

I.e. testing will need to be split into cargo test --test custom_time --features="custom_time" and running all other tests with custom_time disabled.

I did not touch CI (yet). How would you want me to approach that issue?

Yeah, I think splitting it will make sense in the CI

itsyaasir

Thanks for opening this PR, everything is looking fine to me 👍🏾 .

Let us get the CI passing now.

eike-hass · 2024-08-23T12:22:07Z

@itsyaasir @UMR1352: There will need to be some CI changes as cargo test --workspace --all-features no longer compiles (since the test binaries are missing the custom time implementation but have custom_time enabled).

I.e. testing will need to be split into cargo test --test custom_time --features="custom_time" and running all other tests with custom_time disabled.

I did not touch CI (yet). How would you want me to approach that issue?

We will take care of CI 👌

frederikrothenberger · 2024-08-23T12:23:06Z

Thanks a lot!

frederikrothenberger · 2024-08-26T13:08:28Z

@itsyaasir: In order to successfully compile identity_core for the target wasm32-unknown-unknown without having a dependency on js-sys, I needed to do the following additional changes: frederikrothenberger@3dd5198

Could we include that commit in this PR as well, or would you rather have that separately?

itsyaasir · 2024-08-26T13:35:50Z

@itsyaasir: In order to successfully compile identity_core for the target wasm32-unknown-unknown without having a dependency on js-sys, I needed to do the following additional changes: frederikrothenberger@3dd5198

Could we include that commit in this PR as well, or would you rather have that separately?

You can add it in this PR, no problem

eike-hass · 2024-08-29T08:11:50Z

@frederikrothenberger could you try to re-sign your commits? 🙏
Edit: Might mean you need to force push the branch

This PR adds a feature to `identity_core` to allow specifying a custom function to get the current time (`Timestamp::now_utc`). The feature is disabled by default. Closes iotaledger#1391.

Also removes the unused dependency on `iota-crypto` (which also had a dependency on `js-sys` through the `random` feature).

frederikrothenberger · 2024-08-29T09:03:25Z

@eike-hass: The commits are now signed 👍

itsyaasir · 2024-09-02T11:00:52Z

@frederikrothenberger Thanks for the contribution.

In iotaledger#1397 my intention was to make the `js-sys` dependency mutually exclusive with the feature `custom_time`. As it turns out, it's not that simple and mixing target specific dependencies with feature specific dependencies does not actually work. See [here](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies) and [here](https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features). So this removes the broken feature reference in the dependency declaration and instead marks it as `optional`.

In iotaledger#1397 my intention was to make the `js-sys` dependency mutually exclusive with the feature `custom_time`. As it turns out, it's not that simple and mixing target specific dependencies with feature specific dependencies does not actually work. See [here](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies) and [here](https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features). So this removes the broken feature reference in the dependency declaration and instead marks it as `optional`. Also, the feature `custom_time` takes precedence over `js-sys` so these do not actually conflict and one _could_ enable both.

* Mark `js-sys` as optional for identity_core In #1397 my intention was to make the `js-sys` dependency mutually exclusive with the feature `custom_time`. As it turns out, it's not that simple and mixing target specific dependencies with feature specific dependencies does not actually work. See [here](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies) and [here](https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features). So this removes the broken feature reference in the dependency declaration and instead marks it as `optional`. Also, the feature `custom_time` takes precedence over `js-sys` so these do not actually conflict and one _could_ enable both. * Make js-sys a default feature * Fix defaults switch * Don't expose `js-sys` feature Co-authored-by: Yasir <yasirshariffa@gmail.com> --------- Co-authored-by: Yasir <yasirshariffa@gmail.com>

frederikrothenberger requested a review from a team as a code owner August 23, 2024 11:43

frederikrothenberger changed the title ~~Add feature to support custom not_utc implementations~~ Add feature to support custom now_utc implementations Aug 23, 2024

frederikrothenberger force-pushed the frederik/feature-custom-time branch from def22a2 to bccee56 Compare August 23, 2024 11:45

frederikrothenberger mentioned this pull request Aug 23, 2024

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

Closed

UMR1352 approved these changes Aug 23, 2024

View reviewed changes

itsyaasir reviewed Aug 23, 2024

View reviewed changes

identity_core/src/custom_time.rs Outdated Show resolved Hide resolved

itsyaasir reviewed Aug 23, 2024

View reviewed changes

eike-hass added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Aug 26, 2024

Frederik Rothenberger and others added 6 commits August 29, 2024 10:55

Add feature to support custom now_utc implementations

1ce8f33

This PR adds a feature to `identity_core` to allow specifying a custom function to get the current time (`Timestamp::now_utc`). The feature is disabled by default. Closes iotaledger#1391.

Formatting

4deaa59

Fix wrong comment

dde896d

chore: clippy fixes and fmt

f67cb5e

chore: clippy fixes and fmt

2fda099

Allow compilation for target wasm32-unknown-unknown without js-sys

989e6ca

Also removes the unused dependency on `iota-crypto` (which also had a dependency on `js-sys` through the `random` feature).

frederikrothenberger force-pushed the frederik/feature-custom-time branch from 4670be8 to 989e6ca Compare August 29, 2024 08:55

chore(ci): Fix CI actions; add random feature to iota crypto

6cee4a5

itsyaasir approved these changes Aug 30, 2024

View reviewed changes

itsyaasir merged commit 842f483 into iotaledger:main Sep 2, 2024

frederikrothenberger mentioned this pull request Sep 2, 2024

Mark js-sys as optional for identity_core #1405

Merged

10 tasks

eike-hass removed the Wasm Related to Wasm bindings. Becomes part of the Wasm changelog label Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add feature to support custom `now_utc` implementations #1397

Add feature to support custom `now_utc` implementations #1397

Uh oh!

frederikrothenberger commented Aug 23, 2024 •

edited by itsyaasir

Loading

Uh oh!

frederikrothenberger commented Aug 23, 2024

Uh oh!

Uh oh!

itsyaasir commented Aug 23, 2024

Uh oh!

itsyaasir left a comment

Uh oh!

eike-hass commented Aug 23, 2024

Uh oh!

frederikrothenberger commented Aug 23, 2024

Uh oh!

frederikrothenberger commented Aug 26, 2024

Uh oh!

itsyaasir commented Aug 26, 2024

Uh oh!

eike-hass commented Aug 29, 2024 •

edited

Loading

Uh oh!

frederikrothenberger commented Aug 29, 2024

Uh oh!

itsyaasir commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add feature to support custom now_utc implementations #1397

Add feature to support custom now_utc implementations #1397

Uh oh!

Conversation

frederikrothenberger commented Aug 23, 2024 • edited by itsyaasir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Type of change

How the change has been tested

Change checklist

Uh oh!

frederikrothenberger commented Aug 23, 2024

Uh oh!

Uh oh!

itsyaasir commented Aug 23, 2024

Uh oh!

itsyaasir left a comment

Choose a reason for hiding this comment

Uh oh!

eike-hass commented Aug 23, 2024

Uh oh!

frederikrothenberger commented Aug 23, 2024

Uh oh!

frederikrothenberger commented Aug 26, 2024

Uh oh!

itsyaasir commented Aug 26, 2024

Uh oh!

eike-hass commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederikrothenberger commented Aug 29, 2024

Uh oh!

itsyaasir commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add feature to support custom `now_utc` implementations #1397

Add feature to support custom `now_utc` implementations #1397

frederikrothenberger commented Aug 23, 2024 •

edited by itsyaasir

Loading

eike-hass commented Aug 29, 2024 •

edited

Loading