Conversation
8a81f02 to
b87db19
Compare
|
I saw some ci issues come from my evmc_host_context workaround as below.
-struct evmc_host_context {};
+struct evmc_host_context
+{
+};
I think I need some suggestions for pass these tests. Thanks. |
| pub type CallKind = ffi::evmc_call_kind; | ||
| pub type Revision = ffi::evmc_revision; | ||
| pub type StatusCode = ffi::evmc_status_code; | ||
| pub type Capabilities = ffi::evmc_capabilities; |
There was a problem hiding this comment.
Why not use evmc-vm::types instead of duplicating? We could later refactor to pull them out somewhere.
There was a problem hiding this comment.
Yes, there is needless. I don't notice main line is already added alias types after I fork it.
Fixed in f726610
| host_interface: ::std::option::Option<Box<dyn HostInterface>>, | ||
| } | ||
|
|
||
| static mut CALLBACK: Callback = Callback { |
There was a problem hiding this comment.
This makes this not thread-safe. Why did you choose to use this and not use evmc_host_context, which by design is there to avoid having these globals.
There was a problem hiding this comment.
Yes, you are right. The original implementation was limited by how much I familiar with Rust. I have rewritten it more like go-binding implementation in 5b2cc1a.
| value: &Bytes32, | ||
| code: &Bytes, | ||
| create2_salt: &Bytes32, | ||
| ) -> (&Bytes, i64, StatusCode) { |
There was a problem hiding this comment.
It would be nice to use evmc-vm::ExecutionResult here.
There was a problem hiding this comment.
The struct is incompatible with my idea. I try to hide ffi layer's nest struct to user make it more user friendly.
ex. in bindings.rs
pub struct evmc_address {
#[doc = " The 20 bytes of the hash."]
pub bytes: [u8; 20usize],
}evmc-vm expose those nest structures directly like Address, Bytes32 ...etc and ExecutionResult include one Address field.
But I usually prefer use the simply types that re-defined in emc-client as below.
pub const ADDRESS_LENGTH: usize = 20;
pub const BYTES32_LENGTH: usize = 32;
pub type Address = [u8; ADDRESS_LENGTH];
pub type Bytes32 = [u8; BYTES32_LENGTH];I need confirm which way is better.
There was a problem hiding this comment.
Sorry for the delay, but evmc-vm::ExecutionResult is not ffi, it has high-level types.
There was a problem hiding this comment.
Also regarding the type aliases, the goal was to eventually replace them with nicer types, but to do it incrementally.
| EvmcLoaderInvalidOptionValue = 7, | ||
| } | ||
|
|
||
| pub fn evmc_load_and_create(fname: &str) -> (*mut ffi::evmc_vm, EvmcLoaderErrorCode) { |
There was a problem hiding this comment.
Why did you chose to reimplement this in Rust and not just use the C loader which already exists?
There was a problem hiding this comment.
I reimplement it is because I use this function (loader) outside evmc module at first. I want to avoid move partial evmc's C code to my codebase. But I agree it's better and more consistent to use the C loader after I move my evmc relative code into evmc-client now. I will try to replace it use the C loader, but maybe this won't finish too soon.
| use std::ffi::CStr; | ||
|
|
||
| extern "C" { | ||
| fn evmc_create() -> *mut ffi::evmc_vm; |
There was a problem hiding this comment.
I think as a first step it would be better to only support dynamic loading.
Are you using this static linking in SSVM?
There was a problem hiding this comment.
I am not sure this is relative with static linking. I still use dynamic link to lib.so and the statement just tell rustc how to resolve symbol after cargo link the library.
https://github.com/second-state/rust-ssvm/blob/master/build.rs#L25
This is useful in my case that I don't need to know where is my evmc dynamic-link library (especially the target path is not always the same).
There was a problem hiding this comment.
If it does an extern import of evmc_create that is statically linking against an EVMC VM, IIRC, otherwise this would be a lingering reference.
5e150ea to
71d3f22
Compare
Turn evmc_host_context instantiable.
1. remove duplicate types 2. add an alias type Capabilities in evmc-vm
Refine this module make it more like geth client binding implementation.
71d3f22 to
3f9eb21
Compare
3f9eb21 to
5b2b4f2
Compare
1. A global static variable requires static lifetime. 2. Box with a trait object requires static lifetime. Avoid host context contain a outside variable with conflict lifetime issue. # Conflicts: # bindings/rust/evmc-client/src/host.rs # bindings/rust/evmc-client/src/lib.rs
| ) -> ffi::evmc_result { | ||
| let msg = *msg; | ||
| let (output, gas_left, create_address, status_code) = | ||
| (*(context as *mut ExtendedContext)).hctx.call( |
There was a problem hiding this comment.
This entire function could be reduced to hctx.call().into() is using ExecutionResult from evmc-vm/lib.rs because that implements the translation and memory handling already.
Part of #476.
f8466c7 is a temporary workaround for evmc_host_context cannot be instantiated.cherry-pick b0c40df from rust: turn evmc_host_context into a void type #524I am not yet add any unit test for this module
but it work fine with Hera.https://github.com/second-state/rust-ssvm/tree/upgrade-evmc-clientUse this branch follow original example section.(I add hera as submodule, replace build.rs to build and link libhera.so and update dummy host function in example)but it already applied on https://github.com/second-state/rust-ssvm/tree/evmc-7 could run example with SSVM.