Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set controller, alsoKnownAs fields from Account #658

Merged
merged 7 commits into from
Feb 18, 2022
Merged

Set controller, alsoKnownAs fields from Account #658

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef3b771
poc `set_controller`
Feb 14, 2022
10a31a4
implement `set_also_known_as`
Feb 15, 2022
6010425
format
Feb 15, 2022
2915bcd
add tests for `set_controller` and `also_known_as`
Feb 16, 2022
be909de
remote redundant `set_also_known_as(..)`
Feb 16, 2022
cfa1742
Merge branch 'dev' into feat/account-fields-controller-aka
abdulmth Feb 17, 2022
deaecd5
fix set controller
Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions identity-account/src/tests/updates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

use std::sync::Arc;

use identity_core::common::OneOrSet;
use identity_core::common::OrderedSet;
use identity_core::common::Timestamp;
use identity_core::common::Url;
use identity_core::crypto::KeyCollection;
Expand Down Expand Up @@ -698,3 +700,62 @@ async fn test_remove_service() -> Result<()> {

Ok(())
}

#[tokio::test]
async fn test_set_controller() -> Result<()> {
let mut account = Account::create_identity(account_setup(Network::Mainnet).await, IdentitySetup::default()).await?;

let keypair1: KeyPair = KeyPair::new_ed25519().unwrap();
let iota_did1: IotaDID = IotaDID::new(keypair1.public().as_ref()).unwrap();

let keypair2: KeyPair = KeyPair::new_ed25519().unwrap();
let iota_did2: IotaDID = IotaDID::new(keypair2.public().as_ref()).unwrap();

// Set one controller.
let update: Update = Update::SetController {
controllers: Some(OneOrSet::new_one(iota_did1.clone())),
};
account.process_update(update).await.unwrap();
assert_eq!(account.document().controller().unwrap().len(), 1);

// Set two controllers.
let set: OrderedSet<IotaDID> = OrderedSet::from_iter(vec![iota_did1, iota_did2]);
let update: Update = Update::SetController {
controllers: Some(OneOrSet::new_set(set).unwrap()),
};
account.process_update(update).await.unwrap();
assert_eq!(account.document().controller().unwrap().len(), 2);

// Remove all controllers.
let update: Update = Update::SetController { controllers: None };
account.process_update(update).await.unwrap();
assert_eq!(account.document().controller(), None);

Ok(())
}

#[tokio::test]
async fn test_set_also_known_as() -> Result<()> {
let mut account = Account::create_identity(account_setup(Network::Mainnet).await, IdentitySetup::default()).await?;

// No elements by default.
assert_eq!(account.document().also_known_as().len(), 0);

// Set two Urls.
let urls: OrderedSet<Url> = OrderedSet::from_iter(vec![
Url::parse("did:iota:xyz").unwrap(),
Url::parse("did:iota:abc").unwrap(),
]);
let update: Update = Update::SetAlsoKnownAs { urls };
account.process_update(update).await.unwrap();
assert_eq!(account.document().also_known_as().len(), 2);

// Remove all Urls.
let update: Update = Update::SetAlsoKnownAs {
urls: OrderedSet::new(),
};
account.process_update(update).await.unwrap();
assert_eq!(account.document().also_known_as().len(), 0);

Ok(())
}
25 changes: 25 additions & 0 deletions identity-account/src/updates/update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ use crypto::signatures::ed25519;

