-
Notifications
You must be signed in to change notification settings - Fork 10
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: add helper trait for deriving TxEnv
from WithEncoded
#42
feat: add helper trait for deriving TxEnv
from WithEncoded
#42
Conversation
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 is already looking good, left some comments
crates/evm/src/tx.rs
Outdated
pub trait FromEncodedTx<Tx> { | ||
/// Builds a `TxEnv` from encoded transaction bytes. | ||
fn from_encoded_tx(encoded: &[u8]) -> 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.
we also need this to have access to transaction itself and its sender, so I believe signature would need to be something like from_encoded_tx(tx, sender, encoded)
crates/evm/src/eth/block.rs
Outdated
@@ -104,12 +106,14 @@ where | |||
|
|||
fn execute_transaction_with_result_closure( | |||
&mut self, | |||
tx: Recovered<&R::Transaction>, | |||
tx: impl IntoTxEnv<TxEnv> + AsRecoveredTx<Self::Transaction>, |
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.
we should now use IntoTxEnv
when converting to tx_env
the assumption here is that IntoTxEnv
will either be the same as .as_recovered_tx().into_tx_env()
or more optimized (like in case with WithEncoded
)
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 tried to use directly ().into_tx_env()
on tx.as_recovered_tx()
after the gas checks,
yet I'm not sure it's possible to use it directly without having a new impl like this
impl<R> FromRecoveredTx<R::Transaction> for TxEnv
where
R: eth::receipt_builder::ReceiptBuilder,
R::Transaction: Transaction,
{
fn from_recovered_tx(tx: &R::Transaction, sender: Address) -> Self {
}
}
Should I add to enable the conversion ?
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.
we don't want to use the into_tx_env
on tx._recovered_tx()
as in case of WithEncoded
we'll lose optimization that WithEncoded
gives us
instead it should be something like
let recovered = tx.as_recovered();
let tx_env = tx.into_tx_env();
or maybe we should even add a separate trait to avoid having to drop value before into_tx_env
call
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!
Motivation
Closes #40.
Solution
FromEncodedTx<Tx>
helper trait to build transaction environments from encoded bytesIntoTxEnv<TxEnv>
forWithEncoded<T>
typesAsRecoveredTx<T>
trait to access recovered transactionsBlockExecutor::execute_transaction
andexecute_transaction_with_result_closure
to accept any type implementing bothIntoTxEnv<TxEnv>
andAsRecoveredTx<Self::Transaction>
PR Checklist