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: impl SignableTransaction for OpTypedTransaction #318

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ftupas
Copy link

@ftupas ftupas commented Nov 29, 2024

Motivation

Solution

Delegate to inner transaction methods except for TxDeposit as it doesn't implement SignableTransaction

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense, smol nit

{
match self {
Self::Deposit(tx) => {
Signed::new_unchecked(Self::Deposit(tx), TxDeposit::signature(), B256::ZERO)
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 use the deposit tx's from field here

@ftupas ftupas requested a review from mattsse December 1, 2024 16:19
@@ -307,6 +336,61 @@ impl Transaction for OpTypedTransaction {
}
}

impl SignableTransaction<Signature> for OpTypedTransaction {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, we don't have this impl for typedtx on alloy,

need to figure out if we actually want this

Self: Sized,
{
match self {
Self::Deposit(tx) => Signed::new_unchecked(
Copy link

Choose a reason for hiding this comment

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

There is no concept of a "signed deposit tx", a deposit tx is authorized to exist by a log emitted from an L1 contract

Copy link
Member

@mattsse mattsse Dec 3, 2024

Choose a reason for hiding this comment

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

yeah,
given that we also don't this trait implemented for the ethereum typed enum and only for the actual raw transactions, this feels a bit odd, because a signed<TxDeposit> isn't useful.

can't think of a usecase where we'd need TypedTx -> Signed,
I assume the proper way to do this would be to convert it back to the TxRequest and then "build"/sign a valid tx type

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.

[Feature] Impl alloy_consensus::SignableTransaction for OpTypedTransaction
3 participants