Skip to content
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

EVM: Move CALLACTOR into a precompile #861

Merged
merged 12 commits into from
Nov 22, 2022
Merged

Conversation

mriise
Copy link
Contributor

@mriise mriise commented Nov 18, 2022

actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
@mriise mriise marked this pull request as ready for review November 19, 2022 01:06
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/instructions/call.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
Comment on lines 334 to 338
let address_offset = read_u64(input_params.next().unwrap())? as usize;
let send_data_offset = read_u64(input_params.next().unwrap())? as usize;

let address_size = read_u64(input_params.next().unwrap())? as usize;
let send_data_size = read_u64(input_params.next().unwrap())? as usize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, well after talking we are going to go with just specifying the lengths for now, if solidity is having a particularly hard time decoding these then we can add them defined exactly with solidity a ABI, but this could also be implemented easily enough in solidity itself with an SDK

actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore the nits and do that later. But we should make the changes in the actual callactor precompile.

actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
}

pub fn exhausted(&self) -> bool {
self.exhausted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function or field?

// will be nicer with https://github.com/rust-lang/rust/issues/74985
/// Wrapper around `ChunksExact` that pads instead of overflowing.
/// Also provides a nice API interface for reading Parameters from input
struct PaddedChunks<'a, T: Sized + Copy, const CHUNK_SIZE: usize> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice.

let mut input_params = U256Reader::new(input);

let len = input_params.next_param_padded::<u32>()? as usize;
let addr = match Address::from_bytes(&read_right_pad(input_params.remaining_slice(), len)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like reading then padding is common. We might want to provide a method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

later if we need it, this PR already has a lot of changes

}
}

struct Parameter<T>(pub T);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A trait would fit better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally yeah, but it helps a lot to avoid conflicting implementations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'm just recommending defining your own trait rather than combining a newtype with an existing trait. But we can handle that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that would work

actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
actors/evm/src/interpreter/uints.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #861 (f26efee) into next (cd7cfce) will decrease coverage by 0.23%.
The diff coverage is 56.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #861      +/-   ##
==========================================
- Coverage   87.32%   87.09%   -0.24%     
==========================================
  Files         125      125              
  Lines       22784    22953     +169     
==========================================
+ Hits        19897    19991      +94     
- Misses       2887     2962      +75     
Impacted Files Coverage Δ
actors/evm/src/interpreter/uints.rs 74.74% <0.00%> (-1.94%) ⬇️
actors/evm/src/interpreter/precompiles.rs 80.07% <56.06%> (-7.49%) ⬇️
actors/evm/src/interpreter/instructions/call.rs 91.66% <100.00%> (+0.31%) ⬆️
actors/init/src/testing.rs 75.00% <0.00%> (-1.79%) ⬇️
...ors/evm/src/interpreter/instructions/arithmetic.rs 73.28% <0.00%> (-0.69%) ⬇️
state/src/check.rs 86.24% <0.00%> (-0.27%) ⬇️
actors/power/src/lib.rs 83.40% <0.00%> (-0.22%) ⬇️
actors/evm/src/interpreter/system.rs 89.34% <0.00%> (+0.59%) ⬆️
actors/reward/src/lib.rs 83.73% <0.00%> (+0.81%) ⬆️
... and 2 more

@mriise mriise mentioned this pull request Nov 22, 2022
2 tasks
actors/evm/src/interpreter/precompiles.rs Outdated Show resolved Hide resolved
@Stebalien Stebalien enabled auto-merge (squash) November 22, 2022 23:37
@Stebalien Stebalien merged commit 31acd91 into next Nov 22, 2022
@Stebalien Stebalien deleted the feat/callactor-precompile branch November 22, 2022 23:58
@mriise
Copy link
Contributor Author

mriise commented Nov 28, 2022

note: the TODO for static call should be fixed after filecoin-project/ref-fvm#1150 merges

@maciejwitowski
Copy link
Contributor

@mriise @Stebalien filecoin-project/ref-fvm#1150 has been merged. Should we continue with the TODO that you mentioned above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants