Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

[V2.0] DID fragments instead of MethodID #266

Merged
merged 24 commits into from
Aug 29, 2022

Conversation

kwek20
Copy link
Contributor

@kwek20 kwek20 commented Jul 5, 2022

Description of change

Made use of the named fragments during key exchange and signing in an Identity when combined with a streams Identifier.

Fixes #239 and #245

Discussion

  • Move code in lets/src/id/identity.rs:205-222 to did.rs to clean up identity imports
  • Identity Client is currently stored, only as an URL. There may be a time this is not enough
  • This will call a DID document multiple times during wrap and unwrap. Do we want to cache the result 1 (or 5) seconds?

Type of change

  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How the change has been tested

updated and ran local tests on 2 different machines

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

@@ -6,5 +6,7 @@ members = [
"streams",
]

resolver = "2"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required because of an identity feature (Identity also needs this now)

let mut hash = [0; 64];
let mut fragment_bytes = Bytes::default();
let mut fragment_bytes = spongos::ddml::types::Bytes::default();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specified directly so we dont need to import and lock behind a cfg(feature)

Copy link
Contributor

@DyrellC DyrellC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering I assisted in the making of this implementation I don't have too many comments regarding the design choices, i'll leave those up to @arnauorriols to take a look at, but I do have a few general comments regarding the DID module separation and line removals.

@@ -0,0 +1,330 @@
// Rust
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to go so far as to split up the DID implementation we might as well see it through the whole way and define the different pieces of DID and separate them accordingly.

For example DataWrapper and DIDUrlInfo may make more sense to separate into their own files so that DIDUrlInfo doesn't get conflated with DIDInfo. Just a thought on cleaning this file up, because it's pretty chaotic as is right now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the circular dependency between did.rs and mod.rs a bit unsettling. Other than that, I like the approach to better encapsulate the public API of the did optional feature.

I guess I would have to see the separation @DyrellC suggests before having an option about it.

.try_into()
.expect("failed to convert ed25519 public-key to x25519 public-key"),
)
pub async fn _ke_pk(&self) -> Result<x25519::PublicKey> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'll need to keep this function we should probably remove the _ before the ke_pk and get rid of the deprecated comment above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method duplicates the implementation of ContentEncrypt*, and we need to consolidate these implementations. I see 2 options:

  1. Keep the ContentSign, ContentVerify, ContentEncrypt and ContentDecrypt traits, encapsulating these procedures to the Identity and Identifier types. This implies removing this method, and using ContentEncrypt in subscription::wrap() instead.
  2. Remove the traits, and have getters for the signing keys and ke keys from Identity and Identifier, and perform signature and key exchange in the wrapping/unwrapping of each message type.

I do think that it has to be all or nothing, to maintain consistency and predictability of implementation. I tend to favour as little indirection as possible, so normally I'd go with option 2, but I wonder if DID Account won't give us any other choice but to go with option 1...?

lets/src/id/identity.rs Outdated Show resolved Hide resolved
streams/examples/full-example/main.rs Outdated Show resolved Hide resolved
@@ -676,13 +682,14 @@ where
.state
.author_identifier
.as_ref()
.and_then(|author_id| self.state.exchange_keys.get(author_id))
.expect("a user that already have an stream address must know the author identifier");
.expect("a user that already have an stream address must know the author identifier")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use ok_or_else instead of expect in places like this. Erroring out is fine but a panic not so much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take care of this in the WIP error-handling PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe or_else should be used when wanting to avoid an operation. As this is not the case here, this works fineee

Copy link
Contributor

@arnauorriols arnauorriols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, nice work! I only have some comments on minor improvements, but overall looks good.

BTW, there are a bunch of lint errors that somehow are not picked up by CI. Take a look at them (make sure to update your toolchain as there are new lints in the newest versions).

@@ -41,7 +41,7 @@ hex = {version = "0.4", default-features = false}

# Optional dependencies
futures = {version = "0.3.8", default-features = false, optional = true}
identity = {git = "https://github.com/iotaledger/identity.rs", rev = "86edaad", default-features = false, features = ["async"], optional = true}
identity_iota = {git = "https://github.com/iotaledger/identity.rs", rev = "d3920c2", default-features = false, optional = true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that this rev already takes the new package name used for releasing to crates.io, I wonder if we couldn't actually use version = "0.6.1" instead of using the git rev. We will have to do it anyway at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didnt work due to an identity error internally when compiling :P

error[E0432]: unresolved import `identity_did::revocation`
  --> /home/brord/.cargo/registry/src/github.com-1ecc6299db9ec823/identity_iota_client-0.6.1/src/credential/credential_validator.rs:11:19
   |
11 | use identity_did::revocation::RevocationBitmap;
   |                   ^^^^^^^^^^ could not find `revocation` in `identity_did`

lets/src/address.rs Outdated Show resolved Hide resolved
lets/src/id/identifier.rs Outdated Show resolved Hide resolved
Some(pk)
} else {
None
Identifier::DID(url_info) => url_info.did().as_bytes(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identifier::as_bytes() is used for address generation (both AppAddr and MsgId). Therefore, we need "did + signing-fragment + ke-fragment" in the byte output in order to have a unique byte representation of each Identifier

.try_into()
.expect("failed to convert ed25519 public-key to x25519 public-key"),
)
pub async fn _ke_pk(&self) -> Result<x25519::PublicKey> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method duplicates the implementation of ContentEncrypt*, and we need to consolidate these implementations. I see 2 options:

  1. Keep the ContentSign, ContentVerify, ContentEncrypt and ContentDecrypt traits, encapsulating these procedures to the Identity and Identifier types. This implies removing this method, and using ContentEncrypt in subscription::wrap() instead.
  2. Remove the traits, and have getters for the signing keys and ke keys from Identity and Identifier, and perform signature and key exchange in the wrapping/unwrapping of each message type.

I do think that it has to be all or nothing, to maintain consistency and predictability of implementation. I tend to favour as little indirection as possible, so normally I'd go with option 2, but I wonder if DID Account won't give us any other choice but to go with option 1...?

streams/src/api/user.rs Outdated Show resolved Hide resolved
streams/src/api/user.rs Outdated Show resolved Hide resolved
@@ -676,13 +682,14 @@ where
.state
.author_identifier
.as_ref()
.and_then(|author_id| self.state.exchange_keys.get(author_id))
.expect("a user that already have an stream address must know the author identifier");
.expect("a user that already have an stream address must know the author identifier")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take care of this in the WIP error-handling PR

@@ -830,7 +825,7 @@ where
.collect::<Result<Vec<(_, _)>>>()?; // collect to handle possible error
let content = PCF::new_final_frame().with_content(keyload::Wrap::new(
&mut linked_msg_spongos,
subscribers_with_keys.iter().copied(),
subscribers.clone().into_iter().collect::<Vec<_>>(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written an small PR to your branch removing this re-collection of the subscribers iterator: kwek20#4

@@ -0,0 +1,330 @@
// Rust
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the circular dependency between did.rs and mod.rs a bit unsettling. Other than that, I like the approach to better encapsulate the public API of the did optional feature.

I guess I would have to see the separation @DyrellC suggests before having an option about it.

Brord van Wierst and others added 7 commits July 15, 2022 15:51
…-subscribers-iterator-to-avoid-unnecessary-collection
…rs-iterator-to-avoid-unnecessary-collection

Correct lifetime parameters in `Keyload::Wrap` to avoid unnecessary collection of `Subscribers` iterator
Copy link
Contributor

@arnauorriols arnauorriols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kwek20 kwek20 merged commit fa49d74 into iotaledger:v2.0-dev Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants