-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: evm contract calls #35
feat: evm contract calls #35
Conversation
); | ||
|
||
if !is_last { | ||
if let Some(Some(parent)) = call_stack.borrow_mut().last_mut() { |
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.
where is this handled now? Glad to see it gone from here just wondering
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's what i tried explaining in the PR description, we literally got rid of it in favor of returndatacopy + returndatasize:
allowed contract calls by manipulating revm's frame (call) stack by capturing the return data of the chill frame and manually copying it in the memory region specified in the parent frame.
despite this method worked, it required a previous knowledge of the return size (which was hardcoded at 32bytes for the PoC).after asking Dani, he mentioned that rather than figuring out the size of the return data i should use the RETURNDATASIZE and RETURNDATACOPY opcodes.
his suggestion made me realize that my initial implementation was a sort of embedding the RETURNDATACOPY in r55's execution env, so i got rid of the frame stack manipulation, and instead implemented contract calls as he suggested (which is way cleaner and fully evm-compliant).
eth-riscv-runtime/src/call.rs
Outdated
Some(size) => size, | ||
None => return_data_size(), | ||
}; | ||
if ret_size == 0 { return Some(Bytes::default())}; |
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.
Do we have fmt
running in CI? I feel like it would add line breaks 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.
i thought there was a cargo fmt
check in the CI, but maybe it is quite lax? idk
|
||
// Helper function to convert rust types to their solidity equivalent | ||
// TODO: make sure that the impl is robust, so far only tested with "simple types" | ||
fn rust_type_to_sol_type(ty: &Type) -> Result<DynSolType, &'static str> { |
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 feel like there should be something in Alloy that does this or actually the entire conversion from function signature to selector? cc @gakonst
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.
@DaniPopes not sure if we have this in sol somewhere but maybe this is useful somehow
https://docs.rs/alloy-sol-types/latest/alloy_sol_types/trait.SolType.html
this PR tackles 2 main areas:
Revamp previous implementation of contract calls
allowed contract calls by manipulating revm's frame (call) stack by capturing the return data of the chill frame and manually copying it in the memory region specified in the parent frame.
despite this method worked, it required a previous knowledge of the return size (which was hardcoded at 32bytes for the PoC).
after asking Dani, he mentioned that rather than figuring out the size of the return data i should use the
RETURNDATASIZE
andRETURNDATACOPY
opcodes.his suggestion made me realize that my initial implementation was a sort of embedding the
RETURNDATACOPY
in r55's execution env, so i got rid of the frame stack manipulation, and instead implemented contract calls as he suggested (which is way cleaner and fully evm-compliant).This change modifies:
eth-riscv-syscalls/src/lib.rs
: introduces the 2 new opcodeseth-riscv-runtime/src/call.rs
: assembles contract calls asCALL
+RETURNDATASIZE
+RETURNDATACOPY
r55/src/exec.rs
: uses revm's interpreter return data buffer (which is available across frames), rather than having to modify the frame stackAdd support for calls with different compilation targets
with the previous changes, i was confident that the call logic was sound and evm compatible. However, the remaining challenge was that R55 and EVM use different function selectors, so i enhanced the
interface
macro to handle both compilation targets.contracts will still auto-generate interfaces, and users can still create interfaces for r55 contracts like before:
however, now they can also inform an "interface compilation target" (either
r55
orevm
) and they can rename the function names to camel case:This change modifies:
contract-derive/src/helpers.rs
: to handle the new macro args, and generate the corresponding selector.erc20x-standalone/
: deprecated in favor of a new testing crate for evm contract call support.This change introduces:
evm-caller/
: a new crate, with a r55 contract that is set to call a simple evm contract and which is used for testing the new featurer55/tests/evm-contract-call.rs
: tests for r55 > evm calls, as well as r55 > evm > r55 calls