backend: move eth update logic into its own package. #3503
Conversation
bznein
force-pushed
the
extractETHUpdater
branch
2 times, most recently
from
f71685d to
895101e
Compare
bznein
changed the title
bznein
force-pushed
the
extractETHUpdater
branch
3 times, most recently
from
5a784e5 to
87f08e2
Compare
bznein
force-pushed
the
extractETHUpdater
branch
5 times, most recently
from
5a6c73b to
4d139a6
Compare
bznein
changed the title
backend/backend.go
Outdated
| @@ -310,6 +302,7 @@ func NewBackend(arguments *arguments.Arguments, environment Environment) (*Backe | |||
| backend.socksProxy = backendProxy | |||
| backend.httpClient = hclient | |||
| backend.etherScanHTTPClient = hclient | |||
| backend.ethupdater = eth.NewUpdater(accountUpdate, backend.etherScanHTTPClient, backend.etherScanRateLimiter, backend.updateETHAccounts) | |||
There was a problem hiding this comment.
I could swear we removed etherScanHTTPClient in a recent PR 🤔 I think I commented somewhere that we can remove it as it's the same as backend.httpClient, now that the rate limiter is separate from the http client.
There was a problem hiding this comment.
I could swear we removed etherScanHTTPClient in a recent PR 🤔
Now that you mention, I think I remember it too :D maybe it was in a staging branch for blockbook? Anyway, will do :)
backend/coins/eth/updater.go
Outdated
| close(u.enqueueUpdateForAccount) | ||
| close(u.updateETHAccountsCh) |
There was a problem hiding this comment.
No need to close these explicitly afaik. They will be garbage collected, and no risk in making the poll loop handle these events (even though it should not happen with the double nesting on the quit channel).
updateETHAccountsCh should especially not be closed here as it was not created here either.
backend/coins/eth/updater.go
Outdated
| switch { | ||
| case IsERC20(account): | ||
| var err error | ||
| balance, err = account.ERC20Balance() |
There was a problem hiding this comment.
Don't think it's worth introducing a new function for it, as it's only used once, it's short and can just be inlined here as before.
backend/coins/eth/updater.go
Outdated
| quit chan struct{} | ||
|
|
||
| // enqueueUpdateForAccount is used to enqueue an update for a specific ETH account. | ||
| enqueueUpdateForAccount chan *Account |
There was a problem hiding this comment.
Change the type to <- chan *Account to indicate it can only receive, not send/close.
backend/coins/eth/updater.go
Outdated
| } | ||
|
|
||
| // NewUpdater creates a new Updater instance. | ||
| func NewUpdater(accountUpdate chan *Account, etherscanClient *http.Client, etherscanRateLimiter *rate.Limiter, updateETHAccounts func() error) *Updater { |
There was a problem hiding this comment.
Please split into multiple lines (one per argument). Best to try keep lines <100 chars.
There was a problem hiding this comment.
Please split into multiple lines (one per argument). Best to try keep lines <100 chars.
Will do, but just for reference I didn't initially as I usually try to follow what I remember from Google's style guide (https://google.github.io/styleguide/go/guide.html#line-length)
There was a problem hiding this comment.
"if it feels too long", very clear 😂
In my experience 100 chars is pretty good and fits on split screens in vim/emacs on most displays.
backend/coins/eth/updater.go
Outdated
| }() | ||
| case <-u.updateETHAccountsCh: | ||
| go updateAll() | ||
| timer = time.After(PollInterval) |
There was a problem hiding this comment.
move the Pollnterval variable to this file and unexport it - it's not used outside this file anymore.
bznein
force-pushed
the
extractETHUpdater
branch
from
4d139a6 to
8bd5bc4
Compare
|
Ready for another review, thanks @benma ! |
The only eth-balance specific code that remains in the backend is
updateETHAccounts(which is passed to the updater as a callback) which I think needs to stay there as it needs access to both the accounts and keystore lock, and the slice of accounts.Before asking for reviews, here is a check list of the most common things you might need to consider: