Set controller, alsoKnownAs fields from Account

[abdulmth]

Description of change

The change allows setting the controller and alsoKnownAs fields in the Account.

Links to any relevant issues

Relevant issue #647

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

locally tested in examples.

[cycraig]

Looks good but needs tests similar to the existing Account update methods.

[cycraig]

Looks good to me, thanks for the changes.

[PhilippGackstatter]

We should discuss which style to follow for the update types.

[PhilippGackstatter]

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.

[cycraig]

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

[PhilippGackstatter]

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.

[cycraig]

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);
  }

[PhilippGackstatter]

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.

[cycraig]

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.

[PhilippGackstatter]

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.

[abdulmth]

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

[PhilippGackstatter]

After discussion, it seems the most consistent solution is to keep OneOrSet and OrderedSet, to avoid having different parameter and return types, so I'm fine with this current solution.