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: Implement "pass_argument_to_wasm" for remaining types #4979

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

Acaccia
Copy link
Contributor

@Acaccia Acaccia commented Jul 19, 2024

This is the stacks-core part of clarity-wasm's PR 443.

It implements the remaining types and fixes the current implementation that did not respect the ABI.

The tests are not ready yet in the original repo.

@Acaccia Acaccia self-assigned this Jul 19, 2024
@Acaccia Acaccia marked this pull request as ready for review July 23, 2024 07:39
@Acaccia Acaccia requested a review from a team as a code owner July 23, 2024 07:39
Copy link

@csgui csgui left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

This looks great, assuming we add tests in the clarity-wasm repo. I just had one request for an additional check.

@@ -539,21 +539,16 @@ pub fn call_function<'a, 'b, 'c>(

// Convert the args into wasmtime values
let mut wasm_args = vec![];
for arg in args {
for (arg, ty) in args.iter().zip(func_types.get_arg_types()) {
Copy link
Contributor

@obycode obycode Jul 23, 2024

Choose a reason for hiding this comment

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

Can we add a check that args and func_types are the same size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this is useful there, the typechecker should catch the inequality before that stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but aren't we going with a "don't trust the type checker" policy? Or maybe "trust but verify"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right :D

@Acaccia
Copy link
Contributor Author

Acaccia commented Jul 23, 2024

assuming we add tests in the clarity-wasm repo

I am, and there are many bugs for this contract_call? function. @obycode @csgui Is there any possibility to start the regtest from this branch, while I keep working on bug fixing?

@obycode
Copy link
Contributor

obycode commented Jul 23, 2024

Is there any possibility to start the regtest from this branch, while I keep working on bug fixing?

Yup, @csgui is working on that, but running into some strange CI issue to build the image (example)

@csgui csgui merged commit 381398f into feat/clarity-wasm-develop Jul 25, 2024
1 check passed
@csgui csgui deleted the feat/finish-pass-argument-to-wasm branch July 25, 2024 12:05
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.

None yet

3 participants