-
Notifications
You must be signed in to change notification settings - Fork 153
Migrate remaining e2e test utilities to alloy primitives #4001
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Aryan Godara <aryangodara03@gmail.com>
| .execute("anvil_impersonateAccount", vec![json_address]) | ||
| .await?; | ||
|
|
||
| Ok(Account::Local(*address, None)) |
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 side effect of anvil_impersonateAccount still happens
And since this return values wasn't actually being used where this function was called, I changed this to return unit type (also specified in PR description).
I could use some feedback on what should be the equivalent return type in alloyrs for ethcontract::Account, especially if the value isn't being used.
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 there are mostly 2 options:
impersonate()returns an alloy provider that doesn't have any signers (like here)ForkedNodeApihas aimpersonating_provider()that you are supposed to use for any number of different impersonated account.
I think I prefer 2 because it's pretty easy to communicate the correct way to use the API with doc comments.
#[derive(Debug, Clone)]
pub struct ForkedNodeApi {
provider: AlloyProvider,
}
impl ForkedNodeApi {
pub fn new(provider: AlloyProvider) -> Self {
Self { provider: provider.without_wallet() }
}
/// Provider that can sign transactions on behalf of accounts
/// previously impersonated with `[Self::impersonate()]`.
pub fn impersonating_provider(&self) -> &AlloyProvider {
&self.provider
}
/// Configures the node to sign any transaction for the given account.
/// Such transactions should be sent from `[Self::impersonating_provider()]`.
///
/// # Panics
///
/// Panics if the connected RPC does not support impersonating account.
pub async fn impersonate(&self, address: &Address);
// all the other existing functions
}Ultimately both versions are not optimal because you can set tx.from arbitrarily so nothing on the type level stops you from impersonating some account and then using any other impersonation incompatible provider to send transactions for that account.
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.
You can use stop_impersonating_account — https://docs.rs/alloy-provider/latest/alloy_provider/ext/trait.AnvilApi.html#tymethod.anvil_stop_impersonating_account
The reason we need an empty wallet is that if the wallet has at least one signer, it will have a default signer, meaning all TXs will be signed using it. The alternative is to send the TX "manually" as in, outside the regular alloy pipeline that fills in gaps for the TX (like nonce, signature, etc)
Personally, I don't think the current state API is great, a better way of solving this would be having the impersonate taking an async future/closure that executes within a scope of impersonation:
Kinda like:
provider.impersonate(address, |empty_provider| async move {
ERC20::instance(provider)::balanceOf(address)...
}).await
And inside, what really happens is:
self.transport
.execute("anvil_impersonateAccount", vec![json_address])
.await?;
let closure_result = closure.await;
self.transport
.execute("anvil_stopImpersonateAccount", vec![json_address])
.await?;
return closure_result
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.
Or, better yet, remove this whole API and just use what anvil already provides: https://docs.rs/alloy-provider/latest/alloy_provider/ext/trait.AnvilApi.html#tymethod.anvil_send_impersonated_transaction_with_config
Maybe you can make it more ergonomic
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.
Looking into this I opened #4004
Removed the forked node altogether, used alloy/anvil instead, no need to argue about the right return type :P
MartinquaXD
left a comment
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.
Probably makes sense to still address the impersonation topic in this PR.
jmg-duarte
left a comment
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.
LGTM asides the impersonation question, IMO we can either revert the change on the API and merge the rest of the PR; or fix it now
| Ok(()) | ||
| } | ||
|
|
||
| pub fn set_chain_id(&self, chain_id: u64) -> CallFuture<(), T::Out> { |
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 function is unused, IMO we can remove this
| pub fn set_balance(&self, address: &Address, balance: U256) -> CallFuture<(), T::Out> { | ||
| let json_address = serde_json::json!(address); | ||
| let json_balance = serde_json::json!(format!("{:#032x}", balance)); |
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.
Nerd nit: if you expand the macro you'll see that json! simply calls serde_json::to_value (at least for these two cases)
But it also makes the value you pass, a reference, meaning address becomes a double reference (&&Address)
Unlike other parts of the codebase, where address is immediately de-referenced turning it into a copy, here we can simply remove the macro (which usually hides more magic than in this case) use the expanded API and avoid a double reference
For the second call, we could do .as_str() or just the reference even.
I know this is not directly related to the migration, but a migration is the perfect opportunity to improve the codebase like this
| .execute("anvil_impersonateAccount", vec![json_address]) | ||
| .await?; | ||
|
|
||
| Ok(Account::Local(*address, None)) |
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.
Or, better yet, remove this whole API and just use what anvil already provides: https://docs.rs/alloy-provider/latest/alloy_provider/ext/trait.AnvilApi.html#tymethod.anvil_send_impersonated_transaction_with_config
Maybe you can make it more ergonomic
# Description Discussed in this thread: #4001 (comment) This PR moves the impersonation mechanisms to alloy, the migrated ones are only impersonated in the specific transaction, no changes to the actual tests were needed. Exposed the previous string oriented API under a trait to as discussed in a comment below. Anvil provides an anvil specific API that we're not using unless necessary as these ones are more general # Changes - [ ] Migrate the last `mine_pending_block` - [ ] Migrate the other evm calls to the new trait - [ ] Migrate the last impersonations - [ ] Remove the forked_node and local_node modules ## How to test Existing tests
Description
Migrates internal
ethcontracttypes toalloyprimitives within thee2ecrate, reducing reliance on legacy types.Changes
ethcontract::primitiveswithalloy::primitivesAccountreturn was unused; alloy handles impersonation via provider without wallet +.from(address); now returning unit (), since function's return value wasn't being used.into_legacy()/into_alloy()conversionsNote:
ethcontractdependency remains for shared crate compatibility.How to test
cargo check -p e2e --tests