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

ethclient: simplify error handling in TransactionReceipt #28748

Merged
merged 7 commits into from
Jan 4, 2024
Merged

ethclient: simplify error handling in TransactionReceipt #28748

merged 7 commits into from
Jan 4, 2024

Conversation

rosenk
Copy link
Contributor

@rosenk rosenk commented Dec 29, 2023 •
edited by MariusVanDerWijden

Improves the error handling logic within the ethclient to make it easier to understand

holiman
holiman previously requested changes Dec 30, 2023
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Minor nit, otherwise LGTM

ethclient/ethclient.go Outdated Show resolved Hide resolved
@holiman holiman changed the title improve error handling ethclient: improve error handling Dec 30, 2023
@rosenk rosenk requested a review from holiman December 30, 2023 21:48
@rosenk
Copy link
Contributor Author

rosenk commented Jan 1, 2024

I don't understand why AppVeyor failed

@rjl493456442 rjl493456442 added this to the 1.13.8 milestone Jan 2, 2024
@fjl
Copy link
Contributor

fjl commented Jan 2, 2024

I think the two versions are equally easy to understand. There is basically no difference.

@rosenk
Copy link
Contributor Author

rosenk commented Jan 2, 2024

Why i386 linux tests failed?

@lightclient
Copy link
Member

lightclient commented Jan 2, 2024 •
edited

I agree that both versions are equally readable, but I think this PR is slightly more idiomatic because of the err != nil check and avoidance of nested if.

Kinda nitpicky though. Not sure why not combine with err == nil && r == nil though.

@rosenk
Copy link
Contributor Author

rosenk commented Jan 3, 2024

I don’t understand what is going on, why this pull request merge is still blocked.

@fjl
Copy link
Contributor

fjl commented Jan 4, 2024

Changed to the err == nil && r == nil construction since it is commonly used elsewhere in the file.

@fjl fjl changed the title ethclient: improve error handling ethclient: simplify error handling in TransactionReceipt Jan 4, 2024
@fjl fjl merged commit e3eeb64 into ethereum:master Jan 4, 2024
1 of 3 checks passed
@rosenk rosenk deleted the patch-1 branch January 4, 2024 16:15
@MariusVanDerWijden MariusVanDerWijden modified the milestones: 1.13.8, 1.13.9 Jan 10, 2024
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
@rosenk @holiman
@fjl @Dergarcon
ethclient: simplify error handling in TransactionReceipt (ethereum#28748
9b32d3b 
)


Co-authored-by: Martin HS <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Apr 29, 2024
@rosenk @holiman
@fjl @roberto-bayardo
ethclient: simplify error handling in TransactionReceipt (ethereum#28748
3324a90 
)


Co-authored-by: Martin HS <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjl fjl fjl approved these changes

@rjl493456442 rjl493456442 rjl493456442 approved these changes

@MariusVanDerWijden MariusVanDerWijden MariusVanDerWijden approved these changes

@holiman holiman Awaiting requested review from holiman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.13.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rosenk @fjl @lightclient @holiman @rjl493456442 @MariusVanDerWijden