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

Allow using a different serialization format for the initial state and the Rust WASM<->JS bridge #302

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

noomly
Copy link
Contributor

@noomly noomly commented Dec 22, 2022

Implements the idea discussed here #284 (comment).

It allows the usage of alternative serialization formats to JSON for the initial state and the communication between WASM & JS. It implements Messagepack as a first alternative and is currently only focused on bringing this to Rust contracts but it lays the foundations for other formats & contracts.

This PR is still a WIP but I thought I'd submit a PR so that we can discuss the implementation details.

@ppedziwiatr
Copy link
Contributor

I've briefly reviewed the PR and I really like the implementation. The only thing that bothers me is the c/p of the rust-wasm-imports..it will be a bit painful to maintain...

@noomly
Copy link
Contributor Author

noomly commented Dec 23, 2022

Yeah I agree, there's definitely something to be done here. I'm still not sure what though; some functions implementations clash between the original json and the msgpack version... There are definitely some common parts that could be shared but honestly, it's probably still going to be a pain to maintain even if we try to merge the two rust-wasm-imports.

Maybe we could think of another strategy for the way this is handled (at least for Rust contracts, I haven't taken a close look at how it's handled for Go & AS). As I understand it, the only reason the glue code is reconstructed is to expose JS functions to WASM like the various SmartWeave functions & log, right? Have you considered trying to inject these functions by doing some light parsing on the glue code and leaving everything else intact for example?

@ppedziwiatr
Copy link
Contributor

ppedziwiatr commented Dec 23, 2022

  1. I believe the Go and AS will be removed in the near future - there's exactly ZERO interest in the community in writing contracts in those languages (plus both Go and AS have some technical issues in case of WASM); and we're simply unable to properly maintain all the versions.
    It may look nice with the "hey, you can now write the code in many different langs, blah blah blah", but the reality is much more complicated - and in practice both Go and AS are kinda..useless (go has issues with big output wasm binary files, AS has issues with a (lack of) standarizded way of running async code (among other things...)).

  2. I'm not sure what do you understand by "light parsing"...this whole mess (yes, I'm aware that it is a mess) with WASM glue code is caused by the fact, that wasm-bindgen mangles the function names in the wasm module with each compilation - I would have to first read (i.e. eval) the generated js with the glue code - which is kinda risky, cause I would have to trust some external code...
    That's why this whole 'mapping' is done (the SDK runtime is 'static')
    https://github.com/warp-contracts/warp/blob/main/src/core/modules/impl/wasm/rust-wasm-imports.ts#L287

But now that I read again the rustwasm/wasm-bindgen#1128 (comment) issue (which is the exact same issue we're facing in the SDK) - maybe there's some better way (e.g. rustwasm/wasm-bindgen#1128 (comment)).

We're open for PRs!

@noomly
Copy link
Contributor Author

noomly commented Dec 23, 2022

  1. Good to know! Less mess to maintain :)
  2. I understand the security concern of using the glue code as-is; that discards my idea then.

Do you consider this issue of glue code handling critical for this PR or could it be researched and addressed in separate one at a later time?

@ppedziwiatr
Copy link
Contributor

ad. 2 - this should be done/refactored within a separate issue.

@ppedziwiatr
Copy link
Contributor

#309, @rpiszczatowski will try to refactor this crap :-)

@ppedziwiatr
Copy link
Contributor

@noomly , I believe we could merge this PR as is...the only thing missing is an integration test for the new format...
sth similar to https://github.com/warp-contracts/warp/blob/main/src/__tests__/integration/wasm/rust-deploy-write-read.test.ts

@noomly
Copy link
Contributor Author

noomly commented Jan 3, 2023

hey @ppedziwiatr , I'll be back from vacation on the 9th, I'll be able to add the tests then.

@ppedziwiatr
Copy link
Contributor

hey @ppedziwiatr , I'll be back from vacation on the 9th, I'll be able to add the tests then.

no rush :-)

@ppedziwiatr
Copy link
Contributor

btw. @noomly I've released the 1.2.48-beta.1 of the internal writes refactor (#311) - which is a prerequisite to a more advanced optimization for WASM (like reusing the handlers, instead of recreating them and initializing the internal wasm module state every time)

Feel free to test it out. I'll try to return to the #288 this week.

@noomly
Copy link
Contributor Author

noomly commented Jan 17, 2023

Great! I'll get to this when I'm done with this PR :)

By the way, while running the typechecker I've realized that I made the stateFormat field of CommonContractData non-optional; which breaks calls to warp.createContract.deploy as they have to now include this new field. I feel like it would be better to have this field being explicitly set by the user as it'd depend on their contract's implementation; although it would break existing calls to deploy.

What do you think? Should we make it an optional field that defaults to JSON or should we leave it as a required field?

@noomly noomly marked this pull request as ready for review January 23, 2023 17:16
@ppedziwiatr
Copy link
Contributor

What do you think? Should we make it an optional field that defaults to JSON or should we leave it as a required field?

We can't break backwards compatibility here (also - probably 99.9% of the users will still use the json), so please set the default value to JSON.

Thanks!

@ppedziwiatr
Copy link
Contributor

@rpiszczatowski we need to return to this :-)

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.

2 participants