fix(auth): store bip39 compatibility regardless of wallet type#216
fix(auth): store bip39 compatibility regardless of wallet type#216
Conversation
previous behaviour of throwing if HD wallet seed is not bip39 compatible
pre-emptive avoidance of build errors as dev builds are pruned regularly
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughUpdates build configuration hashes and checksums across platforms and adds BIP39 seed compatibility checks during user registration in the local auth service, introducing a helper to decrypt and validate the mnemonic and adjusting the HD wallet registration flow to verify and return early on success/failure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant AuthService
participant Crypto as Crypto/KeyStore
participant BIP39 as MnemonicValidator
App->>AuthService: registerNewUser(config)
AuthService->>Crypto: decrypt seed (_getMnemonic)
Crypto-->>AuthService: mnemonic or error
AuthService->>BIP39: validate mnemonic
BIP39-->>AuthService: isValid (true/false)
AuthService->>AuthService: create KdfUser(isBip39Seed = isValid)
alt HD wallet
AuthService->>Crypto: decrypt seed
Crypto-->>AuthService: mnemonic
AuthService->>BIP39: validate
BIP39-->>AuthService: isValid
alt valid
AuthService-->>App: return verified KdfUser
else invalid
AuthService-->>App: throw AuthException
end
else Non-HD wallet
AuthService-->>App: return KdfUser
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes an issue where the HD wallet toggle was not appearing for legacy wallet migrations and seed file imports by ensuring the isBip39Seed flag is properly set regardless of wallet type. Previously, this flag defaulted to false for iguana wallets and was only updated for HD registrations.
Key changes:
- Extract BIP39 compatibility check into a separate method that runs for all wallet types
- Always set
isBip39Seedflag based on actual seed validation rather than wallet type - Update build configuration with new commit hashes and checksums
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| auth_service_auth_extension.dart | Refactored to always check BIP39 compatibility and set the flag appropriately for all wallet types |
| build_config.json | Updated API commit hash and platform-specific checksums for new build artifacts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/komodo_defi_local_auth/lib/src/auth/auth_service_auth_extension.dart (3)
26-29: Fix invalid argument order in _verifyBip39Compatibility call (positional after named).Dart requires positional args before named. This currently won’t compile.
- currentUser = await _verifyBip39Compatibility( - walletPassword: config.walletPassword, - currentUser, - ); + currentUser = await _verifyBip39Compatibility( + currentUser, + walletPassword: config.walletPassword, + );
132-134: Use a single initialized MnemonicValidator instance.Avoid calling init() on one instance and validate() on another.
- await MnemonicValidator().init(); - isBip39 = MnemonicValidator().validateBip39(plaintext.plaintextMnemonic!); + final validator = MnemonicValidator(); + await validator.init(); + isBip39 = validator.validateBip39(plaintext.plaintextMnemonic!);
146-152: Preserve specific AuthException types; current catch masks invalidBip39Mnemonic.Everything thrown inside the try (including the explicit invalidBip39Mnemonic) is wrapped as generalAuthError, losing signal for callers.
- } catch (e) { - await _stopKdf(); - throw AuthException( - 'Failed to verify seed compatibility: $e', - type: AuthExceptionType.generalAuthError, - ); - } + } on AuthException { + await _stopKdf(); + rethrow; + } catch (e) { + await _stopKdf(); + throw AuthException( + 'Failed to verify seed compatibility: $e', + type: AuthExceptionType.generalAuthError, + ); + }
🧹 Nitpick comments (1)
packages/komodo_defi_local_auth/lib/src/auth/auth_service_auth_extension.dart (1)
65-76: Remove redundant try/catch that only rethrows.It adds noise without changing behavior; let the callee surface precise errors.
- try { - return await _verifyBip39Compatibility( - currentUser, - walletPassword: config.walletPassword, - ); - } on AuthException { - // Verify BIP39 compatibility for HD wallets after registration - // if verification fails, the user can still log into the wallet in legacy - // mode. - rethrow; - } + return await _verifyBip39Compatibility( + currentUser, + walletPassword: config.walletPassword, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/komodo_defi_framework/app_build/build_config.json(3 hunks)packages/komodo_defi_local_auth/lib/src/auth/auth_service_auth_extension.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
- GitHub Check: Flutter tests (all packages)
🔇 Additional comments (3)
packages/komodo_defi_local_auth/lib/src/auth/auth_service_auth_extension.dart (2)
58-60: Good change: set isBip39Seed during registration for all wallet types.This aligns with the PR goal so the HD toggle can be shown appropriately post-login.
62-64: Clarify comment vs behavior.The comments suggest legacy-mode fallback on HD verification failure, but the code rethrows (after the refactor above you’ll still propagate errors). Confirm intended UX; if fallback should occur here, handle it locally instead of throwing.
Also applies to: 71-74
packages/komodo_defi_framework/app_build/build_config.json (1)
3-3: Hashes & checksums validated Verified both commit hashes exist on GitHub and all SHA-256 checksum entries are valid 64-hex strings.
CharlVS
left a comment
CharlVS
left a comment
There was a problem hiding this comment.
Thank you for taking initiative with this issue.
Fixes the issue with the HD wallet toggle not appearing for legacy migrations and seed file imports. Related to #3068, and it's follow-up #3126.
Changes
isBip39Seedregardless of wallet type This previously defaulted tofalseforiguanawallets and only updated for HD registrations.Context
The
WalletLogInpage in komodo-wallet uses theisBip39Seedflag to determine whether to show the HD wallet toggle, which would previously remain hidden for legacy wallet migrations, and likely seed file imports as well.When migrating a legacy wallet or importing a seed file, the user's funds are in the "iguana" derived wallet, so the initial login remains iguana, while allowing for login to the HD wallet if the user wishes (via the HD toggle).
--- title: Previous implementation (with bip39 compatible seed) --- flowchart LR lwl[Legacy Wallet Login] lsr[Register SDK wallet with legacy wallet seed] lsrc@{ shape: comment, label: "derivationMethod: iguana isBip39Seed: false show HD toggle: true" } lwl --> lsr --> lsrc--- title: Current implementation (with bip39 compatible seed) --- flowchart LR lwl[Legacy Wallet Login] lsr[Register SDK wallet with legacy wallet seed] lsrc@{ shape: comment, label: "derivationMethod: iguana isBip39Seed: true show HD toggle: true" } lwl --> lsr --> lsrcBefore
before.mov
After
after.mov
Summary by CodeRabbit
Bug Fixes
Chores