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.

Sign up for GitHub

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

Jump to bottom

feat: unify recover fn result type #13897

Merged
merged 12 commits into from
Jan 22, 2025
Merged

feat: unify recover fn result type #13897

merged 12 commits into from
Jan 22, 2025

Conversation

hoank101
Copy link
Contributor

Closes #13893

mattsse
mattsse requested changes Jan 21, 2025
View reviewed changes
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.

this looks more invasive than expected.

why do we need to change a lot of existing functions?
this pr should only change the return type from option to result

crates/primitives-traits/src/block/recovered.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/block/sealed.rs Outdated Show resolved Hide resolved
@hoank101 hoank101 marked this pull request as ready for review January 21, 2025 16:36
@hoank101 hoank101 requested a review from mattsse January 21, 2025 16:36
@hoank101 hoank101 marked this pull request as draft January 21, 2025 16:42
mattsse
mattsse requested changes Jan 21, 2025
View reviewed changes
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, this is on track

a few more nits and suggestions

crates/ethereum/primitives/src/transaction.rs Outdated Show resolved Hide resolved
crates/optimism/primitives/src/transaction/signed.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/block/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/crypto.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/signed.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/signed.rs Outdated Show resolved Hide resolved
crates/rpc/rpc-eth-types/src/receipt.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/debug.rs Outdated Show resolved Hide resolved
crates/storage/provider/src/providers/database/provider.rs Outdated Show resolved Hide resolved
@mattsse mattsse marked this pull request as ready for review January 21, 2025 23:13
@mattsse mattsse added the A-sdk Related to reth's use as a library label Jan 21, 2025
@hoank101 hoank101 requested a review from mattsse January 22, 2025 13:01
mattsse
mattsse requested changes Jan 22, 2025
View reviewed changes
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.

last q

crates/primitives-traits/src/block/mod.rs Outdated Show resolved Hide resolved
@hoank101
Copy link
Contributor Author

thank @mattsse for review, i just updated

@hoank101 hoank101 requested a review from mattsse January 22, 2025 13:46
mattsse
mattsse approved these changes Jan 22, 2025
View reviewed changes
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.

lgtm, tysm!

@hoank101
Merge branch 'main' of github.com:paradigmxyz/reth into feat/13893
3d84747 
# Conflicts:
#	crates/storage/errors/src/provider.rs
@mattsse mattsse added this pull request to the merge queue Jan 22, 2025
Merged via the queue into paradigmxyz:main with commit 926ad2a Jan 22, 2025
43 checks passed
refcell pushed a commit to ethereum-optimism/op-reth that referenced this pull request Jan 24, 2025
@hoank101 @mattsse @refcell
feat: unify recover fn result type (paradigmxyz#13897)
4e6b1a6 
Co-authored-by: Matthias Seitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@rakita rakita Awaiting requested review from rakita rakita is a code owner

@joshieDo joshieDo Awaiting requested review from joshieDo joshieDo is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin shekhirin is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

Assignees
No one assigned
Labels
A-sdk Related to reth's use as a library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify recover fn result type
2 participants
@hoank101 @mattsse