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

Clarity wasm runtime #3880

Closed
wants to merge 71 commits into from
Closed

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Aug 29, 2023

This is a WIP and not intended to be merged.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (becc2aa) 58.31% compared to head (9b9b158) 58.31%.
Report is 1 commits behind head on feat/2.4-clarity-wasm.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           feat/2.4-clarity-wasm    #3880   +/-   ##
======================================================
  Coverage                  58.31%   58.31%           
======================================================
  Files                          1        1           
  Lines                        571      571           
======================================================
  Hits                         333      333           
  Misses                       238      238           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

obycode and others added 9 commits August 31, 2023 15:32
Handle passing arguments into a contract call using the Wasm runtime and
handle returning a value from the contract call.
The pass that initializes a contract should have the contract analysis
available, but when a contract is being called, only use the
`ContractContext`. This requires adding the return type of a function to
this context.
@obycode obycode changed the base branch from feat/clarity-wasm to feat/2.4-clarity-wasm September 1, 2023 01:26
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

Biggest issues are error handling, but I guess we can write that off for now since we've still got implementations to do? Would be nice to have the correct error variants being returned anyway, I think, instead of panicing (so we don't forget any).

clarity/src/vm/contexts.rs Show resolved Hide resolved
clarity/src/vm/contracts.rs Outdated Show resolved Hide resolved
clarity/src/vm/functions/define.rs Outdated Show resolved Hide resolved
src/chainstate/stacks/db/transactions.rs Outdated Show resolved Hide resolved
clarity/src/vm/clarity_wasm.rs Outdated Show resolved Hide resolved
This comment clarifies that this function should not be used during
normal execution.
All error cases should be properly handled and generate an error to be
handled by the caller. Any `Wasm` errors should never happen in
production and would indicate an implementation problem if they show up.
src/chainstate/stacks/db/transactions.rs Show resolved Hide resolved
"clarity",
"define_function",
|mut _caller: Caller<'_, T>, _kind: i32, _name_offset: i32, _name_length: i32| {
// FIXME: I don't understand why I have to write this like this
Copy link
Member

Choose a reason for hiding this comment

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

@Acaccia maybe you know?

Copy link
Contributor

@Acaccia Acaccia Sep 6, 2023

Choose a reason for hiding this comment

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

Yes I do, it's a bit tricky.
The closure here should implement IntoFunc. For the arguments, no issue, but for the return type, here you are returning a Result, which does not implement WasmRet.
What does implement WasmRet however is WasmTime::Result, which they say is a wrapper for Result<T, anyhow::Error.
So if I add anyhow as dep and rewrite the closure like this, it works:

|mut _caller: Caller<'_, T>, _kind: i32, _name_offset: i32, _name_length: i32| {
    Err::<(), _>(anyhow!(Error::Wasm(WasmError::DefineFunctionCalledInRunMode)))
}

Why it worked with a ? is because this calls an implicit "into" on the error, converting it automatically to a anyhow::Error.
Also, I had to specify the type for Err, since we don't have the Ok(()) anymore, and the compiler cannot infer the complete type for the Result.

clarity/src/vm/clarity_wasm.rs Show resolved Hide resolved
obycode and others added 23 commits September 27, 2023 12:05
For an `optional`, do not try to create a value unless the indicator is
`1`. Similarly, for `response`, do not try to create a value from the
`ok` value if the indicator is `0` or from the `err` value if the
indicator is `1`.
Added a bool to specify whether or not the representation (offset +
length) should be included in the size for in-memory values.
This is the return value from the last expression at the top-level of
the contract.
There are now tests for the functions that read and write the Wasm
memory!
This includes `keccak256`, `sha512`, and `sha512/256`.
This function is useful for debugging wasm code.
@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

@obycode obycode mentioned this pull request Nov 28, 2023
@obycode
Copy link
Contributor Author

obycode commented Nov 28, 2023

Replaced by #4093

@obycode obycode closed this Nov 28, 2023
@hugocaillard hugocaillard mentioned this pull request May 7, 2024
@hugocaillard hugocaillard deleted the feat/clarity-wasm-runtime branch July 12, 2024 14:10
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.

6 participants