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

feat(deps): Move to Alloy ABI encoding/decoding & alloy types #5986

Merged
merged 83 commits into from
Oct 25, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Oct 5, 2023

Ports of most of Foundry to use all the crates alloy/core, replacing common types, and ABI encoding/decoding.

Behavior should be mostly the same as our test suite is able to catch these, albeit manual testing was done with larger repos as well (Sparklend/Maple) with positive results.

It also removes a big portion of glue code from the codebase. The glue that remains is:

ethers::etherscan and ethers::solc have also been removed in favor of foundry-block-explorers and foundry-compilers, which means we now have full ownership of these and do not depend on ethers anymore for this functionality.

Our dependency on ethers now consists of only:

  • RPC Types
  • Providers
  • Misc. wallet management utilities and libraries contained (aws, trezor, ledger, etc).

Planned followups are:

  • Finish cheatcode refactor to use static encoder
  • Move main executor to use SolCall.

@Evalir Evalir changed the title [WIP] feat: introduce foundry-compilers [WIP] feat: foundry-compilers & alloy_json_abi EVM migration Oct 5, 2023
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Good start

@@ -144,25 +140,25 @@ impl CallTraceDecoder {
// TODO: These are the Ethereum precompiles. We should add a way to support precompiles
// for other networks, too.
precompiles: precompiles!(
0x01: ecrecover(hash: FixedBytes(32), v: Uint(256), r: Uint(256), s: Uint(256)) -> (publicAddress: Address),
Copy link
Member

Choose a reason for hiding this comment

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

This change looks a bit odd cc @DaniPopes

Copy link
Member

Choose a reason for hiding this comment

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

The whole point of this macro is to make it easy to modify. Slapping format everywhere doesn't really help towards that. Anyway I am working on removing this in #5998 since some precompiles don't adhere to ABI so it doesn't matter.

core::utils::to_checksum,
types::{Bytes, DefaultFrame, GethDebugTracingOptions, StructLog, H256, U256},
};
use ethers::types::{DefaultFrame, GethDebugTracingOptions, StructLog};
Copy link
Member

Choose a reason for hiding this comment

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

These types should also go to alloy? I guess RPC crate under core/?

Think good opportunity to get that started, along with the TxRequest/Response/Receipt?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, we should move these to alloy somehow. Probably reth might have these somewhat reworked

crates/evm/src/trace/mod.rs Show resolved Hide resolved
crates/evm/src/trace/node.rs Show resolved Hide resolved
crates/evm/src/fuzz/strategies/int.rs Outdated Show resolved Hide resolved
crates/evm/src/fuzz/strategies/param.rs Show resolved Hide resolved
crates/evm/src/fuzz/strategies/state.rs Outdated Show resolved Hide resolved
crates/evm/src/fuzz/strategies/uint.rs Outdated Show resolved Hide resolved
crates/evm/src/trace/node.rs Show resolved Hide resolved
@@ -144,25 +140,25 @@ impl CallTraceDecoder {
// TODO: These are the Ethereum precompiles. We should add a way to support precompiles
// for other networks, too.
precompiles: precompiles!(
0x01: ecrecover(hash: FixedBytes(32), v: Uint(256), r: Uint(256), s: Uint(256)) -> (publicAddress: Address),
Copy link
Member

Choose a reason for hiding this comment

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

The whole point of this macro is to make it easy to modify. Slapping format everywhere doesn't really help towards that. Anyway I am working on removing this in #5998 since some precompiles don't adhere to ABI so it doesn't matter.

crates/evm/src/fuzz/strategies/param.rs Outdated Show resolved Hide resolved
crates/evm/src/fuzz/strategies/param.rs Outdated Show resolved Hide resolved
crates/abi/src/bindings/alloy_console.rs Outdated Show resolved Hide resolved
crates/abi/src/bindings/mod.rs Outdated Show resolved Hide resolved
@Evalir Evalir force-pushed the evalir/introduce-foundry-compilers branch from 691e3d5 to 65f0918 Compare October 17, 2023 10:26
crates/evm/src/decode.rs Outdated Show resolved Hide resolved
@Evalir Evalir force-pushed the evalir/introduce-foundry-compilers branch from f4738e1 to fb7bd39 Compare October 24, 2023 13:33
@Evalir Evalir changed the title feat(deps): Move to Alloy ABI encoding/decoding feat(deps): Move to Alloy ABI encoding/decoding & alloy types Oct 24, 2023
crates/cast/bin/cmd/storage.rs Outdated Show resolved Hide resolved
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

I'm generally supportive of moving forward with this PR but want to point out a few things:

  • There's some code that was vendored over from ethers-rs to keep things going, that's fine but I would like to keep track of that code and what the plan is for it (e.g. Numeric type, unit conversions)
  • There's a few APIs that seem to have gotten worse not better, e.g. the DynSolValue decoding stuff, or the req for the user to pass validate=false to the decoder
  • I really dislike the U256::from stuff and think there must be a From impl that will work with us, please investigate what the path for including From for U256 is in ruint.

Basically, I like that we're moving forward, but I think in some places we need to look at the code and say "hey actually this became worse and it looks like there's something missing from Alloy, let me fix that first".

Comment on lines +195 to +212
ethers = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-addressbook = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-core = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-contract = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-contract-abigen = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-providers = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-signers = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-middleware = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-etherscan = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }
ethers-solc = { git = "https://github.com/gakonst/ethers-rs", rev = "9754f22d06a2defe5608c4c9b809cc361443a3dc" }

foundry-compilers = { git = "https://github.com/foundry-rs/compilers" }
foundry-block-explorers = { git = "https://github.com/foundry-rs/block-explorers"}

alloy-dyn-abi = { git = "https://github.com/alloy-rs/core/" }
alloy-primitives = { git = "https://github.com/alloy-rs/core/" }
alloy-json-abi = { git = "https://github.com/alloy-rs/core/" }
alloy-sol-types = { git = "https://github.com/alloy-rs/core/" }
Copy link
Member

Choose a reason for hiding this comment

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

what's the path towards removing these patches? why do we need them?

Copy link
Member Author

@Evalir Evalir Oct 25, 2023

Choose a reason for hiding this comment

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

The main offender is foundry-block-explorers needing the chain enum from ethers. Once we have alloy-rs/chains we can publish both block explorers and compilers and remove the patches. For alloy, we need a newer release with all the new features we're using off of master.

salt.clone(),
init_code_hash,
));
let addr = SimpleCast::to_checksum_address(&deployer.create2(salt, init_code_hash));
Copy link
Member

