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.

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

[Task] Improve Account::attach/detach_method_relationship #654

Closed
10 tasks
cycraig opened this issue Feb 13, 2022 · 0 comments · Fixed by #775
Closed
10 tasks

[Task] Improve Account::attach/detach_method_relationship #654

cycraig opened this issue Feb 13, 2022 · 0 comments · Fixed by #775
Assignees
Labels
Enhancement New feature or improvement to an existing feature
Milestone

Comments

@cycraig
Copy link
Contributor

cycraig commented Feb 13, 2022

Description

Modify Account::attach_method_relationship and Account::detach_method_relationship:

  • pluralise the names to match the Wasm bindings Account, i.e. attach_method_relationships and detach_method_relationships.
  • allow specifying multiple relationships at once rather than individually, this would allow passing collections of relationships.

Current behaviour:

account
  .update_identity()
  .attach_method_relationship()
  .fragment("key1")
  .relationship(MethodRelationship::Authentication)
  .relationship(MethodRelationship::CapabilityInvocation)
  .apply()
  .await?;

Desired behaviour:

account
  .update_identity()
  .attach_method_relationships()
  .fragment("key1")
  .relationships(&[MethodRelationship::Authentication, MethodRelationship::CapabilityInvocation])
  .apply()
  .await?;

The .relationships implementation can either take a slice, vec, or arbitrary IntoIter generic depending on which we find is most convenient for developers to use.

We should also think about making attaching/detaching relationships easier for developers, since it is currently restricted to non-embedded methods. That is, trying to attach/detach an embedded method always causes an error, we could instead move the method to be a verificationMethod.

Motivation

Porting the Account for the Wasm bindings in #574 demonstrated that using the builder pattern to add/remove multiple method relationships is cumbersome due to ownership/borrowing rules on intermediate structs. This issue is an attempt to make these specific methods more user-friendly and to reduce differences with the Wasm bindings.

E.g. current code needed to add relationships from a list, which is definitely not ergonomic:

// The list is static here but is usually defined programmatically.
let relationships: Vec<MethodRelationship> = vec![
  MethodRelationship::Authentication, 
  MethodRelationship::CapabilityInvocation,
];
// Combining any of these lines angers the borrow checker.
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);
}

attach_relationship.apply().await?;

To-do list

  • rename Account::attach_method_relationship to attach_method_relationships
  • rename Account::detach_method_relationship to detach_method_relationships
  • allow Account::attach_method_relationships to specify multiple relationships at once.
  • allow Account::detach_method_relationships to specify multiple relationships at once.

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • The feature or fix is implemented in Rust and across all bindings whereas possible.
  • The feature or fix has sufficient testing coverage
  • All tests and examples build and run locally as expected
  • Every piece of code has been document according to the documentation guidelines.
  • If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
  • If the feature is not currently documented, a documentation task Issue has been opened to address this.
@cycraig cycraig added the Enhancement New feature or improvement to an existing feature label Feb 13, 2022
@cycraig cycraig added this to the v0.5 Features milestone Feb 17, 2022
@abdulmth abdulmth self-assigned this Mar 23, 2022
@abdulmth abdulmth linked a pull request Mar 29, 2022 that will close this issue
4 tasks
@abdulmth abdulmth moved this from Backlog to In Progress in IOTA Identity - Framework Developments Mar 29, 2022
Repository owner moved this from In Progress to Done in IOTA Identity - Framework Developments Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement to an existing feature
Projects
Development

Successfully merging a pull request may close this issue.

2 participants