-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add rust binding client #527
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to use evmc-vm::ExecutionResult
here.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, but evmc-vm::ExecutionResult
is not ffi, it has high-level types.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Finish 5b2b4f2, btw I need cmake in ci environment.
use std::ffi::CStr; | ||
|
||
extern "C" { | ||
fn evmc_create() -> *mut ffi::evmc_vm; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.