Add Wasm `Account` `Storage` interface #597

HenriqueNogara · 2022-01-17T19:30:23Z

Description of change

Expose Storage trait in the bindings.

Links to any relevant issues

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

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

…ount

…ount # Conflicts: # identity/Cargo.toml

…ount

…torage

…Value, JsValue> to an AccountResult

bindings/wasm/src/account/identity/did_lease.rs

PhilippGackstatter

Thanks for the memory store implementation in TS!

I still have two primary concerns.

There are still a lot of types that seem unnecessarily exposed. On the flipside, I'm concerned we might be missing places where the types that appear in the storage trait do not have (de)serialization functions exposed. Ideally, we would verify that these types can all be (de)serialized in the memory_storage.ts implementation, by turning it into a file store, or something like that. In-memory by itself also seems fine, as long as all types go through serialization at some point.
The other bigger concern is around documentation of functions, mostly relating to stating the base58 requirement, and if we actually want to keep that, since it requires lots of encoding from implementors, while we could just as easily expose these things as bytes.

bindings/wasm/Cargo.toml

bindings/wasm/examples-account/src/memory_storage.ts

PhilippGackstatter · 2022-02-19T09:55:01Z

identity-account/src/storage/traits.rs

+    #[cfg_attr(not(feature = "wasm"), async_trait)]
+    #[cfg_attr(feature = "wasm", async_trait(?Send))]


I suspect that we will need this Send/?Send feature for future bindings as well (Python comes to mind), and so we could introduce a separate feature for this, so it's not tied to wasm exclusively. Maybe non-send-storage, or some better name 😅? Anyway, this might be overdoing it for now. I think we can change this in non-breaking ways in the future.

bindings/wasm/src/account/storage/traits.rs

PhilippGackstatter · 2022-02-19T10:17:39Z

bindings/wasm/src/account/storage/traits.rs

+  /** Deletes the keypair specified by `location`.*/
+  keyDel: (did: DID, keyLocation: KeyLocation) => Promise<void>;


Is keyDel supposed to be idempotent? This came up when I looked over the memory_storage.ts implementation, which behaved slightly differently from the Rust MemStore. Idempotency seems like a good idea to me here, and we should state that in the doc as well, then. That would also require changes to all implementations. Feel free to leave this for a later PR, in which case we should open an issue, so it's not forgotten about. I can open the issue if there is agreement for that.

PhilippGackstatter · 2022-02-19T10:21:24Z

bindings/wasm/src/account/storage/traits.rs

+  /** Returns the last generation that has been published to the tangle for the given `did`.*/
+  publishedGeneration: (did: DID) => Promise<Generation>;
+
+  /** Sets the last generation that has been published to the tangle for the given `did`.*/
+  setPublishedGeneration: (did: DID, generation: Generation) => Promise<void>;


I assume this will be removed in the napi or epic branch?

Yes. Handled on #660

bindings/package-lock.json

PhilippGackstatter

A few minor notes on the latest commits, will do another pass in a bit.

bindings/wasm/src/account/storage/traits.rs

bindings/wasm/src/core/ed25519.rs

bindings/wasm/src/tangle/message_did.rs

PhilippGackstatter

This looks good to me now. Thank you very much for addressing all the comments! This is a really solid Rust "trait export" to Wasm, imho.

I honestly may have missed a thing or two because of the split in epic branch and napi stronghold, but since this goes through more reviews in those PRs I believe that should be okay.

cycraig

I assume things like DIDLease and publishedGeneration have been removed in the other PR.

Otherwise it seems good overall, few general comments.

cycraig · 2022-02-22T14:26:02Z

bindings/wasm/examples-account/src/memory_storage.ts

+    public async setPassword(_encryptionKey: Uint8Array): Promise<void> {}
+
+    public async flushChanges(): Promise<void> {}


We might need a comment here explaining that these functions are not necessary here but are needed for other Storage implementations. I.e. it's fine to have no-op functions?

Aside: still not sure if we're going to keep setPassword, since I'm hoping that Stronghold will take over password management. Basically authentication should be part of the implementation rather than the interface, ideally.

Good point, it should be public async flushChanges(): Promise<void> {return Promise.resolve()}}

Fully agree setPassword should be removed, but perhaps in a follow-up PR? The changes required are minimal, I think, so it could be done here as well. I'm fine either way. Passwords should be passed to storage constructors and not be required post-creation anymore, which will, for example, be the case with the upcoming stronghold refactor. Some storages might not even need a password, so it shouldn't be part of the interface.

I'm fine with any changes being added to an issue or appended to #669, as long as there's something to keep track of them.

cycraig · 2022-02-22T14:36:19Z

identity-account/src/error.rs

+  #[cfg(feature = "wasm")]
+  #[error("JsValue serialization error: {0}")]
+  SerializationError(String),
+  #[cfg(feature = "wasm")]
+  #[error("javascript function threw an exception: {0}")]
+  JsError(String),


Regarding errors for the Storage interface, I think it's a bit unwieldy to restrict implementers to the top-level AccountError.

Maybe we can have a sub-error enum for the Storage and map them to a single AccountError internally? This would be similar to the Error in the diff crate, where each variant maps to a function.

#[derive(Debug, thiserror::Error, strum::IntoStaticStr)] pub enum Error { #[error("Diff Error: {0}")] DiffError(String), #[error("Merge Error: {0}")] MergeError(String), #[error("Conversion Error: {0}")] ConversionError(String), }

We could use strings too or retain the error source with e.g. Box<dyn Error> to preserve reporting, as mentioned in #636.

This would basically eliminate the need for these feature-gated JsError/SerializationError variants since we could define errors in the Wasm bindings themselves that are bubbled up to the correct variant with no loss of context (hopefully).

This would benefit future bindings and Storage implementers using the core Rust library too.

cc @olivereanderson

There definitely needs to be a separate StorageError, and I really like your suggestion with Box<dyn Error>. Still, I think this needs a good amount of thought put into it to provide a reasonably abstract error that can accommodate not just stronghold and memory stores, which is why I would suggest doing this in a separate PR and leave the current approach as an acceptable temporary solution. I'd be happy to open an issue for it.

Sure, it can be added to #669 or its own issue. I wouldn't merge this without having a tracking issue for what's left, however.

cycraig · 2022-02-22T14:37:57Z

bindings/wasm/examples-account/src/memory_storage.ts

+        let signature: Uint8Array = Ed25519.sign(data, keyPair.private);
+        return new Signature(keyPair.public, signature)


This discussion is continued in the related issue #669 (comment)

cycraig · 2022-02-22T14:42:10Z

bindings/wasm/src/core/secret_key.rs

+  #[wasm_bindgen(js_name = "fromBase58String")]
+  pub fn from_base58_string(private_key: &str) -> Result<WasmPrivateKey> {
+    let private_key: PrivateKey = decode_b58(private_key).map_err(wasm_error)?.into();
+    let private_key_bytes: [u8; 32] = <[u8; 32]>::try_from(private_key.as_ref())
+      .map_err(|err| AccountError::InvalidPrivateKey(format!("expected a slice of 32 bytes - {}", err)))
+      .wasm_result()?;
+    Ok(WasmPrivateKey(ed25519::SecretKey::from_bytes(private_key_bytes)))
+  }
+
+  /// Returns a base58 encoded string that represents the PublicKey.
+  #[wasm_bindgen(js_name = publicKey)]
+  pub fn public_key(&self) -> String {
+    let public: ed25519::PublicKey = self.0.public_key();
+    let public_key: PublicKey = public.to_bytes().to_vec().into();
+    encode_b58(&public_key)
+  }


Mentioned in #669 that we should probably move to UInt8Array rather than Base58-BTC encoded strings but that's a larger refactor for the Wasm bindings. We should really use the same types everywhere though, so WasmPrivateKey in WasmDocument and WasmKeyPair too?

Doesn't need to be addressed in this PR, maybe added to #669 as a discussion point?

cycraig · 2022-02-22T14:44:14Z

bindings/wasm/src/error.rs

+/// Convenience struct to convert Result<JsValue, JsValue> to an AccountResult<_, AccountError>
+pub struct JsValueResult(pub(crate) Result<JsValue>);


See the previous comment on possibly refactoring Storage errors, it might make JsValueResult unnecessary since errors would be mapped to e.g. StorageError variants internally?

cycraig · 2022-02-22T14:47:52Z

identity-account/src/types/signature.rs

+#[derive(Clone, Debug, Deserialize)]
 pub struct Signature {
  pub(crate) pkey: PublicKey,
  pub(crate) data: Vec<u8>,


I think I commented about this on the other PR but do we need Signature to contain the PublicKey as well? I don't think it's used. Is it needed for future signature schemes?

Not related to the changes in this PR but it is about the Storage implementation.

No we don't need it. In fact, this is unused in the account, too. The account Signature type could need a refactoring to simply return either a Vec<u8> or a newtype wrapper around it. The pkey is simply discarded in RemoteSign. Could be part of a number of storage refactors mentioned before (redesign errors, remove set_password, refactor Signature)?

I'm fine with any changes being added to an issue or appended to #669.

Abdulrahim Al Methiab and others added 23 commits December 7, 2021 18:01

compile account in WASM

f3b1fa6

Merge remote-tracking branch 'origin/dev' into feat/wasm-bindings-acc…

e756160

…ount

PoC account and accountBuilder wasm bindings

6958f5f

WASM binding PoC create_account()

bcb4379

Merge remote-tracking branch 'origin/dev' into feat/wasm-bindings-acc…

40ccb57

…ount # Conflicts: # identity/Cargo.toml

OneOrMany bindings PoC

0c093d2

Merge remote-tracking branch 'origin/dev' into feat/wasm-bindings-acc…

41cb60b

…ount

bind create_method, delete_method, create_service

6e9c903

add bindings for attach_method_relationship

afd09f6

fix lint issues

957f4c6

remove empty lines

f898303

add detach_method_relationship

b221ff6

add bindings for delete_service

ee01e52

implement wasm_bindgen types needed for the storage trait

bc53a62

fix MethodRelationship type

c170f97

refactor AccountBuilder

fa64fea

remove Sync + Send bound in the Storage trait, impl storage for WasmS…

94b9d64

…torage

add create_did Account example

4e86c46

add manipulate_did account example

6d943f8

add lazy account example

04bbb27

add macro for allowing two different storage trait

f1f8956

merge with latest dev

d170ef2

enable wasm for identity account, impl macro for converting Result<Js…

6cd3ec6

…Value, JsValue> to an AccountResult

HenriqueNogara added Enhancement New feature or improvement to an existing feature Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Added A new feature that requires a minor release. Part of "Added" section in changelog labels Jan 17, 2022

Henrique Nogara added 4 commits January 17, 2022 17:01

add license

402f905

add missing docs

0717213

rename extern type

25584e1

improve js naming, add missing wasmfragment type

ed7608e

Abdulrahim Al Methiab and others added 4 commits February 7, 2022 23:53

use builder instead of AccountConfig

7cbc644

implement From for SignatureOptions

aec32e9

reverse to original impl From<WasmSignatureOptions>

6850fb8

solve merge with account bindings

5e9cdd2

HenriqueNogara changed the base branch from feat/wasm-bindings-account to epic/wasm-bindings-account February 18, 2022 13:48

Henrique Nogara added 2 commits February 18, 2022 11:09

merge with epic branch

68b251a

move update files

d041967

abdulmth reviewed Feb 18, 2022

View reviewed changes

bindings/wasm/src/account/identity/did_lease.rs Outdated Show resolved Hide resolved

add cfg_attr to avoid CI warnings

9fc551c

HenriqueNogara requested a review from PhilippGackstatter February 18, 2022 17:21

PhilippGackstatter suggested changes Feb 19, 2022

View reviewed changes

PhilippGackstatter mentioned this pull request Feb 19, 2022

Make Wasm support dependent on target_arch rather than feature #666

Merged

10 tasks

Henrique Nogara added 2 commits February 21, 2022 12:58

merge with epic - remove didlease conflicts

a54ede7

add comments, remove useless code, rename types

b557e73

HenriqueNogara requested a review from PhilippGackstatter February 21, 2022 21:34

eike-hass reviewed Feb 22, 2022

View reviewed changes

bindings/package-lock.json Outdated Show resolved Hide resolved

PhilippGackstatter reviewed Feb 22, 2022

View reviewed changes

Henrique Nogara added 3 commits February 22, 2022 09:52

remove file, throw error in ts, remove unused type

ad1779f

improve erro msgs, and comments

86a8585

clippy

28affe7

PhilippGackstatter approved these changes Feb 22, 2022

View reviewed changes

cycraig reviewed Feb 22, 2022

View reviewed changes

merge with epic - docs

c07aa0c

HenriqueNogara merged commit 4b0164c into epic/wasm-bindings-account Feb 22, 2022

cycraig mentioned this pull request Feb 22, 2022

[Task] Wasm Bindings Improvements #669

Open

20 tasks

HenriqueNogara deleted the feat/wasm-storage branch March 3, 2022 12:41

cycraig mentioned this pull request Mar 14, 2022

Epic: Wasm bindings account #681

Merged

10 tasks

cycraig changed the title ~~Feat/wasm storage~~ Add Wasm Account Storage interface Mar 17, 2022

cycraig removed the Enhancement New feature or improvement to an existing feature label Mar 17, 2022

PhilippGackstatter mentioned this pull request Jun 22, 2022

[Task] Design and implement a StorageError #920

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Wasm `Account` `Storage` interface #597

Add Wasm `Account` `Storage` interface #597

HenriqueNogara commented Jan 17, 2022 •

edited

PhilippGackstatter left a comment

PhilippGackstatter Feb 19, 2022

PhilippGackstatter Feb 19, 2022

PhilippGackstatter Feb 19, 2022

HenriqueNogara Feb 21, 2022

PhilippGackstatter left a comment

PhilippGackstatter left a comment

cycraig left a comment

cycraig Feb 22, 2022

eike-hass Feb 22, 2022

PhilippGackstatter Feb 22, 2022

cycraig Feb 22, 2022

cycraig Feb 22, 2022

PhilippGackstatter Feb 22, 2022

cycraig Feb 22, 2022

cycraig Feb 22, 2022

cycraig Feb 22, 2022 •

edited

cycraig Feb 22, 2022

cycraig Feb 22, 2022

PhilippGackstatter Feb 22, 2022

cycraig Feb 22, 2022

		#[cfg_attr(not(feature = "wasm"), async_trait)]
		#[cfg_attr(feature = "wasm", async_trait(?Send))]

		/** Deletes the keypair specified by `location`.*/
		keyDel: (did: DID, keyLocation: KeyLocation) => Promise<void>;

		public async setPassword(_encryptionKey: Uint8Array): Promise<void> {}

		public async flushChanges(): Promise<void> {}

		let signature: Uint8Array = Ed25519.sign(data, keyPair.private);
		return new Signature(keyPair.public, signature)

		/// Convenience struct to convert Result<JsValue, JsValue> to an AccountResult<_, AccountError>
		pub struct JsValueResult(pub(crate) Result<JsValue>);

Add Wasm Account Storage interface #597

Add Wasm Account Storage interface #597

Conversation

HenriqueNogara commented Jan 17, 2022 • edited

Description of change

Links to any relevant issues

Type of change

How the change has been tested

Change checklist

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

cycraig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cycraig Feb 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add Wasm `Account` `Storage` interface #597

Add Wasm `Account` `Storage` interface #597

HenriqueNogara commented Jan 17, 2022 •

edited

cycraig Feb 22, 2022 •

edited