search

iotaledger / identity.rs

Implementation of the Decentralized Identity standards such as DID and Verifiable Credentials by W3C for the IOTA Tangle.
https://www.iota.org
Apache License 2.0
298 stars 84 forks source link

[Task] Wasm Bindings Improvements #669

Open HenriqueNogara opened 2 years ago

HenriqueNogara commented 2 years ago

Description

Remove exposure of Ed25519.sign, remove Storage Send/Sync ties to the wasm feature, and make keyDel in typescript compatible with the Rust code. Refactor the Storage trait to return better errors, and simplify its API.

Motivation

  1. Users should be able to sign data without relying on our code.
  2. We might need the Storage trait not to be Send/Sync for other bindings (e.g Python) as well, so we should not tie its bounds to the wasm feature.
  3. Js implementation of MemStore keyDel slightly differs from the Rust code regarding idempotency.
  4. identity_account::types::signature::Signature::pkey is not used in the Rust code.
  5. Storage::set_password is no longer needed.
  6. Improve error feedback. See: https://github.com/iotaledger/identity.rs/pull/597#discussion_r812013066

To-do list

Change checklist

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

cycraig commented 2 years ago

Users are already able to sign data without relying on our code.

E.g. using TweetNaCl

const key = new KeyPair(KeyType.Ed25519);
const seed = bs58.decode(key.private);
let naclKeyPair = nacl.sign.keyPair.fromSeed(seed);
let challengeBytes = Uint8Array.from([1,2,3,4]);
let signature = nacl.sign(challengeBytes, naclKeyPair.secretKey);
console.log(`Signature: ${bs58.encode(signature)}`);

I do think we should switch to using UInt8Array to represent the private/public keys though, from Base58-BTC encoded strings. This is because it's the type existing libraries like TweetNaCl expect as input and to avoid requiring developers bring in Base58 dependencies to decode the strings we return. Alternative would be to expose the dencode_b58, decode_multibase functions too, which might be useful for Storage implementers regardless.

Advantages of exposing Ed25519::sign:

Disadvantages:

We have had requests to include other features in identity.rs such as BIP-39 mnemonics, so I think some developers might appreciate being able to use same the functions we do internally out of convenience. See also this comment: https://github.com/iotaledger/identity.rs/pull/597/files#r811781156

We might need the Storage trait not to be Send/Sync for other bindings (e.g Python) as well, so we should not tie its bounds to the wasm feature.

I agree. Some conditional compilation required now just for wasm might be beneficial for other use-cases too, so we can use other features (named more specifically like storage-sync for example) on top of target-based compilation. Edit: as long as enabling --all-features does not break compilation.

Verify the need to return an error on KeyDel

I think all Storage functions need to be async and fallible to account for unknowable remote implementations where every little network request could fail. Not the point the issue raised, it's about idempotency and consistency between Rust and bindings behaviour.

cycraig commented 2 years ago

This comment on #660 may have been missed regarding the Stronghold bindings package namespace: https://github.com/iotaledger/identity.rs/pull/660/files#r816770207

Edit: resolved.

cycraig commented 2 years ago

As per internal discussions, the following tasks were marked as either important or easy to do prior to 0.5.0.

Each needs its own issue or PR now.