Modularise the wasm module#6441
Modularise the wasm module#6441a1q123456 wants to merge 1 commit intoripple/wasmi-host-functionsfrom
Conversation
Signed-off-by: JCW <a1q123456@users.noreply.github.com>
| #pragma once | ||
|
|
||
| #include <xrpld/app/wasm/HostFunc.h> | ||
| #include <xrpl/tx/transactors/smart_escrow/HostFunc.h> |
There was a problem hiding this comment.
Smart Escrow is just one feature, wheras the WASM VM will power multiple "Smart Features." Because of that, I think may <xrpl/tx/transactors/wasm/HostFunc.h> is the better location?
There was a problem hiding this comment.
Do we want it to be in the transactors module or a separate module entirely?
There was a problem hiding this comment.
My idea was, once we want to use wasm for something else, we take the smart_escrow/wasm folder and make it a top level module like xrpl.wasm and then we implement other things for that feature specifically.
HostFunc is something tightly coupled with smart escrow and I don't think we'll reuse it.
There was a problem hiding this comment.
HostFunc is something tightly coupled with smart escrow and I don't think we'll reuse it.
Actually, HostFunctions are coupled to the broader category of Smart Features (with more coming soon), not just Smart Escrow.
Smart Escrow serves as a proving ground for generalized WASM functionality in xrpld. Future WASM-based features—like Smart Features, Smart Contracts, potentially even Native Transactors—will reuse this layer.
Ultimately, WASM should be viewed as a cross-cutting architectural feature that may even extend beyond transactor processing.
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ WasmEngine (WasmVM.h) │ | ||
| │ EscrowWasm.h │ |
There was a problem hiding this comment.
This is incorrect. The WasmEngine is not specific to Escrow (it's used by Escrow, but will be used by more in the future).
|
|
||
| namespace xrpl { | ||
|
|
||
| // Generic WASM constants |
There was a problem hiding this comment.
This comment explain / clarify nothing, I don't think we need to add obvious comments.
| uint32_t const MAX_PAGES = 128; // 8MB = 64KB*128 | ||
|
|
||
| class WasmiEngine; | ||
| class WasmiRuntime; |
There was a problem hiding this comment.
I know choosing naming is tough process, so it need to be done in separate PR with proper discussion. For example "Runtime" is more suitable for javascript engine, wasm is more docker like.
Renaming it here just complicate reviewing modularization.
| class WasmiRuntime; | ||
|
|
||
| /** | ||
| * WasmEngine - Singleton facade for WASM runtime execution. |
There was a problem hiding this comment.
Same comment about renaming.
| class WasmEngine | ||
| { | ||
| std::unique_ptr<WasmiEngine> const impl; | ||
| std::unique_ptr<WasmiRuntime> const impl; |
There was a problem hiding this comment.
Same comment about renaming.
| * @return Shared pointer to the import vector | ||
| */ | ||
| std::shared_ptr<ImportVec> | ||
| createWasmImport(HostFunctions& hfs); |
There was a problem hiding this comment.
This doesn't belong to "Escrow", it is more general.
| @@ -0,0 +1,65 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Name EscrowWasm.h is more like some ledger object / transaction style. EscrowWasmHelpers.h more suitable
|
|
||
| // std::cout << "runEscrowWasm, mod size: " << wasmCode.size() | ||
| // << ", gasLimit: " << gasLimit << ", funcName: " << funcName; | ||
| #ifdef DEBUG_OUTPUT |
There was a problem hiding this comment.
Please use another definition, as it not always needed during debugging.
| WasmiEngine::allocate(int32_t sz) | ||
| WasmiRuntime::allocate(int32_t sz) | ||
| { | ||
| if (sz <= 0) |
There was a problem hiding this comment.
Why? You shouldn't remove security checks.
| wasm_byte_vec_delete(&msg); | ||
|
|
||
| return trap; | ||
| wasm_message_t message; |
There was a problem hiding this comment.
Same here, why you remove security checks?
| gas = std::numeric_limits<decltype(gas)>::max(); | ||
| WasmResult<int32_t> const ret{res.r.vec_.data[0].of.i32, gas - moduleWrap_->getGas()}; | ||
|
|
||
| // #ifdef DEBUG_OUTPUT |
There was a problem hiding this comment.
Coudl you please just put it into additional #ifdef section and not removing it.
|
@a1q123456 can we focus on just the modularization in this PR, and not make any other changes? If you have other concerns/changes you want to make, we can discuss those separately, in PR review for #6075 |
High Level Overview of Change
This PR modularises the WASM module to make it possible to call wasm from transactors after transactor modularisation.
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)