Scope backup nudge to wallets created on-device#544
Conversation
Only show the recovery-phrase backup nudge for mnemonic-backed wallets created on this device (or legacy wallets of unknown origin). Track wallet origin (created vs imported) and never nudge keystone/hardware wallets, multisig/entrusted accounts, or imported wallets. Add an "I already backed up my wallet" action on the caution screen so users who already have their phrase can dismiss the nudge without viewing/copying.
n13
left a comment
n13
left a comment
There was a problem hiding this comment.
Review: Scope backup nudge to wallets created on-device (#544)
Summary of changes
The PR scopes the home backup/recovery-phrase nudge to only on-device created (or legacy unknown-origin) mnemonic wallets. It introduces WalletOrigin tracking, sets it on create/import paths, updates the backupReminderWalletIndexProvider with the necessary guards, and adds an "I already backed up my wallet" secondary action (only surfaced from the home nudge) to dismiss without forcing phrase reveal.
What looks good
- Correct scoping logic:
RegularAccount+accountType == local+origin != imported+!viewed+ balance threshold. Hardware, multisig, entrusted, and imported wallets are correctly excluded. - Legacy compatibility:
getWalletOriginreturningnull(unknown) still allows the nudge, matching the stated intent. - UX improvement: The new "I already backed up" path from the banner is a nice touch and only wired when
showAlreadyBackedUp: true. - Settings storage: Origin is stored per
walletIndex(same granularity as viewed/mnemonic), using the existing prefs pattern. - No linter/analyze issues on changed files; the "Analyze" CI check is green.
- Translations added for both
enandid(plus generated localizations).
Minor issues / suggestions (non-blocking)
-
Test coverage gap
wallet_creation_service_test.dartverifies mnemonic + account + referral, but does not assert the newsetWalletOrigin(WalletOrigin.created)call (in the!hasRootbranch) orverifyNeverin the has-root branch.
The import path that now callssetWalletOrigin(imported)also lacks a corresponding assertion/test. Recommend adding:verify(settings.setWalletOrigin(0, WalletOrigin.created)).called(1);
(and the negative case).
-
Slight duplication in wallet bootstrap paths (DRY)
Root account creation + mnemonic persist lives inWalletCreationService(used by welcome) and is duplicated inline inimport_wallet_screen.dart(setMnemonic + addAccount + discover). Origin is now set in both.
Not introduced by this PR, but a good future refactor would be to add animportWallet(...)helper in the service so both creation and import go through a single place for side effects like origin. -
Cleanup on wallet removal (hygiene)
settings_service.removeWalletdeletes the mnemonic but does not remove thewallet_origin_$norrecovery_phrase_viewed_$nkeys. SincenextWalletIndexcan reuse freed indices, a subsequent create/import on a reused index will overwrite, so no correctness bug today. Still, for cleanliness consider:_prefs.remove(_walletOriginKey(walletIndex)); _prefs.remove(_recoveryPhraseViewedKey(walletIndex));
(out of scope for this change).
-
Minor inconsistency
Import explicitly invalidateswalletOriginProviderafter setting; the welcome/create path does not. Because the provider is a plain derivedProviderthat reads prefs on access, it is not required for correctness, but adding the invalidate inwelcome_screen.dartwould be consistent. -
Debug flag interaction
debugAlwaysShowBackupNudgeis evaluated after the Regular+local type checks (good), so it won't force the nudge on hardware/imported in debug either.
Nitpicks
- The provider now has a multi-line doc comment explaining the rules. If the project prefers very minimal comments, the doc could be trimmed, but the explanation is helpful for maintainers.
- No behavior changes to multisig/entrusted/hardware flows or serialization.
Verdict
Approve
The change is focused, safe, and solves the stated problem without side effects on other account types. Ready to merge (test coverage improvement would be nice to have).
|
|
||
| /// When true (the home backup nudge), offers an "I already backed up" action | ||
| /// that dismisses the nudge without revealing the phrase. | ||
| final bool showAlreadyBackedUp; |
There was a problem hiding this comment.
is this necessary? we can just not displaying it at all if user already back up, can't we?
There was a problem hiding this comment.
It's an option on the screen because the screen is also used from settings. So when users open it from settings the button doesn't make sense.
dewabisma
left a comment
dewabisma
left a comment
There was a problem hiding this comment.
Overall looking good, just one thing to clarify.
dewabisma
left a comment
dewabisma
left a comment
There was a problem hiding this comment.
Actually will approve, but need clarification on the intent.
The recovery-phrase nudge stayed hidden for a freshly created wallet after a logout/reset because recoveryPhraseViewedProvider/walletOriginProvider are cached Provider.family instances that survived logout (clearAll wipes prefs from disk but not the Riverpod cache). - Invalidate both providers on logout so a reused wallet index reads fresh state - Invalidate them in the welcome create flow for consistency with the import flow - Clear wallet_origin_/recovery_phrase_viewed_ keys in removeWallet (hygiene)
2 participants
Summary
createdvsimported) via a newWalletOrigin, persisted perwalletIndexinSettingsService.Test plan