Skip to content

fix(notification-services-controller): source wallet-activity addresses from the keyring - #9985

Open
oilnam wants to merge 3 commits into
mainfrom
fix/wallet-activity-addresses-from-keyring
Open

fix(notification-services-controller): source wallet-activity addresses from the keyring#9985
oilnam wants to merge 3 commits into
mainfrom
fix/wallet-activity-addresses-from-keyring

Conversation

@oilnam

@oilnam oilnam commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Explanation

Users are receiving notifications for addresses they do not own.

Since notification-services-controller v24, the wallet-activity address list has been stored in Authenticated User Storage, whose notification-preferences blob is keyed by canonical profile ID. Profile pairing shares that ID across every SRP belonging to the same human, so the stored list pools the addresses of unrelated SRPs and every installation reads back the union of all of them. Addresses are keyring-scoped and cannot live in profile-scoped storage.

This restores the pre-v24 arrangement: the keyring is the source of truth for which addresses exist, and the Trigger API for which of them are enabled.

What changed

  • checkAccountsPresence, fetchAndUpdateMetamaskNotifications and enablePushNotifications take the candidate addresses from the keyring and read the per-address enabled bit from the Trigger API. An installation can therefore only ever ask about addresses it actually holds.
  • enableAccounts / disableAccounts write subscriptions through the reintroduced updateOnChainNotifications. The endpoint is a per-address upsert rather than a full replace, so two installations authenticating as the same profile no longer clobber each other's subscriptions.
  • createOnChainTriggers writes walletActivity.accounts: [] and subscribes the keyring's accounts only on genuine first-time setup.

Two decisions worth reviewing

First-time subscribe is gated on the absence of a preferences blob, not the absence of subscriptions. createOnChainTriggers also runs on the daily re-subscribe, so the pre-v24 condition ("no subscriptions yet, enable everything") would re-enable every account each day for a user who had turned them all off. Even at first-time setup, existing subscriptions win, so a user upgrading from a client that never wrote a preferences blob keeps whichever accounts they had already disabled.

The two user-level walletActivity channel toggles are still read from Authenticated User Storage. They contain no addresses, so sharing them across a paired profile is correct, and they need no follow-up removal. An unreadable blob now counts as both toggles enabled, matching the API's "every toggle is on unless stated otherwise" default — previously a storage outage produced an empty address list and silently emptied the notification list.

Drive-by fix

The device is now unregistered from push when no account has notifications enabled. The push API rejects a registration with no addresses, and that request is what performs the delete-and-reinsert of the device's links, so the rejection left the previous links in place and push kept arriving after a user disabled every account. This was reachable before this PR too, via an all-disabled list in storage.

References

  • Backend prerequisite: consensys-vertical-apps/va-mmcx-notify-users-storage#124, which preserves the stored account list when a client sends an empty one, so this change cannot blank the list that unmigrated installations on the same profile still read.
  • Client PRs to follow once this is released; both have had their now-dead reads of walletActivity.accounts removed locally and are ready to raise.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Testing

292 tests pass across the package's 18 suites with coverage thresholds met. New coverage for: the Trigger API write path including address lower-casing, the daily re-subscribe not re-enabling disabled accounts, existing subscriptions surviving first-time setup, both push-unregister paths, the wallet-activity in-app toggle gating the on-chain fetch without affecting other notification types, and the fetch surviving an unavailable preferences blob.

Note

yarn lint currently fails on clean main in my environment with 89 parsing errors, all in gitignored .tsc-lint-cache files across unrelated packages: lint:tsc generates those caches and build:only-clean doesn't remove them before eslint runs. yarn eslint scoped to this package passes clean. Flagging in case it isn't just my machine.

Made with Cursor

…es from the keyring

Authenticated User Storage is keyed by canonical profile ID, which profile
pairing shares across every SRP belonging to the same user. Storing the
wallet-activity address list there therefore pooled the addresses of unrelated
SRPs and delivered each installation the union of all of them, so users
received notifications for addresses they do not hold. Addresses are
keyring-scoped and cannot live in profile-scoped storage.

Candidate addresses now come from the keyring and the per-address enabled bit
from the Trigger API, so an installation can only ever ask about addresses it
holds. enableAccounts/disableAccounts write subscriptions back to the Trigger
API, whose per-address upsert means two installations on the same profile no
longer clobber each other.

createOnChainTriggers subscribes the keyring only on genuine first-time setup,
gated on the absence of a preferences blob rather than the absence of
subscriptions: it also runs on the daily re-subscribe, so the latter condition
would re-enable every account each day for a user who had turned them all off.

The user-level walletActivity in-app and push toggles are still read from
Authenticated User Storage. They contain no addresses, so they remain correct
to share across a paired profile. An unreadable blob now counts as both
toggles enabled, so a storage outage no longer silently empties the
notification list.

Also unregister the device from push when no account has notifications
enabled. The push API rejects a registration with no addresses, and that
request is what performs the delete-and-reinsert of the device's links, so the
rejection left the previous links in place and push kept arriving after a user
disabled every account.

Co-authored-by: Cursor <cursoragent@cursor.com>
@oilnam
oilnam requested review from a team as code owners August 27, 2026 12:20
@oilnam
oilnam deployed to default-branch August 27, 2026 12:20 — with GitHub Actions Active
Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 424c450. Configure here.

…igger API config

A failed Trigger API config query returned the same empty list as "no address
is subscribed", so a transient outage could re-subscribe accounts the user had
turned off, unregister the device from push, or report every account as
disabled. Failure is now distinguishable from emptiness and each caller leaves
the existing state alone instead of guessing.

Subscription writes also treated a 4xx/5xx as success and cached the intended
state, so the settings UI, the config cache and the push links could diverge
from the server for the full cache TTL. They now reject.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant