fix(auth): missing login failed error and BIP39 validation clarification#2554
fix(auth): missing login failed error and BIP39 validation clarification#2554
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 WalkthroughThis pull request adds a new error message entry for wallet creation in the translations file and refines authentication state management. The AuthBloc now handles additional events and states, including clear state, loading, error, and logged-in statuses. UI components have been updated for wallet login and password validation with enhanced asynchronous handling and dynamic error feedback. Additionally, form validation logic in the wallet import view has been centralized, and text input error processing has been improved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletLogIn
participant AuthBloc
participant AuthService
User->>WalletLogIn: Enter login credentials
WalletLogIn->>AuthBloc: Trigger _submitLogin event
AuthBloc->>AuthService: Authenticate credentials
AuthService-->>AuthBloc: Return currentUser or error
alt Authentication successful
AuthBloc->>WalletLogIn: Emit loggedIn state
else Authentication failed
AuthBloc->>WalletLogIn: Emit error state
end
sequenceDiagram
participant User
participant IguanaWalletsManager
participant AuthBloc
User->>IguanaWalletsManager: Trigger cancel action
IguanaWalletsManager->>AuthBloc: Dispatch AuthStateClearRequested event
AuthBloc->>AuthBloc: Execute _onClearState (cancel subscriptions, reset state)
AuthBloc->>IguanaWalletsManager: Emit initial state
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 233de55): https://walletrc--pull-2554-merge-nc6nuna5.web.app (expires Tue, 11 Mar 2025 16:32:18 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@coderabbitai review |
✅ 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: 2
🧹 Nitpick comments (4)
lib/views/common/wallet_password_dialog/password_dialog_content.dart (1)
111-120: Consider catching specific exceptionsThe
catch (_)block is very broad. Consider catching specific exceptions related to authentication failures to better handle different error scenarios.- try { - final seed = await sdk.auth.getMnemonicPlainText(password); - if (seed.plaintextMnemonic?.isEmpty ?? true) { - _setInvalidPasswordState(); - return; - } - } catch (_) { - _setInvalidPasswordState(); - return; - } + try { + final seed = await sdk.auth.getMnemonicPlainText(password); + if (seed.plaintextMnemonic?.isEmpty ?? true) { + _setInvalidPasswordState(); + return; + } + } catch (e) { + if (e is AuthenticationException || e is InvalidPasswordException) { + _setInvalidPasswordState(); + } else { + setState(() { + _error = LocaleKeys.genericError.tr(); + _inProgress = false; + }); + } + return; + }lib/views/wallets_manager/widgets/wallet_login.dart (3)
64-64: Async return on_submitLogin().
Even though noawaitis used, returning aFuture<void>can be helpful if you plan to expand async functionality. Otherwise, you could remove the async marker.
80-85: Centralized error message usage.
Using a singularLocaleKeys.invalidPasswordError.tr()for all auth errors is consistent but might be limiting if you need different error messages. Keep this in mind if more detailed error handling is required in the future.
111-112: Conditional rendering for BIP39 seeds.
Suggestion: use a more concise null check likeif (_user?.isBip39Seed == true)instead of_user != null && _user!.isBip39Seed == true.- if (_user != null && _user!.isBip39Seed == true) + if (_user?.isBip39Seed == true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (9)
assets/translations/en.json(1 hunks)lib/bloc/auth_bloc/auth_bloc.dart(11 hunks)lib/bloc/auth_bloc/auth_bloc_event.dart(1 hunks)lib/bloc/auth_bloc/auth_bloc_state.dart(1 hunks)lib/views/common/wallet_password_dialog/password_dialog_content.dart(3 hunks)lib/views/wallets_manager/widgets/iguana_wallets_manager.dart(1 hunks)lib/views/wallets_manager/widgets/wallet_login.dart(4 hunks)lib/views/wallets_manager/widgets/wallet_simple_import.dart(5 hunks)packages/komodo_ui_kit/lib/src/inputs/ui_text_form_field.dart(1 hunks)
🔇 Additional comments (56)
lib/bloc/auth_bloc/auth_bloc_event.dart (1)
14-16: Well-structured addition to auth event hierarchyThe addition of
AuthStateClearRequestedfollows the established pattern in the codebase and enables proper clearing of authentication state. This will help manage auth state lifecycle more effectively.packages/komodo_ui_kit/lib/src/inputs/ui_text_form_field.dart (1)
133-134: Great improvement to error validation handlingThis change enhances the form validation by dynamically running the validator on the current text when the widget updates, rather than just comparing static error texts. This makes the form field more responsive to validation changes.
The field will now properly display validation errors even when the widget updates from external state changes, improving the user experience with faster feedback on input errors.
assets/translations/en.json (1)
204-204: Clear and helpful error message for BIP39 complianceThis new error message provides excellent guidance for users when their seed doesn't comply with BIP39 standards for multi-address wallet mode. It explains both the issue and offers alternative solutions.
The message clearly addresses the previously missing error scenario mentioned in the PR objectives, specifically for cases where BIP39 validation fails in multi-address wallet context.
lib/views/wallets_manager/widgets/iguana_wallets_manager.dart (1)
186-186: Improves state management with centralized auth state clearingAdding this event dispatch ensures the auth state is properly cleared when cancelling operations, addressing the issue where error messages weren't being cleared properly.
This change directly addresses the PR objective of fixing "disappearing widgets" by ensuring the UI state is properly reset when operations are cancelled.
lib/views/wallets_manager/widgets/wallet_simple_import.dart (7)
62-66: Improved form validation logicThe refactoring to use
_refreshFormValidationState()enhances code readability and maintainability by centralizing form validation logic.
133-134: Improved validation flowThe custom seed checkbox now properly triggers form validation when its state changes.
138-147: Good abstraction of validation logicThis new method centralizes form validation logic, making it more maintainable and easier to understand. The approach of checking input presence before triggering full validation is efficient.
199-205: Enhanced state handling for HD wallet modeThe changes properly reset custom seed when switching HD mode and trigger form validation, ensuring a consistent user experience.
310-314: Improved seed validationThe early return when custom seed is allowed simplifies the validation logic flow.
331-333: Improved error messaging based on wallet typeThe error message now correctly differentiates between HD and non-HD wallet modes, providing more specific guidance to the user.
340-342: Consistent error handling for invalid seed lengthsError messages are now contextual to the wallet type, matching the approach used for other validation errors.
lib/bloc/auth_bloc/auth_bloc.dart (9)
24-24: Added state clearing capabilityAdded event handler for clearing authentication state, which is essential for proper error state management.
33-33: More descriptive naming of subscription variableRenamed
_authorizationSubscriptionto_authChangesSubscriptionfor better clarity.
45-50: Improved logout flow with loading stateThe logout process now properly emits a loading state and transitions to the initial state after completion, providing better user feedback during the process.
67-85: Enhanced login flow with proper error handlingThe login flow now correctly checks if a user was successfully signed in and provides appropriate error feedback. The loading state is also properly emitted.
105-111: Well-structured state clearing methodThe
_onClearStatemethod properly handles subscription cleanup and state reset.
117-121: Code reuse through extracted methodExtracting the wallet existence check into
_didSignInExistingWalletimproves code reusability and readability.
141-146: Improved error handling in registration flowThe registration now properly checks if a user is signed in after registration and provides appropriate error feedback.
164-167: Code reuse through extracted methodReusing the
_didSignInExistingWalletmethod for both register and restore flows ensures consistent behavior.
214-228: Well-designed helper method for wallet existence checkThis extracted method encapsulates the logic for checking if a wallet already exists and initiating the sign-in process, improving code organization and readability.
lib/views/common/wallet_password_dialog/password_dialog_content.dart (6)
4-5: Added SDK dependency for seed phrase verificationAdding the KomodoDefiSdk import is necessary for proper password validation.
13-14: Good use of TODO for future improvementThis TODO comment clearly indicates the need for refactoring this widget to use a dedicated bloc for seed access attempts.
16-21: Updated constructor to use super.keyUsing
super.keyfollows modern Flutter coding conventions.
23-23: Improved function type definitionChanged from
Function(String)to the more specificvoid Function(String)for better type safety.
110-121: Fixed missing error handling for invalid passwordsAdded proper error handling for password validation, which addresses the issue where no error was shown for invalid passwords.
This change fixes issue #2548 mentioned in the PR objectives, which was about the lack of error messages for incorrect passwords in the backup seed dialog.
128-133: Good extraction of error state handlingThe extracted
_setInvalidPasswordStatemethod improves code reusability and makes the intent clear.lib/bloc/auth_bloc/auth_bloc_state.dart (6)
3-4: Well-designed AuthStatus enumThis enum provides a clear set of states for tracking authentication progress and outcomes, making the state management more robust.
6-11: Enhanced state model with status and error trackingThe modified constructor now includes status tracking and error message capability, which is essential for providing proper feedback to users.
15-28: Well-structured factory constructorsThese factory constructors provide a clean, semantic way to create different authentication states, improving code readability and maintainability.
32-33: Added essential state tracking fieldsThe new status and errorMessage fields allow for better tracking of the authentication process and provide detailed feedback.
36-37: Convenient state checking gettersThese getters provide a clean way to check for loading and error states, improving readability throughout the codebase.
40-40: Updated props for proper state equalityThe props list now includes all state fields, ensuring proper equality comparisons and state transitions.
lib/views/wallets_manager/widgets/wallet_login.dart (24)
1-2: Confirmeddart:asyncimport usage.
This import is necessary for usingunawaited(...). No issues found.
10-10: AuthBloc import looks good.
You are correctly integrating the AuthBloc for handling authentication states.
22-23: Constructor update is appropriate.
Passingsuper.keyin the constructor is a standard best practice.
42-42: Consider handling silent errors for unawaited calls.
unawaited(_fetchKdfUser())may mask potential errors if it fails. Consider wrapping it in a try-catch or providing logging to track silent failures.
86-88: Column layout with dynamic mainAxisSize.
No issues found. The ternary structure forisMobileis a typical responsive approach.
89-95: Display wallet login title.
Clean usage of translations for theTextwidget.
96-96: Spacing usage.
TheSizedBoxfor vertical spacing is straightforward and fine.
97-102: Read-only wallet name field.
Good approach to prevent unintended edits to the wallet name.
103-104: Additional spacing block.
The spacing blocks enhance visual structure.
106-110: Adoption of aPasswordTextField.
Passingstate.isLoading ? null : _submitLoginensures proper disable behavior when loading.
113-119: Toggle HD wallet mode.
The usage ofHDWalletModeSwitchis logical, ensuring user control over wallet type.
120-133: Login button with loading state.
Disabling the button while loading prevents accidental duplicate submissions. No issues.
134-140: Back/cancel button addition.
Allows end-users to return or cancel at any point. This improves UX.
141-142: Column closure.
No concerns; structure is adequately closed.
145-145: Widget class closure.
No issues detected.
148-153: NewPasswordTextFieldconstructor.
Fields are clearly defined, with an optionalerrorText.
155-158: Essential fields for password handling.
These fields enable flexible integration with bloc states.
159-161: Stateful construction for password text field.
Best practice for controlling internal UI states such asobscuredstate.
163-165: Internal password toggle initialization.
Defaulting_isPasswordObscuredto true is standard for security.
166-166: Override ofbuild()is well-structured.
Nothing concerning found here.
174-174: Controller binding.
Matches the standard Flutter text field pattern.
176-176: Error text binding.
Enabling consistent error messaging is good.
179-179: Visibility control connection.
Links thePasswordVisibilityControlcallback properly.
186-194: Visibility toggling logic.
Looks straightforward. ThesetStateusage is appropriate for updating_isPasswordObscured.
takenagain
requested review from
AndrewDelaney,
CharlVS,
ShantanuSharma9873 and
smk762
takenagain
changed the title
ShantanuSharma9873
left a comment
ShantanuSharma9873
left a comment
There was a problem hiding this comment.
-
The error message is now properly displayed on both the 'Backup Seed Phrase' window and the Login page, but there is a delay of approximately 15-17 seconds on the Login page to display the error message.
Attachment:
https://github.com/user-attachments/assets/106c07fb-c80b-4a5b-9c17-10b77d550c89 -
When the 'Multi Address Wallet' toggle is ON, the error message changes accordingly, providing proper guidance that the input field is not BIP39 compliant.
Observation:
In the 'Allow Seed Phrase' window, the placeholder text 'I Understand' is missing.
@takenagain It'd be better to leave the placeholder/hint empty as you've done. The objective of the text box is as a "test" that they've thoroughly read the disclaimer. While hints improve UX, it defeats the purpose of this confirmation. If the slow authentication time isn't a regression caused by this PR and is solely an SDK issue, lmk plz. |
The slow authentication failure time is an SDK issue. There's a timeout in kdf_operations_wasm.dart that we could possibly improve or adjust to a lower value. The timeout was used to catch failed startups, seeing as the WASM implementation did not have an early exit/failure equivalent to |
3 participants
Fixes the following issues from #2547, #2548, and the backup seed password input issue mentioned in #2553:
Summary by CodeRabbit
New Features
Refactor