-
Notifications
You must be signed in to change notification settings - Fork 22
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
WEB3-67: feat: switch Steel to alloy #183
Conversation
// we cannot clone the database, so it gets moved in and out of the task | ||
let db = self.env.db.take().unwrap(); | ||
|
||
let (res, db) = tokio::task::spawn_blocking(move || { |
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 it would be preferable, if Steel as a library relies on tokio
. Would be nice if this could remain runtime agnostic.
However, we have to call a long running blocking function inside of an async library, I don't think there is a way around this.
}; | ||
|
||
EvmEnv::from_provider(provider, block_number) | ||
pub async fn from_rpc(url: Url, number: BlockNumberOrTag) -> anyhow::Result<Self> { |
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.
Should be rather switch this to a block hash or tag?
Could there be situations in which the block number is not well defined, maybe due to re-orgs?
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's possible. One API solution to this is to use a builder pattern and split the method that adds the RPC URL from the method that specifies the block ref. In this case, we could provide two methods, one for BlockNumberOrTag
and one for block hash (or something like that).
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.
Nice work! I added some comments. None of them are concerns about the core of this PR.
ethers-core = "2.0" | ||
ethers-providers = "2.0" |
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.
🎉
let provider = ProviderBuilder::new() | ||
.with_recommended_fillers() | ||
.wallet(wallet) | ||
.on_http(args.rpc_url); |
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.
Another option is to use on_builtin
to support HTTPS and WS URLs without code changes. https://docs.rs/alloy-provider/latest/alloy_provider/struct.ProviderBuilder.html#method.on_builtin
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.
Very interesting, I didn't even know that existed. It even takes a string similar to the eth provider before...
🤔 I kinda liked the change to Url for its additional type safety and I guess HTTP is the more realistic provider as we don't rely on any pubsub and even then we have the from_provider
for any alloy provider...
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.
On the question of using Url
, I also like that from a semantics point of view. With on_builtin
, we could call url.as_str
to pass it in, so we can keep that change either way. On the use of HTTP provider. I do see that we constrain the types to HTTP providers anyway. Something we should think about before marking 1.0 is whether we should/can loosen thew type constraints there. It's really not a major issue in any case.
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 types of EthEvmEnv::from_rpc
are only Http
because this is what .on_http
returns. There is also
risc0-ethereum/steel/src/host/mod.rs
Line 62 in eded136
pub async fn from_provider(provider: P, number: BlockNumberOrTag) -> anyhow::Result<Self> { |
which works with any generic alloy provider.
}; | ||
|
||
EvmEnv::from_provider(provider, block_number) | ||
pub async fn from_rpc(url: Url, number: BlockNumberOrTag) -> anyhow::Result<Self> { |
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's possible. One API solution to this is to use a builder pattern and split the method that adds the RPC URL from the method that specifies the block ref. In this case, we could provide two methods, one for BlockNumberOrTag
and one for block hash (or something like that).
state_trie: MerkleTrie, | ||
storage_tries: impl IntoIterator<Item = MerkleTrie>, | ||
contracts: impl IntoIterator<Item = Bytes>, | ||
block_hashes: HashMap<u64, B256>, |
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.
Should this also be impl IntoIterator
or is there a reason not to (e.g. maybe it would waste a lot of time constructing the map)?
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 HashMap is actually already constructed during validation in the into_env
step. So I think it makes sense to just move it here.
⚡️ Features
ethers
dependency completely withalloy
.host
functionsasync
.EvmEnv
from anyalloy
provider.🛠 Fixes
EvmEnv
.eth_getTransactionCount
andeth_getBalance
instead ofeth_getProof
to query basic account information.Anvil
.🚨 Breaking Changes
EthEvmEnv::from_rpc
now accepts aUrl
instead of a&str
for the HTTP RPC endpoint.EvmEnv::from_provider
now requires analloy
provider, and the block number parameter has been changed to aBlockNumberOrTag
.EvmEnv::sol_commitment
has been replaced withEvmEnv::commitment
(to get a reference), orEvmEnv::into_commitment
(to consume and return the commitment).ETH_SEPOLIA_CHAIN_SPEC
andETH_MAINNET_CHAIN_SPEC
have been moved to theethereum
module.CachedProvider
has been removed completely. As alternatives, you can:anvil --fork-url https://ethereum-rpc.publicnode.com@20475759
to create a cached fork for block20475759
.async
instead of blocking: