Skip to content

Miner improvements#489

Merged
n13 merged 4 commits into
mainfrom
miner-improvements
May 17, 2026
Merged

Miner improvements#489
n13 merged 4 commits into
mainfrom
miner-improvements

Conversation

@n13
Copy link
Copy Markdown
Collaborator

@n13 n13 commented May 17, 2026

  • Logout wallet only
  • Add warning text for inner hash (can't redeem)
  • More Bip39 validation, better error messages

n13 added 3 commits May 17, 2026 12:40
logout without losing all data
tell user that inner hash won't have redeem function
@v12-auditor
Copy link
Copy Markdown

v12-auditor Bot commented May 17, 2026

Warning

No auditable source files found in this PR's diff.

@n13
Copy link
Copy Markdown
Collaborator Author

n13 commented May 17, 2026

PR review: miner-improvements

Tested the branch locally — analyzer is clean on all four files and all 9 new tests pass. Overall this is a focused, well-scoped UX improvement.

What's in it

  1. Wallet-only logout (miner_app_bar.dart) — splits the existing single "Logout (Full Reset)" item into:
    • Logout Wallet (orange key-off icon): wipes mnemonic + preimage, stops mining, keeps node binary + identity, routes to /rewards_address_setup.
    • Reset App (red logout icon): the original full reset.
  2. Better mnemonic import errors (rewards_address_setup_screen.dart) — now distinguishes "word not in BIP-39 wordlist (typo)" from "checksum invalid (wrong order / valid-but-wrong word)", and lists the offending words with 1-based positions.
  3. Mnemonic normalization (miner_wallet_service.dart) — new normalizeMnemonic (trim + lowercase + whitespace collapse) and findInvalidMnemonicWords helpers; validateMnemonic and saveMnemonic now go through normalize, so mixed-case input works.
  4. Copy updates — inner-hash setup screen now correctly says redeeming is impossible with only the inner hash (previously it said "withdraw via CLI", which was misleading since the inner hash alone is insufficient to redeem anywhere).
  5. Tests — new miner_wallet_service_test.dart covering the wordlist check, mixed case, whitespace, reversed-but-valid words, 12 vs 24 words.

Things I like

  • The two-tier logout is the right model — full reset was too destructive for the common "I entered the wrong phrase" case.
  • The error-message split is genuinely useful: "Not in the BIP-39 wordlist: 'driip' (word 3)" is far more actionable than the old generic message.
  • Stopping the orchestrator before wiping wallet data avoids a race where mining keeps running against a now-empty preimage.
  • Tests use a real valid mnemonic and cover the interesting edge cases (reversed-valid, mixed case, normalization, both word counts).
  • Records (({int position, String word})) keep the helper's API tight without a one-off class.

Suggestions / nits

  1. Stale doc comment in miner_wallet_service.dart (lines 16–17): still says "the user must withdraw via CLI" for preimage-only mode, but the UI copy was just corrected to clarify redeeming requires the secret phrase from anywhere. Suggest aligning the comment:
   /// - Preimage-only: only the rewards preimage file is written. The node can mine
   ///   but the user must withdraw via CLI.

Something like "…the node can mine but redeeming the rewards requires the secret phrase (not stored)." would match the new screen copy.

  1. _performLogout doesn't stop the orchestrator before deleting the node binary (pre-existing behavior, not introduced here). Now that _performWalletLogout does it correctly, the inconsistency is more visible — worth doing the same orchestrator?.stop() in the full Reset path in a follow-up.

  2. Minor copy: the wallet-logout dialog body says "wipe your secret phrase and inner hash from this app". Technically the preimage file (not "inner hash") is what's deleted; users imported in inner-hash mode never had a secret phrase to wipe. Not wrong, just slightly imprecise — could say "…wipe your secret phrase or inner hash, whichever you entered…". Up to you.

  3. findInvalidMnemonicWords is only used by the import screen, where the caller has already split on \s+ and joined with single spaces before calling it. The helper does the same split again via normalizeMnemonic. Not a bug, just one redundant tokenization per import (negligible). Could be tightened by passing words directly, but the current shape is more reusable and the tests rely on the string form, so leaving it is fine.

  4. Dart records public API: List<({int position, String word})> is exposed across the service boundary. That's idiomatic in 3.x but if you ever want to extend the return shape (e.g. add suggestion: 'did-you-mean'), promoting to a tiny named class later will be a touch easier than evolving an inline record. Not a blocker.

Verification

flutter analyze lib/features/miner/miner_app_bar.dart \
                lib/features/setup/rewards_address_setup_screen.dart \
                lib/src/services/miner_wallet_service.dart \
                test/miner_wallet_service_test.dart
# No issues found! (1.6s)

flutter test test/miner_wallet_service_test.dart
# 00:00 +9: All tests passed!

Also confirmed Language.english.isValid(word) is a real API in bip39_mnemonic 3.0.10, and the route /rewards_address_setup plus redirect logic in main.dart correctly handles the post-wallet-logout state (since isSetupComplete() returns false once the preimage file is gone).

LGTM with the doc-comment nit. Nothing here looks risky to merge.

@n13 n13 merged commit fe5d672 into main May 17, 2026
2 checks passed
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