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

Add Wasm Stardust Client #975

Merged
merged 35 commits into from
Aug 22, 2022
Merged

Add Wasm Stardust Client #975

merged 35 commits into from
Aug 22, 2022

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Aug 18, 2022

Description of change

Add a client implementation for the Stardust Wasm bindings. The client helper traits are split following the suggestion in #963 (comment). The trait names are pretty arbitrary, open to suggestions.

Supersedes #969. Previous PR used @iota/iota.js, which does not currently implement automatic input and output selection, so that was previously manually implemented in TypeScript to match the behaviour of iota.rs in #969. To avoid maintaining that complex logic, we opted to go with Wasm bindings to iota.rs ourselves.

This uses unofficial iota.rs Wasm bindings, We chose not to use the official iota.rs Node.js bindings initially since they don't work in the browser and require Rust to install.

This also requires importing several new npm packages: @iota/iota.js, and @iota/types. But we would need to import them if using iota.js or iota.rs Node.js bindings too, unless we duplicate certain functionality from them (even then @iota/types would still be required).

Added

  • Add StardustIdentityClient trait.
  • Add StardustIdentityClientExt trait.
  • Add "client" feature for the new traits without the iota-client dependency.
  • Add WasmStardustIdentityClient.
  • Add StardustIdentityClient TypeScript wrapper class.
  • Add examples-stardust Wasm examples.

Changed

  • Move most StardustClientExt methods to the StardustIdentityClient traits.
  • Replace private extract_documents_from_block function with public StardustDocument::unpack_from_block.
  • Change StardustClientExt to error on network mismatch.
  • Change delete_did_output to promote the block until included.
  • Change delete_did_output to transfer native tokens too.
  • Change StardustDocument to allow empty state metadata.
  • Change state metadata fieldnames to use doc and meta, instead of document, metadata.

Links to any relevant issues

Resolves #950.

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

Run the new Wasm Stardust examples:

npm run build
npm run test:stardust

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

Add allow_empty to StardustDocument::unpack.
Sort inputs.
Add some block validation checks.
Fix unlocking logic.
Fix consumeAmount, remainderAmount calculations.
Remove timestamps when unpacking an empty DID Document.
@cycraig cycraig added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Aug 18, 2022
@cycraig cycraig requested a review from abdulmth August 18, 2022 15:28
@cycraig cycraig self-assigned this Aug 18, 2022
@cycraig cycraig changed the title Feat/wasm shimmer client Add Wasm Stardust Client Aug 18, 2022
@cycraig
Copy link
Contributor Author

cycraig commented Aug 18, 2022

Rust Stardust tests are failing due to the runner being banned from the faucet API.

https://github.com/iotaledger/identity.rs/runs/7902204219?check_suite_focus=true

@cycraig cycraig mentioned this pull request Aug 18, 2022
10 tasks
@cycraig cycraig added this to the v0.7 Features milestone Aug 18, 2022
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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. Thanks for the massive effort here!

/// This trait is not intended to be implemented directly, a blanket implementation is
/// provided for [`StardustIdentityClient`] implementers.
#[async_trait::async_trait(? Send)]
pub trait StardustIdentityClientExt: StardustIdentityClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we do the renaming from stardust to iota, I assume we'll get rid of the "Stardust" prefix.

So just IdentityClientExt or ClientIdentityExt would be good for this trait.

Since the other part of the trait, what is currently called StardustClientExt is used for modifying the ledger (publishing, deleting) we could call it ClientIdentityWriteExt or something to clarify its modifying nature. Just throwing ideas around.

Copy link
Contributor Author

@cycraig cycraig Aug 22, 2022

Choose a reason for hiding this comment

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

It should just match whatever the name is on the Rust side, likely IotaClientExt/IotaIdentityClient, to avoid ambiguity with other DID methods in the future.

Probably just:

  • IotaClient for the get_network_hrp, get_alias_output, get_rent_structure methods (since iota-client uses Client).
  • IotaIdentityClient for new_did_output etc.
  • IotaIdentityClientExt/IotaIdentityPublish for publish_did_output, delete_did_output? I wouldn't use Write since it doesn't match the terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense 👍

// Deactivation may only be performed by the state controller of the Alias Output.
let deactivated_output: AliasOutput = client.deactivate_did_output(&did).await?;

// Optional: reduce and reclaim the storage deposit, sending the tokens to the state controller.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does that work? If the tokens no longer needed for the storage deposit are not enough to cover the deposit for a new basic output, that would fail, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? iota.rs will try choose another Basic Output to combine it with to avoid dust outputs. I wrote logic to that effect in the previous PR too. Yes it can fail otherwise, but it's not generally the concern of the library, hence why we don't do it automatically.

Comment on lines -1 to -8
comment_width = 120
format_code_in_doc_comments = true
license_template_path = ".license_template"
max_width = 120
normalize_comments = false
normalize_doc_attributes = false
tab_spaces = 2
wrap_comments = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was that removed?

Copy link
Contributor Author

@cycraig cycraig Aug 22, 2022

Choose a reason for hiding this comment

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

It kept giving problems due to being a symlink and rustfmt uses the config file in parent directories automatically, so this was an unnecessary copy anyway.

"node-fetch": "^2.6.7"
},
"peerDependencies": {
"@cycraig/iota-client-wasm": "^0.5.0-alpha.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be moved to the @iota namespace or will that happen if and when the iota.rs team takes over those bindings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When or if iota.rs allows me to merge in the bindings. I just put it under my account to avoid polluting the global namespace for now, it can be changed to whatever.

bindings/wasm/examples-stardust/src/ex3_deactivate_did.ts Outdated Show resolved Hide resolved
bindings/wasm/examples-stardust/src/ex4_delete_did.ts Outdated Show resolved Hide resolved
@cycraig cycraig removed the request for review from abdulmth August 22, 2022 08:12
@cycraig cycraig merged commit 0d5ed61 into dev Aug 22, 2022
@cycraig cycraig deleted the feat/wasm-shimmer-client branch August 22, 2022 10:32
@cycraig cycraig mentioned this pull request Aug 22, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
Development

Successfully merging this pull request may close these issues.

[Task] Implement Client for Stardust
3 participants