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

Miden SDK proof-of-concept using Wasm component model #79

Merged
merged 15 commits into from
Jan 9, 2024

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Dec 11, 2023

Close #72

This PR adds a WIT implementation of Miden SDK and a basic wallet test using the Wasm component model.

Design decisions

Marking call-able functions (account type)

I chose to mark the whole Wasm component as having call-able functions. The benefits vs. marking individual functions as call-able are:

  • Data segments contain the data for all functions in the component, so we have lower chances of loading unwanted data;
  • Marking the individual functions can be done only via an attribute in the doc string, which is more error-prone than marking the whole component. Plus, I'm pretty sure we'll want to treat the account code specially, so we'll have to mark the whole component anyway.
  • The whole component is marked via exporting a "marker" interface (see account interface defined in miden.wit(SDK) and used in basic-wallet.wit). Alternatively, we can use metadata in the Cargo.toml where will define some properties of the Miden component anyway.

Wasm components are limited to one exported world, so we cannot have both call-able and exec-able functions in the same component with this approach. That is the drawback of this approach.
The helper function are expected to be linked as a regular Rust crate to avoid the CanonicalABI overhead when called.

Wasm resource does not play well with call, so we should not use it

I found a problem that I believe prevents us from using the resource for our types. As an experiment, I made Asset a resource and since the implementation is opaque (see) and not on the WIT "level" (see) there is no knowledge of the size of the Asset instance in the compiled Wasm component. The resource handle is a pointer to the instance. Thus making it impossible to pass the resource value via a call.
As I understood from the generated code, the resource is passed as a pointer to the instance, and since its methods are implemented in the Wasm component where the resource instance "resides," they can access its memory directly no matter from what component they were called.

Use "library" Wasm component for note instead of a "command"

When using "command" Wasm component the runtime is quite sizable and pulls in arg parsing and thread-related setup that we don't need. So I decided to use a "library" Wasm component instead that is implementing a note interface (see).

@greenhat greenhat force-pushed the greenhat/rollup-poc-wit-i72 branch 3 times, most recently from fb08550 to 764f858 Compare December 18, 2023 07:56
@greenhat greenhat marked this pull request as ready for review December 18, 2023 10:31
@greenhat greenhat requested a review from bitwalker December 18, 2023 10:32
@greenhat greenhat force-pushed the greenhat/rollup-poc-wit-i72 branch from 792c360 to 7c8f390 Compare December 26, 2023 13:58
@greenhat greenhat force-pushed the greenhat/rollup-poc-wit-i72 branch from 7c8f390 to 4711d61 Compare January 2, 2024 07:55
@greenhat
Copy link
Contributor Author

greenhat commented Jan 9, 2024

@bitwalker Following our discussion, I removed the component with the account's helper functions.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

A couple small notes, but I think this looks good so far!

We discussed the following on our compiler call today, but just to document the decisions somewhere relevant:

1.) The need to avoid resource at the moment is due to missing features in the VM, for which we have an outstanding proposal that may or may not enable the use of resource in the future - but for the time being it can't be used.

2.) As you noted, all public exports of a component should be called, and we formalized that decision today (though it isn't final if we decide to revisit that in the future for some reason).

sdk/wit/miden.wit Outdated Show resolved Hide resolved
sdk/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

LGTM!

@bitwalker bitwalker merged commit 4197b7e into main Jan 9, 2024
1 check passed
@bitwalker bitwalker deleted the greenhat/rollup-poc-wit-i72 branch January 9, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PoC account code compilation (WIT version)
2 participants