use identity_core::common::Fragment;
use identity_core::common::Object;
use identity_core::common::OneOrSet;
use identity_core::common::OrderedSet;
use identity_core::common::Timestamp;
use identity_core::common::Url;
use identity_core::crypto::KeyPair;
use identity_core::crypto::KeyType;
use identity_core::crypto::PublicKey;
Expand Down Expand Up @@ -130,6 +133,12 @@ pub(crate) enum Update {
DeleteService {
fragment: String,
},
SetController {
controllers: Option<OneOrSet<IotaDID>>,
},
SetAlsoKnownAs {
urls: OrderedSet<Url>,
},
Comment on lines +138 to +143
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't really follow the convention the account updates have so far. The existing updates add or remove a service/method from the set of services/methods in the document. These Set* updates overwrite whatever was there previously. Either we change this to be CreateController & DeleteController and CreateAlsoKnownAs & DeleteAlsoKnownAs or we change the existing updates to also be Set* updates.

I think the Set* variants are rather unwieldy for non-trivial updates, where e.g. one wants to add a controller to a document. You'd have to copy over the old controllers. So, I'm in favor of having the Create* and Delete*-style updates. One way or another, we should be consistent.

The Create terminology doesn't feel appropriate here, so we could also use the Add* prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly due to my description in issue #647. The controller and also_known_as fields are simple collections rather than complex structs like VerificationMethod and Service, so it is my opinion that they don't need fine-grained operations which contribute to API pollution (maybe too strong a term).

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering how simple the other account updates are, I don't think set-style updates fit nicely with the ease of the API that we have so far. Just adding a controller requires:

let set = match account.document().controller() {
  Some(set) => {
    let mut set = OrderedSet::from_iter(set.to_vec());
    set.append(iota_did3);
    OneOrSet::new_set(set).unwrap()
  }
  None => OneOrSet::new_one(iota_did3),
};

let update: Update = Update::SetController {
  controllers: Some(set),
};
account.process_update(update).await.unwrap();

I assume these updates "in code" are infrequent, to either controller or alsoKnownAs, compared to services or methods, but when they occur it's quite verbose.
I generally don't see set operations for collections exposed on APIs because they are unwieldy to work with. And since in our API we don't have the possibility to return a &mut OrderedSet to let users append through that, I think the nicest solution would be to expose add and delete operations. Of course, I would like to avoid the pollution of the API, but that takes lower priority in my mind.

Copy link
Contributor

@cycraig cycraig Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That example can be simplified somewhat (and even more so if using an intermediate Vec and From/TryFrom):

let set = match account.document().controller().cloned() {
  Some(mut one_or_set) => {
    one_or_set.append(iota_did3);
    one_or_set
  }
  None => OneOrSet::new_one(iota_did3),
};

// Or, equivalently.
let mut vec = account.document().controller().cloned().map(Vec::from).unwrap_or_default();
vec.push(iota_did3);
let set = OneOrSet::try_from(vec)?;

let update: Update = Update::SetController {
  controllers: Some(set),
};
account.process_update(update).await.unwrap();

Would it be better if they took Vec instead of OneOrSet? That would work around the awkward OneOrSet restrictions at the risk of runtime errors?

I generally don't see set operations for collections exposed on APIs because they are unwieldy to work with.

My problem with individual builder operations on collections is that it's even more difficult to use when you actually have a collection you want to pass to the builder. As was seen when exposing the attach/detach_method_relationships API to the Wasm bindings due to intermediate, temporary structs in the builder pattern being dropped.

  let mut account: RefMut<Account> = account.borrow_mut();
  let mut updater: IdentityUpdater<'_> = account.update_identity();
  let mut attach_relationship: AttachMethodRelationshipBuilder<'_> =
    updater.attach_method_relationship().fragment(fragment);

  for relationship in relationships {
    attach_relationship = attach_relationship.relationship(relationship);
  }

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 can solve the latter by using something like:

pub fn relationships<T, E>(&self, t: T)
where
  E: KeyComparable,
  T: TryInto<OneOrSet<E>> {}

on the AttachMethodRelationshipBuilder so someone can call

  account
    .update_identity()
    .attach_method_relationship()
    .fragment("my-next-key")
    .relationships(MethodRelationship::CapabilityDelegation)
    .relationships(vec![MethodRelationship::CapabilityInvocation, MethodRelationship::AssertionMethod])
    .apply()
    .await?;

Granted, it's not ergonomic to require TryInto so we may want to reconsider the bounds on OneOrSet (probably not possible) or introduce a new small type to facilitate that. (This example ignores the errors). We could do that similarly for controllers/alsoKnownAs. I think that would at least solve the "extend an existing collection with a collection" problem.

If we still want to do set-style updates, I think taking Vec would be a good step at least.

Copy link
Contributor

@cycraig cycraig Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding relationships: I opened #654 previously and recommended either a vec, slice, or iterator as input since MethodRelationship implements Copy.

I wouldn't mind using TryInto. The only difference is that it moves the error from apply up to the field call at relationships, so really the example would be:

pub fn relationships<T, E>(self, t: T) -> Result<Self>
where
  E: KeyComparable,
  T: TryInto<OneOrSet<E>> {}
  account
    .update_identity()
    .attach_method_relationship()
    .fragment("my-next-key")
    .relationships(MethodRelationship::CapabilityDelegation)?
    .relationships(vec![MethodRelationship::CapabilityInvocation, MethodRelationship::AssertionMethod])?
    .apply()
    .await?;

Not much different but it sets a precedent about where update builder errors can be thrown. One good thing about requiring the actual type (OneOrSet) is that it forces the developer to handle those errors up-front rather than having to interpret errors from the builder, but I do admit it is less easy-to-use.

Edit: attach_method_relationship/detach_method_relationship are idempotent so they can just use Vec/slice, having them called with an empty list or duplicates doesn't matter there since it's an operation not a field setter like controller/also_known_as. The example translates to the matter at hand though.

We could do that similarly for controllers/alsoKnownAs. I think that would at least solve the "extend an existing collection with a collection" problem.

Back to this: what's the conclusion? Are we staying with set_controller/set_also_known_as (with improved ergonomics for the input parameters such as Vec or TryInto) or are we set on splitting them into four methods for create/remove each? I'm still of the opinion that set_* methods are preferable for these fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

For builders, I think I'd prefer them not to throw errors. Not sure how common errors in builders are. If From<Vec<T>> for OrderedSet was implemented, that would make things a lot easier, and we could by ignoring duplicates. But I suppose that was a deliberate choice, so I don't want to reopen that discussion, too.

what's the conclusion? Are we staying with set_controller/set_also_known_as (with improved ergonomics for the input parameters such as Vec or TryInto)

I'm still more in favor of the create/delete methods, since with the Vec or TryInto, it should make working with single and collection values fairly easy, and because adding any value with set-style methods is cumbersome.
Since we're not really coming to a conclusion, though, I'm fine with going with set-updates for now and see how they turn out in practice. In that case, I would go with Vec to avoid errors on the builder method.

Unless there's more opinions, @abdulmth this might be the conclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PhilippGackstatter and I agreed in a call that we might want to keep the implementation as-is since it's not trivial to implement the delete methods because it must deal with IotaDID and Url objects rather than enum variants (for example in detach_relationships). We also don't wanna use Vec to keep parity with the return values of the getter methods.

}

impl Update {
Expand Down Expand Up @@ -269,6 +278,12 @@ impl Update {

state.document_mut().remove_service(service_url)?;
}
Self::SetController { controllers } => {
state.document_mut().set_controller(controllers);
}
Self::SetAlsoKnownAs { urls } => {
*state.document_mut().also_known_as_mut() = urls;
}
}

state.document_mut().metadata.updated = Timestamp::now_utc();
Expand Down Expand Up @@ -406,3 +421,13 @@ impl_update_builder!(
DeleteService {
@required fragment String,
});

impl_update_builder!(
SetController {
@required controllers Option<OneOrSet<IotaDID>>,
});

impl_update_builder!(
SetAlsoKnownAs {
@required urls OrderedSet<Url>,
});