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

chore(sdk): SignedTransaction abstraction #11432

Merged
merged 26 commits into from
Oct 18, 2024
Merged

chore(sdk): SignedTransaction abstraction #11432

merged 26 commits into from
Oct 18, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Oct 2, 2024

Closes #11253

Checks out the abstraction of TransactionSigned from #11348 and #11352

@emhane emhane added C-debt A clean up/refactor of existing code A-sdk Related to reth's use as a library labels Oct 2, 2024
@emhane emhane requested a review from Rjected as a code owner October 2, 2024 13:52
@emhane emhane requested a review from mattsse October 2, 2024 13:56
@emhane emhane force-pushed the emhane/signed-tx-trait branch from a26253c to d28569f Compare October 2, 2024 14:04
@emhane emhane requested a review from mattsse October 3, 2024 14:48
Copy link
Collaborator

@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.

hmm, I really don't want to define new rlp functions on transaction trait now that we almost entirely on De/Encodable2718

could we start with something basic like Transaction: Encodable2718 + Decodeable2718?

Copy link
Collaborator

@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.

we def need this trait, but this should be moved to the traits crate.

and we should try without introducing new rlp functions, we should manage without and instead require that this type is also De/Encodeable2718

@emhane
Copy link
Member Author

emhane commented Oct 5, 2024

we def need this trait, but this should be moved to the traits crate.

and we should try without introducing new rlp functions, we should manage without and instead require that this type is also De/Encodeable2718

pr doesn't introduce any new methods at all. method deprecated in #11218 is removed in commit b820aa8.

@emhane emhane requested a review from joshieDo as a code owner October 5, 2024 16:54
@emhane emhane requested a review from mattsse October 5, 2024 18:25
@emhane emhane force-pushed the emhane/signed-tx-trait branch from cf72724 to 00f899f Compare October 5, 2024 18:33
@emhane
Copy link
Member Author

emhane commented Oct 7, 2024

re-request review @mattsse

Copy link
Collaborator

@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.

I feel like this already does too much.

I'd like to start very simple with a very simplistic trait

@emhane emhane requested a review from mattsse October 8, 2024 11:24
Copy link
Collaborator

@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.

cool, lgtm!

@emhane emhane enabled auto-merge October 18, 2024 09:29
@emhane emhane added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit cfd066c Oct 18, 2024
39 checks passed
@emhane emhane deleted the emhane/signed-tx-trait branch October 18, 2024 09:57
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-debt A clean up/refactor of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add trait abstraction for TransactionSigned
3 participants