feat: clean up balance fetching from token manager #6991
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Jenkins BuildsClick to see older builds (66)
|
dlipicar
force-pushed
the
feat/clean-up-balance-fetching-from-token-manager
branch
10 times, most recently
from
31a7bc6 to
3f9f480
Compare
fbarbu15
approved these changes
Oct 10, 2025
Contributor
fbarbu15
left a comment
fbarbu15
left a comment
There was a problem hiding this comment.
Test part LGTM
Although for the smart contract I cannot give my approval since I don't have enough experience there
vkjr
reviewed
Oct 13, 2025
vkjr
reviewed
Oct 13, 2025
vkjr
reviewed
Oct 13, 2025
| c.stopCh = nil | ||
| } | ||
|
|
||
| c.stopWalletEventsWatcher() |
Contributor
There was a problem hiding this comment.
Stop() can be guarded by mutex since it closing and nilling channel, to make this change non-interrupted.
Contributor
Author
There was a problem hiding this comment.
ugh this point keeps coming up, I'll deal with it soon, I promise!
Start and Stop is, at the moment, only expected to be called when the app starts and ends, respectively, we don't expect race conditions to happen.
I feel it would be healthier for our services to be prepared for start and stop calls at any point during the application lifetime, it's just not a requirement at the moment. I'll apply this change to all services when I work on this improvement.
vkjr
approved these changes
Oct 13, 2025
friofry
reviewed
Oct 14, 2025
|
|
||
| ret := make(map[AccountAddress]map[ContractAddress]*big.Int) | ||
|
|
||
| for result := range resultsCh { |
Contributor
There was a problem hiding this comment.
For loop does not check for context cancellation
Contributor
Author
There was a problem hiding this comment.
context cancellation causes context.Canceled to be sent for every pending job, after which the channel is closed, at which point this loop exits.
friofry
reviewed
Oct 14, 2025
|
|
||
| for { | ||
| select { | ||
| case <-c.stopCh: |
Contributor
There was a problem hiding this comment.
from fetcher.go
// jobResultsCh is closed when all results have been sent, causing the // loop to exit and the goroutine to end.
So maybe, If stopCh stops the goroutine, we should still read everything from the channel
Contributor
Author
There was a problem hiding this comment.
hmm yeah, closing stopCh should cancel the ctx we pass to FetchBalances. I guess it's not super critical now since stopCh is only called when the app closes, but it's not proper to leave the Fetcher running in the background when the controller stops.
Contributor
Author
There was a problem hiding this comment.
oh wait it does!
go func() {
defer gocommon.LogOnPanic()
defer cancel()
for {
select {
case <-c.stopCh:
return
case result, ok := <-resultsCh:
if !ok {
return
}
c.handleFetchResult(ctx, chainID, result)
}
}
}()
closing stopCh ends the goroutine, which causes cancel() to be called.
Contributor
Author
There was a problem hiding this comment.
I don't think we're interested in any remaining results if we stop the controller, it's not a super critical operation we're dealing with here. If it were, calling Stop() should block until the operation is complete.
friofry
reviewed
Oct 14, 2025
| } | ||
|
|
||
| r.triggerReloadIfRecent(event.NewState.FetchedAt, event.Key.Account, true) | ||
| r.refreshBalanceCache(context.TODO(), []uint64{event.Key.ChainID}, []common.Address{event.Key.Account}) |
Contributor
There was a problem hiding this comment.
I wonder what is the plan with contexts?
For now maybe add a context with 60sec timeout?
Contributor
Author
There was a problem hiding this comment.
The ctx basically does nothing here, since no balances are getting "fetched", only read from cache. The plan is to remove reader.go completely soon and let tokenbalances provide the API that will get balances from the cache :D
friofry
approved these changes
Oct 14, 2025
Contributor
friofry
left a comment
friofry
left a comment
There was a problem hiding this comment.
Looks good to me! Nice solution!
Added a few comments
dlipicar
force-pushed
the
feat/clean-up-balance-fetching-from-token-manager
branch
from
3f9f480 to
19f7f81
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6991 +/- ##
===========================================
+ Coverage 55.11% 59.45% +4.34%
===========================================
Files 820 822 +2
Lines 116628 116494 -134
===========================================
+ Hits 64275 69257 +4982
+ Misses 45548 40146 -5402
- Partials 6805 7091 +286
Flags with carried forward coverage won't be shown. Click here to find out more.
|
siddarthkay
approved these changes
Oct 15, 2025
Contributor
siddarthkay
left a comment
siddarthkay
left a comment
There was a problem hiding this comment.
changes in tests-functional/Dockerfile looks okay from devops POV
dlipicar
deleted the
feat/clean-up-balance-fetching-from-token-manager
branch
AYAHASSAN287
pushed a commit
that referenced
this pull request
Oct 20, 2025
* feat: improvemets to multistandardbalance controller * feat: introduce tokenbalances package * chore: replace balance fetching capabilities from reader and token manager with tokenbalances * feat: allow overriding multicall3 contract address * chore: fix tests * chore: pr comments
AYAHASSAN287
pushed a commit
that referenced
this pull request
Oct 20, 2025
* feat: improvemets to multistandardbalance controller * feat: introduce tokenbalances package * chore: replace balance fetching capabilities from reader and token manager with tokenbalances * feat: allow overriding multicall3 contract address * chore: fix tests * chore: pr comments
romanzac
pushed a commit
that referenced
this pull request
Nov 6, 2025
* feat: improvemets to multistandardbalance controller * feat: introduce tokenbalances package * chore: replace balance fetching capabilities from reader and token manager with tokenbalances * feat: allow overriding multicall3 contract address * chore: fix tests * chore: pr comments
romanzac
added a commit
that referenced
this pull request
Nov 14, 2025
* test: First draft for settings wrappers and test file added * test: Making changes in test env & add tests * test: Add new test * feat: improve blockchainstate * feat: replace old transfer detection algorithm with multistandardbalance and transferdetector * build: add Android x86_64 support to Makefile (#6990) - Sets MOBILE_GOARCH to amd64 when ARCH is x86_64 - Sets ANDROID_CLANG_TARGET to x86_64-linux-android<API> * ci: use official base docker image for builds Referenced issue: * status-im/infra-ci#188 * test: community chats (#6987) * test: community chats * test: community chats review * test: Adding node tests * test: contact verification (#6997) * chore: point go.mod to original go-ethereum repo * chore: drop usage of non-multichain ethclient methods in rpc chain client * chore: drop usage of non-multichain ethclient methods in community tokens service * chore: drop usage of non-multichain ethclient methods in fees manager * chore: adapt to new geth logger * chore: remove unused log_parser code * chore: replace custom NoSign parameter from bind.TransactOpts * chore: update SingleRequestCodec to go-ethereum upstream * chore: use `go-waku` with upstream `go-ethereum` (#6978) * chore: upgrade go-waku and go-discover * chore: make vendor * feat: go-waku v0.10.0 * chore: fix tests * chore: bump go-waku and go-discover (#6983) * refactor: use common Migrate function - Use sqlite.Migrate in protocol to remove logic duplication. - Remove legacy ad-hoc migration fix. It is now safe to assume that no users are running versions old enough to require this workaround. * refactor: move pubkeys utils from protocol to crypto package These utils are generic enough to be moved to crypto package. * fix: Crash on uninitialized filterManager Not sure if this will only hide the root cause because the proper flow on pairing is unknown to me. But from what I've seen Waku is only started after login. While `subscribe` and `unsubscribe` is used in the pairing flow as well. * test: Add new tests * test: Adding wrappers &tests * test: Add positive & negative tests * test: Added backup tests * feat: clean up balance fetching from token manager (#6991) * feat: improvemets to multistandardbalance controller * feat: introduce tokenbalances package * chore: replace balance fetching capabilities from reader and token manager with tokenbalances * feat: allow overriding multicall3 contract address * chore: fix tests * chore: pr comments * fix(scripts): branch_version_generated use current base branch (#7007) * fix: goimports configuration (#7004) * fix: goimports only 1 local package * fix: vscode settings goimports * fix: un-gitignore IDEA code styles * feat: goland code styles * fix: remove generate_handlers nolint comment * chore: lint-fix all files * feat: local timesource (#7003) * feat: local timesource * fix: nwaku * fix: rebase issues * feat(benchmark): threads and goroutines count (#7006) * feat: added numThreads to expvars * test: added num of threads and goroutines to benchmarks * fix: issues * chore: cleanup chain client and transactor (#7008) * chore: make rpc.Client implement EthClientGetter * chore: remove unused types * chore: cleanup unused methods and use interface instead of rpc Client * chore: cleanup transactor * chore: move pendingtxtracker to separate package * chore: fix tests * test: add all APIs tests * test: Apply linters * fix: reduce test variants to shorten test durations * fix: lints * fix: remove print statements for debug-only purpose * fix: use logger when necessary * chore: use `go-waku` with upstream `go-ethereum` (#6978) * chore: upgrade go-waku and go-discover * chore: make vendor * feat: go-waku v0.10.0 * chore: bump go-waku and go-discover (#6983) * feat: clean up balance fetching from token manager (#6991) * feat: improvemets to multistandardbalance controller * feat: introduce tokenbalances package * chore: replace balance fetching capabilities from reader and token manager with tokenbalances * feat: allow overriding multicall3 contract address * chore: fix tests * chore: pr comments * fix: rebase on develop - remove unused imports * fix: wait longer for backend in Jenkins * test: look for potential problems beyond failing tests * test: test_check_node_config_params * test: service context behavior * test: revert wakuext service name * fix: test_news_feed_enabled * fix: test_news_notifications_enabled * fix: test_toggle_news_notifications_enabled * fix: test_toggle_news_rss_enabled * fix: test_set_valid_backup_path * fix: wait 40s for backend in Jenkins * fix: remove unused function * fix: test_last_tokens_update_advances_after_updating_token_preferences * test: temporary fix for - test_delete_exemptions_invalid_id * test: temporary fix for - test_notifications_set_global_mentions_rejects_wrong_types_and_preserves_value * test: race condition fix for - test_update_keycard_uid_success * test: return to the original timeout value * fix: remove redundant deepcopy * fix: remove line without effect in - test_full_migrate_flow * fix: change to skip annotation * fix: improve assertions * fix: remove ExemptionsDefaults related code * fix: uncomment - test_notifications_set_group_chats_invalid_values * fix: remove Go code for - NotificationsSetGlobalMentions - DeleteExemptions * fix: commented out tests - test_notifications_set_global_mentions_rejects_wrong_types_and_preserves_value - test_delete_exemptions_invalid_id * fix: naming and test clarity * fix: remove redundant news feed tests * fix: regroup tests * fix: re-run stuck codecov tests --------- Co-authored-by: Dario Gabriel Lipicar <[email protected]> Co-authored-by: Mag. <[email protected]> Co-authored-by: markoburcul <[email protected]> Co-authored-by: fbarbu15 <[email protected]> Co-authored-by: Patryk Osmaczko <[email protected]> Co-authored-by: Igor Sirotin <[email protected]> Co-authored-by: Alex Jbanca <[email protected]> Co-authored-by: Roman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the following changes, in separate commits:
multistandardbalancetokenbalancespackagemultistandardbalancereaderandtoken.Managerwithtokenbalancestoken.Manageris now only in charge of the token listsreaderconsumes the balances fromtokenbalances, since it no longer has a need to trigger fetches.Allow overriding the multicall contract address through config params for functional-tests usage. Deploy multicall3 in Anvil for functional tests.
unit test fixes
A follow-up PR will bring the full removal of
readerand changes to the API. This PR was intended to keep the app functional with no API (methods/events) changes.