Add separate `identity-iota-core`, `identity-account-storage` crates #693

HenriqueNogara · 2022-03-07T18:22:31Z

Description of change

Split identity-iota and identity-account into identity-iota-core and identity-account-core.

Links to any relevant issues

#669

Type of change

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

All existing tests are still present.

Change checklist

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

…tangle ref back to identity-iota

cycraig

I approve of the overall approach but there are still dependencies that can be removed like iota-client in identity-iota-core and we should discuss the finer-details.

I think we have to delay this PR until both Update Stronghold #691 and Overhaul CredentialValidator, add PresentationValidator #599 have been merged unfortunately, since they both alter many of the files moved in this PR. I do not think the merge conflicts would be easily resolvable without losing changes otherwise.
We should decide whether to keep the iota_core and account_core modules in the top-level identity crate or re-export their types under the iota and account modules respectively. I am leaning towards re-exporting the types at the top-level for convenience to developers unless it doesn't make sense (e.g. the documentation and doc-comments still refer to those types under iota_core). We should probably not re-export those types in the sub-crates, however, i.e. do not re-export identity_iota_core::IotaDocument from identity_iota.
There are some dependencies like iota-client which can be removed/replaced with bee-message. Edit: if necessary we can re-export MessageId from identity-iota-core to avoid having to add the bee-message dependency anywhere else.
Error handling is a bit tricky since errors are just bubbled up from the sub-crates now. It's probably fine to leave it as-is for this PR? Not sure.

bindings/wasm/src/did/wasm_document_metadata.rs

identity-account-core/Cargo.toml

identity/Cargo.toml

cycraig · 2022-03-08T11:15:46Z

identity/src/lib.rs

+#[cfg(feature = "account")]
+#[cfg_attr(docsrs, doc(cfg(feature = "account")))]
+pub mod account_core {
+  //! Storage Trait and Types definitions
+
+  pub use identity_account_core::crypto::*;
+  pub use identity_account_core::error::*;
+  pub use identity_account_core::identity::*;
+  pub use identity_account_core::storage::*;
+  pub use identity_account_core::stronghold::*;
+  pub use identity_account_core::types::*;
+  pub use identity_account_core::utils::*;
 }


Discussion: I think we should re-export types from the _core crates under the non-core namespace (unless there are conflicts). That is, move all these pub use identity_account_core lines under pub mod account. Similarly for identity_iota_core and identity_iota.

All this does is simplify imports and allow us to sub-divide crates as much as we want without causing breaking changes to the external interface, which I think is desirable. The Error and Result instances are a problem, I'm not sure if we want to adopt a convention of prefixing the crate name onto those like in this PR where e.g. IotaCoreError is used instead of just Error? Not sure, need to decide and implement the convention everywhere.

The convenience of condensed imports may not be worth any confusion, just to be clear. Edit: weighed against the benefit of not causing breaking changes when we re-organise internal crates, of course, assuming we consider identity the external API and not the constituent crates as well.

Overall I agree with re-exporting types from _core crates under the non-core namespace in identity because consumers of the main identity are likely not interested in how it is organized internally. This same principle also applies to errors hence I would prefer to avoid variants/names like IotaCoreError if possible.

I am less sure about how much re-exports from sub(-sub) crates to unified modules in identity protect us against introducing breaking changes in the external API upon further sub-division however. The types in our public API need to be interchangeable between minor versions, but cargo may not be able to "understand" that the types are actually still the same when their origin has changed from one sub crate to another. It might be possible to help express the compatibility of types between minor versions by using something like the semver trick however.

I would prefer to avoid variants/names like IotaCoreError if possible.

The problem is that we cannot export two Error enums of the same name under identity::iota from identity_iota and identity_iota_core without changing the name of one or the other.

If we cannot avoid breaking changes by re-exporting types then I am somewhat less in favour of re-exporting them. The semver trick is fascinating but we should hopefully not plan to use it as a crutch.

identity-iota-core/Cargo.toml

identity-did/src/document/core_document.rs

identity-account/src/updates/update.rs

identity-account-core/src/error.rs

Cargo.toml

cycraig

Thanks for the changes! I pushed some commits to fix the last few comments, hopefully without causing too many merge conflicts for the epic branch.

PhilippGackstatter

I know this has been merged now, but: looks good to me. Just one comment unrelated to the split.

PhilippGackstatter · 2022-03-14T11:55:56Z

identity-iota/Cargo.toml

@@ -48,7 +49,7 @@ features = ["blake2b"]

 [dev-dependencies]
 proptest = { version = "1.0.0", default-features = false, features = ["std"] }
-tokio = { version = "1.15", default-features = false, features = ["macros"] }
+tokio = { version = "1.17.0", default-features = false, features = ["macros"] }


Should we actually specify the highest possible version if we don't require it? To maximize compatibility within our crates and for our users, shouldn't we specify the lowest possible version that still satisfies our needs?

I'm unsure of the circumstances in which we should not update to the latest version of Rust dependencies, particularly when they are semver compatible (edit: since as a library we're pulling in the latest compatible version automatically anyway).

