Refactor sp-sandbox; make sure both sandbox executors are always tested
#10173
Conversation
koute
added
A0-please_review
| } | ||
|
|
||
| impl<T> EnvironmentDefinitionBuilder<T> { | ||
| pub trait SandboxEnvironmentBuilder<State, Memory>: Sized { |
There was a problem hiding this comment.
Loving not using <T>. These generic names are great.
There was a problem hiding this comment.
generic names
Isn't that a contradiction in terms?
I mean a 'generic' name is by definition meaningless and that is why I prefer symbols instead of 'names' for generic type parameters. For me, the semantic of a generic type parameter is not induced by it's name but by the respective type bounds.
There was a problem hiding this comment.
Loving it. This include_file! stuff always bothered me.
've reexported the executor which was previously selected at compile time as sp_sandbox::default to make sure I don't inadvertently change which executor is used; do we want to get rid of that and explicitly refer to the concrete executor everywhere sp_sandbox::default is currently used? (Or perhaps move the logic of which one should be used to the places it's actually being used?)
For contracts I want be able to switch to wasmer by only recompiling without source file changes. This change would complicate that. Ideally, I want be able to switch between wasmi/wasmer at runtime. This does not work because the host API does not allow selecting the execution engine.
I think for now it should stay the way it is. Eventually we want to overhaul the API exposed by the client. This change should also include selecting the execution engine (wasmtime, wasmer, wasmer singlepass).
|
@bkchr Okay, I've fixed the duplication; looks good now? (: |
Ty ❤️ |
|
bot merge |
…sted (paritytech#10173) * sp-sandbox: convert executors into normal `mod`s instead of using `include!` * sp-sandbox: run `cargo fmt` on `host_executor.rs` * sp-sandbox: abstract away the executors behind traits * sp_sandbox: always compile both executors when possible * sc-executor: make sure all sandbox tests run on both sandbox executors * sc-executor: fix brainfart: actually call into the sandbox through the trait * sc-runtime-test: fix cargo fmt * sc-runtime-test: deduplicate executor-specific sandbox test entrypoints * sc-executor: test each sandbox executor in a separate test * cargo fmt (Github's conflict resolving thingy broke indentation)
…sted (paritytech#10173) * sp-sandbox: convert executors into normal `mod`s instead of using `include!` * sp-sandbox: run `cargo fmt` on `host_executor.rs` * sp-sandbox: abstract away the executors behind traits * sp_sandbox: always compile both executors when possible * sc-executor: make sure all sandbox tests run on both sandbox executors * sc-executor: fix brainfart: actually call into the sandbox through the trait * sc-runtime-test: fix cargo fmt * sc-runtime-test: deduplicate executor-specific sandbox test entrypoints * sc-executor: test each sandbox executor in a separate test * cargo fmt (Github's conflict resolving thingy broke indentation)
…sted (paritytech#10173) * sp-sandbox: convert executors into normal `mod`s instead of using `include!` * sp-sandbox: run `cargo fmt` on `host_executor.rs` * sp-sandbox: abstract away the executors behind traits * sp_sandbox: always compile both executors when possible * sc-executor: make sure all sandbox tests run on both sandbox executors * sc-executor: fix brainfart: actually call into the sandbox through the trait * sc-runtime-test: fix cargo fmt * sc-runtime-test: deduplicate executor-specific sandbox test entrypoints * sc-executor: test each sandbox executor in a separate test * cargo fmt (Github's conflict resolving thingy broke indentation)
This PR refactors
sp-sandboxso that (when possible) both execution backends are always compiled, and makes sure that both of them are actually tested in thesc-executorintegration tests. (Currently only theembedded_executorcodepath is tested; after this PR bothembedded_executorandhost_executorare.)Open questions/possible further work
sp_sandbox::defaultto make sure I don't inadvertently change which executor is used; do we want to get rid of that and explicitly refer to the concrete executor everywheresp_sandbox::defaultis currently used? (Or perhaps move the logic of which one should be used to the places it's actually being used?)