Choose a reason for hiding this comment

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

Agree supportive of combining the two if the functions are simple enough.

Comment on lines 29 to 35
value_parser = NameOrAddress::from_str,
value_parser = Address::from_str,
default_value = "0x0000000000000000000000000000000000000000",
env = "ETH_FROM",
)]
from: NameOrAddress,
from: Address,
Copy link
Member

Choose a reason for hiding this comment

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

this should also remain nameoraddress -- maybe i didn't communicate this appropriately, my thought was that we pull in NameOrAddress from ethers-rs, and in the CLI you have a helper method that does the resolution for you, where you'd convert the ethers-rs returned Address from the ENS string to an Alloy Address

@@ -208,6 +211,78 @@ fn build_filter_topics(topics: Vec<String>) -> Result<TopicFilter, eyre::Error>
})
}

fn parse_params<'a, I: IntoIterator<Item = (&'a ParamType, &'a str)>>(
Copy link
Member

Choose a reason for hiding this comment

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

Should this whole function be part of alloy?

0x07: ecmul(x1: Uint(256), y1: Uint(256), s: Uint(256)) -> (x: Uint(256), y: Uint(256)),
0x08: ecpairing(x1: Uint(256), y1: Uint(256), x2: Uint(256), y2: Uint(256), x3: Uint(256), y3: Uint(256)) -> (success: Uint(256)),
0x09: blake2f(rounds: Uint(4), h: FixedBytes(64), m: FixedBytes(128), t: FixedBytes(16), f: FixedBytes(1)) -> (h: FixedBytes(64)),
0x01: ecrecover(hash: format!("bytes32"), v: format!("uint256"), r: format!("uint256"), s: format!("uint256")) -> (publicAddress: format!("address")),
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 very ugly and needs to be removed -- what's the plan for this? sol macro? let's pls track this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking on #6102

crates/evm/src/decode.rs Outdated Show resolved Hide resolved
crates/evm/src/decode.rs Outdated Show resolved Hide resolved
crates/config/src/utils.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
Comment on lines -137 to +126
U256::from_str(value)?
U256::from_str_radix(value, 16)?
} else {
U256::from(LenientTokenizer::tokenize_uint(value)?)
alloy_dyn_abi::DynSolType::coerce_str(&alloy_dyn_abi::DynSolType::Uint(256), value)?
.as_uint()
.wrap_err("Could not parse ether value from string")?
.0
Copy link
Member

Choose a reason for hiding this comment

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

this got more complex now, how do we simplify it back?

Copy link
Member Author

Choose a reason for hiding this comment

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

A move to SolCall should simplify this—will be done on a followup

@Evalir
Copy link
Member Author

Evalir commented Oct 25, 2023

Merging—CI is flaky due to arbitrum rpc limiting (happening on other PRs as well)

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.

4 participants