umbracle / ethgo

Ethereum Golang API
https://www.ethgoproject.io
Mozilla Public License 2.0
484 stars 134 forks source link

Update blocktracker and tracker to provide synchronous API #246

Closed sergerad closed 1 year ago

sergerad commented 1 year ago

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

// This runs a goroutine for us and does not return all errors from its stack
tracker.Sync()

to

go func() {
    // This goroutine receives all errors from Sync stack, we decide what to do with those errors
    err := tracker.Sync()
}

Alternative Impl?

Could keep the funcs asynchronous and update them to return a channel of errors. More complicated, more side effects

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ethgo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2023 9:04pm
sergerad commented 1 year ago

@ferranbt could you please take a look at this PR. My biggest concern is the unhandled error

paulgoleary commented 1 year ago

bump @ferranbt @vcastellm

ferranbt commented 1 year ago

Please check the CI.

paulgoleary commented 1 year ago

The test that seems to be failing here, TestFilterIntegration does not fail for me locally.

pauloleary@Pauls-MacBook-Pro-3 ethgo % go test -v -run TestFilterIntegration ./tracker
=== RUN   TestFilterIntegration
--- PASS: TestFilterIntegration (3.97s)
PASS
ok      github.com/umbracle/ethgo/tracker   4.159s

Not sure what the CI problem could be?

sergerad commented 1 year ago

@paulgoleary thanks for taking a look. Sorry I have been out sick all week. Hoping to be back online tomorrow to look at this 🙏

paulgoleary commented 1 year ago

re-bump @ferranbt @vcastellm