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

Replace RecoveredTx with alloy's RecoveredTx type #13651

Closed
mattsse opened this issue Jan 4, 2025 · 6 comments
Closed

Replace RecoveredTx with alloy's RecoveredTx type #13651

mattsse opened this issue Jan 4, 2025 · 6 comments
Assignees
Labels
A-sdk Related to reth's use as a library C-enhancement New feature or request D-complex Quite challenging from either a design or technical perspective Ask for help! D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Jan 4, 2025

Describe the feature

#13650 unblocks us from phasing out:

pub struct RecoveredTx<T = TransactionSigned> {

which is identical to:

https://github.com/alloy-rs/alloy/blob/fa109e3a41bc84ca0721a218003ff3c45dcbc36e/crates/consensus/src/transaction/recovered.rs#L8-L8

there could be a few additional unexpected blockers, the recommended strategy for this pr would be:

  1. rename reth_primitives::RecoveredTx functions so that they match alloy::Recovered's (this should be an initial separate PR first)
  2. re-export alloy::Recovered as RecoveredTx from reth-primitives-traits
  3. remove extension trait and integrate these functions in SignedTransaction directly
    pub trait SignedTransactionIntoRecoveredExt: SignedTransaction {
  4. uncomment all reth_primitives::RecoveredTx code and replace with a re-export pub use reth_primitives_traits::RecoveredTx
  5. see what breaks

possible that we need alloy-rs/alloy#1885 first

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Jan 4, 2025
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started D-complex Quite challenging from either a design or technical perspective Ask for help! A-sdk Related to reth's use as a library and removed S-needs-triage This issue needs to be labelled labels Jan 4, 2025
@bendanzhentan
Copy link
Contributor

Could you please assign this issue to me?

@mattsse
Copy link
Collaborator Author

mattsse commented Jan 4, 2025

assigned, ty.

strongly recommend to start with step 0. first to keep overall changes manageable

@V15HNUPM
Copy link

V15HNUPM commented Jan 6, 2025

I’d like to resolve this.

@bendanzhentan
Copy link
Contributor

Progress:

#13663 does the first step,
#13670 removes reth RecoveredTx and re-exports alloy Recovered.

Very appreciate for your kindly help @mattsse
But there is a remaining sub-task that I have failed to complete. Hope someone can resolve it. Thanks :

remove extension trait and integrate these functions in SignedTransaction directly

pub trait SignedTransactionIntoRecoveredExt: SignedTransaction {

maybe cc @V15HNUPM @mattsse

@mattsse
Copy link
Collaborator Author

mattsse commented Jan 7, 2025

remove extension trait and integrate these functions in SignedTransaction directly

this has also been moved already #13677

marking this issue as closed

@mattsse mattsse closed this as completed Jan 7, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Jan 7, 2025
@V15HNUPM
Copy link

V15HNUPM commented Jan 7, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sdk Related to reth's use as a library C-enhancement New feature or request D-complex Quite challenging from either a design or technical perspective Ask for help! D-good-first-issue Nice and easy! A great choice to get started
Projects
Status: Done
Development

No branches or pull requests

3 participants