In this case I found it prudent specifically because tokio 1.17.0 contains fixes to avoid panics.

I'll keep it in mind that we may not want to update all dependencies for compatibility reasons, especially if those updates contain API-breaking changes.

I have no such justification for strum, however. I just wanted the latest version (and forgot to update it everywhere).

Henrique Nogara added 10 commits March 4, 2022 14:02

split iota-identity - add iota-identity-core

8e29365

fix tests, fix doc tests, fix imports in indentity-account

db72fed

add default impl for coredoc, remove doc clone from resolver

fe996e7

fix wasm imports

18a6ae4

move bee_message::message to iota_core

e28161d

add set_previous_message_id and set_message_id for DiffMessage, move …

8ebb410

…tangle ref back to identity-iota

split identity-account, add identity-account-core

25536f5

move MessageId back to identity-iota

42680a5

remove old error variants

6bf7aad

remove nodejs bindings

cf49bb1

HenriqueNogara marked this pull request as ready for review March 7, 2022 21:50

solve merge conflicts - wasm

74b6231

HenriqueNogara added Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Chore Tedious, typically non-functional change labels Mar 7, 2022

fix tom fmt, tests

f62678b

HenriqueNogara requested review from cycraig and olivereanderson March 7, 2022 23:11

cycraig suggested changes Mar 8, 2022

View reviewed changes

PhilippGackstatter reviewed Mar 10, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

cycraig mentioned this pull request Mar 10, 2022

Run stronghold bindings in build-and-test and prepare release #690

Merged

10 tasks

Henrique Nogara added 4 commits March 10, 2022 18:30

solve merge with dev

46b6f81

solve merge conflicts with dev

7b3b98b

add default features false

9ecb805

change network import

c0ac491

HenriqueNogara requested a review from cycraig March 14, 2022 04:19

HenriqueNogara mentioned this pull request Mar 14, 2022

[Task] Feature-gate the diff module in identity-iota-core #710

Closed

6 tasks

Replace iota-client dependency with bee-message in identity-iota-core

1bc24e7

cycraig changed the title ~~Chore/split identity iota~~ Add identity-iota-core, identity-account-storage crates Mar 14, 2022

Combine message and types modules into tangle

b2b9ad2

cycraig added 9 commits March 14, 2022 10:46

Remove unnecessary clone

6c0a6a8

Improve identity-iota-core errors

bb0e118

Fix Wasm compilation

2db2dbf

Fix Wasm bindings

cde400d

Remove dead code

a84d5c0

Fix bee-message features

c30eea7

Remove dead code

b0f7479

Feature-gate stronghold

11dbe15

Make tokio optional in identity-account-storage

4b2d2b7

cycraig requested a review from PhilippGackstatter March 14, 2022 10:17

cycraig approved these changes Mar 14, 2022

View reviewed changes

HenriqueNogara merged commit 34bb77a into dev Mar 14, 2022

HenriqueNogara deleted the chore/split-identity-iota branch March 14, 2022 12:45

PhilippGackstatter reviewed Mar 14, 2022

View reviewed changes

cycraig changed the title ~~Add identity-iota-core, identity-account-storage crates~~ Add separate identity-iota-core, identity-account-storage crates Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate `identity-iota-core`, `identity-account-storage` crates #693

Add separate `identity-iota-core`, `identity-account-storage` crates #693

HenriqueNogara commented Mar 7, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

cycraig Mar 8, 2022 •

edited

Loading

cycraig Mar 8, 2022 •

edited

Loading

olivereanderson Mar 8, 2022 •

edited

Loading

cycraig Mar 8, 2022

cycraig left a comment •

edited

Loading

PhilippGackstatter left a comment

PhilippGackstatter Mar 14, 2022

cycraig Mar 14, 2022 •

edited

Loading

cycraig Mar 14, 2022

Add separate identity-iota-core, identity-account-storage crates #693

Add separate identity-iota-core, identity-account-storage crates #693

Conversation

HenriqueNogara commented Mar 7, 2022 • edited Loading

Description of change

Links to any relevant issues

Type of change

How the change has been tested

Change checklist

cycraig left a comment • edited Loading

Choose a reason for hiding this comment

cycraig Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

cycraig Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

olivereanderson Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

cycraig Mar 8, 2022

Choose a reason for hiding this comment

cycraig left a comment • edited Loading

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

PhilippGackstatter Mar 14, 2022

Choose a reason for hiding this comment

cycraig Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

cycraig Mar 14, 2022

Choose a reason for hiding this comment

Add separate `identity-iota-core`, `identity-account-storage` crates #693

Add separate `identity-iota-core`, `identity-account-storage` crates #693

HenriqueNogara commented Mar 7, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

cycraig Mar 8, 2022 •

edited

Loading

cycraig Mar 8, 2022 •

edited

Loading

olivereanderson Mar 8, 2022 •

edited

Loading

cycraig left a comment •

edited

Loading

cycraig Mar 14, 2022 •

edited

Loading