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

Complete Document Wasm bindings #679

Merged
merged 20 commits into from
Mar 7, 2022
Merged

Complete Document Wasm bindings #679

merged 20 commits into from
Mar 7, 2022

Conversation

abdulmth
Copy link
Contributor

Description of change

Adds WASM bindings for some of the unported methods in IotaDocument.

Links to any relevant issues

fixes issue #622

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

Tested the methods locally.

@abdulmth abdulmth added Added A new feature that requires a minor release. Part of "Added" section in changelog Wasm Related to Wasm bindings. Becomes part of the Wasm changelog labels Feb 28, 2022
@abdulmth abdulmth self-assigned this Feb 28, 2022
@abdulmth abdulmth added this to the v0.5 Features milestone Feb 28, 2022
@abdulmth abdulmth linked an issue Feb 28, 2022 that may be closed by this pull request
16 tasks
Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

As discussed, this should be done on the dev branch rather than the Account bindings epic branch.

@abdulmth abdulmth force-pushed the feat/wasm-iota-document branch from d632573 to 6ff932f Compare March 1, 2022 10:28
@abdulmth abdulmth changed the base branch from epic/wasm-bindings-account to dev March 1, 2022 10:28
@olivereanderson
Copy link
Contributor

Would it be possible to include some examples that illustrate how to use this functionality in javascript?

@abdulmth
Copy link
Contributor Author

abdulmth commented Mar 1, 2022

@olivereanderson do you mean examples for the library users or just for the review?

@olivereanderson
Copy link
Contributor

@olivereanderson do you mean examples for the library users or just for the review?

I was thinking for the library users, but it would also be useful for the review.

@abdulmth
Copy link
Contributor Author

abdulmth commented Mar 1, 2022

I was thinking for the library users, but it would also be useful for the review.

@olivereanderson I think we try to keep parity in the examples in Rust and WASM now and since there's no examples for that in Rust, I didn't add them to WASM.

Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Some changes requested, notably regarding attach/detachMethodRelationships.

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
Comment on lines 101 to 118
/// Returns a list of the document controllers.
#[wasm_bindgen(js_name = controller)]
pub fn controller(&self) -> DIDArray {
match self.0.controller() {
Some(controllers) => controllers
.iter()
.cloned()
.map(WasmDID::from)
.map(JsValue::from)
.collect::<js_sys::Array>()
.unchecked_into::<DIDArray>(),
None => js_sys::Array::new().unchecked_into::<DIDArray>(),
}
}

/// Returns a set of the document's `alsoKnownAs`.
#[wasm_bindgen(js_name = alsoKnownAs)]
pub fn also_known_as(&self) -> DIDArray {
Copy link
Contributor

@cycraig cycraig Mar 1, 2022

Choose a reason for hiding this comment

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

The controller and alsoKnownAs fields need setters as well to be equivalent to Rust. We should also consider returning None/null in the accessors if they are not present, since that's more accurate? Latter is optional, we should discuss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Rust it doesn't return an Option so I kept the same behavior. But an empty array is not wrong here in my opinion.

Copy link
Contributor

@cycraig cycraig Mar 2, 2022

Choose a reason for hiding this comment

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

The controller returns an Option, just not alsoKnownAs. Returning an empty array should be fine, but we should allow taking null for the setters to unset them?

/// Returns a reference to the [`IotaDocument`] controllers.
pub fn controller(&self) -> Option<&OneOrSet<IotaDID>>

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
Comment on lines 279 to 280
pub fn attach_method_relationships(&mut self, options: &MethodRelationshipOptions) -> Result<()> {
let relationships: Vec<MethodRelationship> = options
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this use a duck-typed interface? I think it should take a DIDUrl parameter and a custom Typescript type (assuming we want to allow multiple relationships).

This should match the Rust API, so if we want to allow multiple relationships here we should allow it in Rust too.

E.g.

  pub fn attach_method_relationships(&mut self, did_url: &WasmDIDUrl, relationships: &UMethodRelationships) -> Result<()> {
...

#[wasm_bindgen]
extern "C" {
  #[wasm_bindgen(typescript_type = "MethodRelationship | MethodRelationship[]")]
  pub type UMethodRelationships;
}

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 kept the support for multiple relationships since it's quite easy to support here. I think there's another discussion for supporting it in Rust.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed it is a separate discussion (i.e. not for this PR). The counter-argument is that the Account allows adding multiple relationships easily (at least in the Wasm bindings pending #654 for Rust) so it's fine for the Document to only allow adding relationships one-at-a-time given that it's part of the low-level API.

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 changed it to allow only one relationship

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
@abdulmth abdulmth requested a review from cycraig March 2, 2022 17:47
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
Comment on lines +259 to +260
#[wasm_bindgen(js_name = resolveSigningMethod)]
pub fn resolve_signing_method(&mut self, query: &UDIDUrlQuery) -> Result<WasmVerificationMethod> {
Copy link
Contributor

Choose a reason for hiding this comment

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

See issue #686.

Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Minor changes.

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
abdulmth and others added 2 commits March 2, 2022 22:06
Co-authored-by: Craig Bester <craig.bester@iota.org>
@abdulmth abdulmth requested a review from cycraig March 2, 2022 21:33
Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, thanks for the changes.

Minor comments, mostly cosmetic. Previous unresolved comments will be addressed by #686,

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
Comment on lines 202 to 204
/// Return a set of all `Service`s in the document.
#[wasm_bindgen]
pub fn service(&self) -> ArrayService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for the doc-comment to link to Service but replace the content with Services? Like we do in Rust: [Services](Service).

Suggested change
/// Return a set of all `Service`s in the document.
#[wasm_bindgen]
pub fn service(&self) -> ArrayService {
/// Return a set of all {@link Service}s in the document.
#[wasm_bindgen]
pub fn service(&self) -> ArrayService {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, {@link Service Services} achieves exactly that.

bindings/wasm/src/did/wasm_document.rs Outdated Show resolved Hide resolved
bindings/wasm/src/did/mod.rs Show resolved Hide resolved
@cycraig cycraig changed the title complete wasm bindings for IotaDocument Complete Document Wasm bindings Mar 3, 2022
Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@abdulmth abdulmth merged commit 37a48af into dev Mar 7, 2022
@abdulmth abdulmth deleted the feat/wasm-iota-document branch March 7, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release. Part of "Added" section in changelog Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Complete WASM bindings for IotaDocument
3 participants