Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Wasm support #351
Wasm support #351
Changes from 5 commits
1b2139f
07fe1e2
f297f27
52acd70
a976e12
05119dd
f9550e3
40bdcd5
e38398d
272b918
89cab88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running
cargo test --target wasm32-unknown-unknown
with the following code:I encountered a strange error:
this was on ubuntu 20.04. My m1 mac wasn't even able to compile this, so that should be factored into the devux of writing wasm indexer modules since it may require a lot of llvm toolchain config and setup to get this working in most dev environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given fuels-core still largely relies on std, I'm skeptical about whether any of the
fs
methods (e.g. loading the json abi file) will actually be implemented or just panic at runtime. Guess it depends on the runtime we use the wasm in. However, that should be accounted for in a macro billed as wasm_abigen since we don't know where people may attempt to run this publicly exported macro.It seems like the safer option is to perform this abigen as a build step using build-deps in a std env, rather than a normal dep in a wasm environment.
cc @vlopes11 since he also has experience with no_std and might have something to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately, would using the typescript sdk abigen w/ assemblyscript be a more dev friendly experience for writing wasm indexer modules? @digorithm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That first error was because cargo test was trying to run WASM as if it were a native executable... so not gonna work. I put in a WASM test harness to run that test.
I think for mac, the only thing needed is a
brew install llvm
, the one shipped by default does not have a WASM target support.For loading the json abi file, that only happens in the proc macro. Proc macros are actually compiled prior to the crate that's being built, and only needs to be built for the host architecture, not to WASM. The thing that is important here is that the token stream that is output by the proc macro needs to be supported by WASM. All I need are rust type definitions and a way to deserialize a binary blob into those types, and that's the only thing that the ABI gen should output for WASM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense, thanks for completing the test harness example. It works for me on ubuntu, however it appears that it doesn't actually run in CI. Maybe we need a separate job to run cargo test with the appropriate target instead of using a workspace level cargo test? https://github.com/FuelLabs/fuels-rs/runs/6729727596?check_suite_focus=true#step:8:358
As far as my mac issue, I used brew llvm and had the wasm target working, but it got stuck on secp256k:
As far as the proc macro/no-std issue, that is reassuring. It's surprising to me that cargo loads proc-macros from normal deps instead of build deps, considering they're only run at build time. I wonder if that's because types from the macro deps that get used in the resultant token stream need to be transitively imported for hygiene purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the
.cargo/config.toml
would've handled that? I guess not though. I'll put in a separate command for it.For mac are you env vars pointed at the right binary? I had to do this:
Though that issue looks like it's building secp256k to wasm? I don't think we'll ever have a need for making signatures in wasm, so maybe it should be an optional dep of fuels-core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, these exports fixed the mac issue for me!
Having secp256k be optional in fuels-core would make sense since fuels-rs only uses secp256k for signing. If someone was writing fancy indexer logic involving some cryptography, they could source their own no-std libs for that. Looks like we'd just need to disable default features for
fuel-tx
when importing it withinfuels-core
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I put in a CI step to check WASM. I looked at making fuel-tx optional, but fuels-core would not compile with --no-default-features. Mainly because of the Receipt type, which is something we do need in the indexers, so maybe we should keep it as-is for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah maybe file an issue to investigate this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, filed it here: #371