fix(cast): Always use from field of getTransaction rpc response in cast run #10795
Conversation
ckoopmann
requested review from
DaniPopes,
klkvr,
mattsse,
grandizzy,
yash-atreya and
zerosnacks
as code owners
ckoopmann
force-pushed
the
cast-run-optionaly-bypass-sender-recovery
branch
from
cddd4da to
a66953c
Compare
|
Note: To fully e2e test this feature you'd have to:
I did this locally but wasn't sure if / how to add this to the CI. Lmk if this is critical (or you have a simpler idea of how to test this) and I will look into it further. |
ckoopmann
force-pushed
the
cast-run-optionaly-bypass-sender-recovery
branch
from
a66953c to
b72ba35
Compare
|
Furthermore we might want to add a warning when this flag is not set and the recovered from address differs from the one returned by the |
crates/cast/src/cmd/run.rs
Outdated
| @@ -261,7 +270,11 @@ impl RunArgs { | |||
| let result = { | |||
| executor.set_trace_printer(self.trace_printer); | |||
|
|
|||
| configure_tx_env(&mut env.as_env_mut(), &tx.inner); | |||
| configure_tx_env_assume_impersonation( | |||
There was a problem hiding this comment.
Note that naming is not entirely consistent here. Elsewhere i call it "bypass sender recovery" whereas here i call it "assume impersonation", happy to change it to either one, or any other naming that improves readability / clairity.
crates/evm/core/src/utils.rs
Outdated
| pub fn configure_tx_env(env: &mut EnvMut<'_>, tx: &Transaction<AnyTxEnvelope>) { | ||
| let impersonated_from = is_impersonated_tx(&tx.inner).then_some(tx.from()); | ||
| configure_tx_env_inner(env, &tx.clone(), impersonated_from); |
There was a problem hiding this comment.
after reviewing this again, I tink we should actually just always use the rpc response from value here?
There was a problem hiding this comment.
I agree.
I think when running cast run at least it is fine to trust the "getTransaction" response coming back from the RPC.
I only added the additional flag to keep the default behaviour backwards compatible assuming that there are some users / use cases that depend on the transaction signature being "verified" by recovering the sender from it. Otherwise why was it done this way in the first place ?
Anyway, I will adjust this PR to remove the additional flag and just always use the rpc response as suggested.
There was a problem hiding this comment.
Adjusted to always use the rpc "from" field in cast run: 5d0c3e6
…bility with hardhat impersonated transactions
ckoopmann
force-pushed
the
cast-run-optionaly-bypass-sender-recovery
branch
from
b72ba35 to
eba043d
Compare
| if let AnyTxEnvelope::Ethereum(tx) = &tx.inner.inner() { | ||
| configure_tx_req_env(env, &tx.clone().into(), impersonated_from).expect("cannot fail"); | ||
| configure_tx_req_env(env, &tx.clone().into(), Some(from)).expect("cannot fail"); |
There was a problem hiding this comment.
Note: I didn't change the configure_tx_req_env function signature or implementation to avoid having to change logic in other places where it is used such as:
I could easily change the argument name from impersonated_from to something like overwrite_from though to indicate that this value is not only used for impersonated tx's anymore.
|
Clippy failures seem unrelated to my changes so will just ignore them for now. |
ckoopmann
changed the title
Motivation
Currently the code assumes that any impersonated transaction has been signed with a given constant key. (r=1, s=1). If this check is not passed then the
fromaddress will be recovered from the tx signature.This makes
cast runincompatible with impersonated transactions on rpc implementations where that assumption does not hold (i.e. when debugging tests in legacy hardhat projects).Solution
Add abypass-sender-recoveryflag tocast runthat makes it such that thefromaddress is always set to the corresponding field on thegetTransactionresponse from the rpc. (and not recovered from the tx signature)Always use the from address as returned by the rpc in the
getTransactionresponse.PR Checklist