hide custom seed toggle unless bip39 validation fails#3260
Conversation
|
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 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. WalkthroughModifies wallet import flows to conditionally display the custom seed toggle based on BIP39 validation results. The toggle is now hidden by default and revealed only when BIP39 validation fails or in specific HD mode scenarios, across two import widget files. Changes
Sequence DiagramsequenceDiagram
participant User
participant ImportUI
participant Validator
participant StateManager
User->>ImportUI: Initiate Import
ImportUI->>StateManager: Clear Prior Errors
ImportUI->>Validator: Validate Seed (BIP39)
alt Valid BIP39 Seed
Validator-->>ImportUI: ✓ Valid
ImportUI->>StateManager: Hide Custom Seed Toggle
ImportUI->>User: Proceed with Import
else Invalid + HD Mode
Validator-->>ImportUI: ✗ Invalid
ImportUI->>StateManager: Set HD-Specific Error
ImportUI->>StateManager: Show Custom Seed Toggle
ImportUI->>User: Display Error & Toggle
else Invalid + Custom Seed Not Allowed
Validator-->>ImportUI: ✗ Invalid
ImportUI->>StateManager: Set BIP39 Error
ImportUI->>StateManager: Show Custom Seed Toggle
ImportUI->>User: Display Error & Toggle
else Invalid + Custom Seed Allowed
Validator-->>ImportUI: ✗ Invalid (but custom allowed)
ImportUI->>User: Proceed with Custom Seed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Visit the preview URL for this PR (updated for commit b9ef42b): https://walletrc--pull-3260-merge-kg7oibpe.web.app (expires Wed, 05 Nov 2025 06:24:22 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@cursoragent please review @coderabbitai please review |
|
I completed a focused review of the PR changes around the custom seed toggle, BIP39 validation, and HD (multi-address) mode flows. Verdict
What I verified
bool get _shouldShowCustomSeedToggle {
if (_isHdMode) return false;
if (_allowCustomSeed) return true; // keep visible once enabled
final seed = _seedController.text.trim();
if (seed.isEmpty) return false;
final validator = context.read<KomodoDefiSdk>().mnemonicValidator;
final isBip39 = validator.validateBip39(seed);
return !isBip39;
} return switch (maybeFailedReason) {
MnemonicFailedReason.empty =>
LocaleKeys.walletCreationEmptySeedError.tr(),
MnemonicFailedReason.customNotSupportedForHd =>
_isHdMode
? LocaleKeys.walletCreationHdBip39SeedError.tr()
: LocaleKeys.walletCreationBip39SeedError.tr(),
MnemonicFailedReason.customNotAllowed =>
LocaleKeys.customSeedWarningText.tr(),
final bool isBip39 = _sdk.mnemonicValidator.validateBip39(decryptedSeed);
if (!isBip39) {
if (_isHdMode) {
setState(() {
_commonError = LocaleKeys.walletCreationHdBip39SeedError.tr();
_showCustomSeedToggle = true;
});
return;
}
if (!_allowCustomSeed) {
setState(() {
_commonError = LocaleKeys.walletCreationBip39SeedError.tr();
_showCustomSeedToggle = true;
});
return;
}
// Non-HD and custom seed allowed: continue without setting an error
} bool get _shouldShowCustomSeedToggle {
if (_allowCustomSeed) return true; // keep visible once enabled
if (_showCustomSeedToggle) return true; // show after first failure, even in HD
return false;
}
onChanged: (newValue) async {
if (!value && newValue) {
final confirmed = await customSeedDialog(context);
if (!confirmed) return;
}
onChanged(newValue);
},Minor nits (optional)
All else looks good and aligns with the test steps in the PR description. Learn more about Cursor Agents |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the wallet import flow to conditionally show the "custom seed" toggle only when BIP39 validation fails, improving UX by hiding unnecessary options when seeds are compliant.
Key changes:
- Custom seed toggle now appears dynamically based on BIP39 validation results
- Enhanced error handling for non-compliant seeds in file imports
- Added listener to rebuild UI as users type seed phrases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/views/wallets_manager/widgets/wallet_simple_import.dart | Adds dynamic validation logic to show/hide custom seed toggle based on BIP39 compliance during manual seed entry |
| lib/views/wallets_manager/widgets/wallet_import_by_file.dart | Implements conditional toggle visibility for seed file imports with improved error messaging for non-compliant seeds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CharlVS, I'll review the changes in this pull request now. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/views/wallets_manager/widgets/wallet_simple_import.dart (1)
151-160: Memory leak: Remove listener before disposing the controller.The listener added at line 154 is never removed in the
disposemethod. This will cause a memory leak as the listener persists after widget disposal.Apply this diff to fix:
@override void dispose() { + _seedController.removeListener(_onSeedChanged); _nameController.dispose(); _seedController.dispose(); _passwordController.dispose(); super.dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/views/wallets_manager/widgets/wallet_import_by_file.dart(5 hunks)lib/views/wallets_manager/widgets/wallet_simple_import.dart(3 hunks)
🔇 Additional comments (5)
lib/views/wallets_manager/widgets/wallet_simple_import.dart (2)
272-272: LGTM! Conditional visibility aligned with PR objectives.The toggle now correctly shows only when appropriate based on HD mode, custom seed state, and BIP39 validation.
436-446: LGTM! Logic correctly implements conditional visibility.The getter appropriately hides the toggle in HD mode, keeps it visible once enabled, and reveals it when BIP39 validation fails on non-empty seeds.
lib/views/wallets_manager/widgets/wallet_import_by_file.dart (3)
151-157: LGTM! Correctly resets custom seed when HD mode is enabled.This ensures custom seed usage is properly disabled when switching to HD mode, which doesn't support custom seeds.
220-225: LGTM! Good practice to clear previous errors.Clearing
_commonErrorbefore a new import attempt prevents stale error messages from confusing the user.
256-273: LGTM! BIP39 validation logic is clear and correct.The validation flow properly handles HD vs non-HD cases and sets appropriate errors. The comment on line 272 helpfully clarifies the success path.
|
Cursor Agent can help with this pull request. Just |
|
@cursoragent review |
|
I reviewed the PR. Functionality matches the acceptance criteria for both manual entry and seed-file import. Two small, actionable nits below. Must-fix
@override
void initState() {
super.initState();
_seedController.addListener(_onSeedChanged);
}
void _onSeedChanged() {
// Rebuild to update custom seed toggle visibility as user types
setState(() {});
} @override
void dispose() {
_nameController.dispose();
_seedController.dispose();
_passwordController.dispose();
super.dispose();
}Suggested edit: @override
void dispose() {
_seedController.removeListener(_onSeedChanged);
_nameController.dispose();
_seedController.dispose();
_passwordController.dispose();
super.dispose();
}Nice-to-have
bool get _shouldShowCustomSeedToggle {
if (_isHdMode) return false;
if (_allowCustomSeed) return true; // keep visible once enabled
final seed = _seedController.text.trim();
if (seed.isEmpty) return false;
final validator = context.read<KomodoDefiSdk>().mnemonicValidator;
final isBip39 = validator.validateBip39(seed);
return !isBip39;
}File import currently shows the toggle after failure, even if still in HD: bool get _shouldShowCustomSeedToggle {
if (_allowCustomSeed) return true; // keep visible once enabled
if (_showCustomSeedToggle) return true; // show after first failure, even in HD
return false;
}Optional alignment for file import: bool get _shouldShowCustomSeedToggle {
if (_isHdMode) return false;
if (_allowCustomSeed) return true;
if (_showCustomSeedToggle) return true;
return false;
}
"customSeedWarningText": "Custom seed phrases are generally less secure and easier to crack than a generated BIP39-compliant seed phrase. To confirm you understand and are aware of the risk, type \"I understand\" in the box below.",
"customSeedIUnderstand": "i understand",
"walletCreationBip39SeedError": "BIP39 seed phrase validation failed, try again or select 'Allow custom seed'",
"walletCreationHdBip39SeedError": "Your input seed phrase is not BIP39 compliant, and can not be used in multi-address wallet mode. Please try again, or disable multi-address wallet mode and select 'Allow custom seed' to proceed.",Suggestion: replace “Allow custom seed” with “Use custom seed.” All core flows look good. Learn more about Cursor Agents |
closes #3071
To test:
manual input:
use custom seedtoggle when input is non-compliantuse custom seedtoggle when input is compliantSeed file input:
Disable multi-address mode, and check custom seed and input
I understand.seed file is imported successfully
Log out, then log back into the imported wallet
Successful login.
Summary by CodeRabbit
Release Notes