-
Notifications
You must be signed in to change notification settings - Fork 134
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
Update blocktracker and tracker to provide synchronous API #246
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
67793d8
to
55ed9b2
Compare
if err := block.UnmarshalJSON(buf); err != nil { | ||
s.logger.Printf("[ERR]: Tracker failed to parse ethgo.Block: %v", err) | ||
} else { | ||
handle(&block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error was being ignored completely
@ferranbt could you please take a look at this PR. My biggest concern is the unhandled error |
bump @ferranbt @vcastellm |
Please check the CI. |
487e156
to
1cfddc7
Compare
92b909e
to
1cfddc7
Compare
This reverts commit 1cfddc7.
The test that seems to be failing here,
Not sure what the CI problem could be? |
@paulgoleary thanks for taking a look. Sorry I have been out sick all week. Hoping to be back online tomorrow to look at this 🙏 |
re-bump @ferranbt @vcastellm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
Why?
Pkg API funcs should return errors they encounter, rather than log them. This allows the caller to decide what to do on error and means we don't leak logging impl/semantics into ethgo (e.g. [ERROR] prefix).
See https://github.com/golang/go/wiki/CodeReviewComments#synchronous-functions
Usage goes from
to
Alternative Impl?
Could keep the funcs asynchronous and update them to return a channel of errors. More complicated, more side effects