Skip to content

feat(wallet): validate wallet name and allow import renaming#2792

Merged
CharlVS merged 21 commits intodevfrom
codex/add-wallet-name-validation
Sep 25, 2025
Merged

feat(wallet): validate wallet name and allow import renaming#2792
CharlVS merged 21 commits intodevfrom
codex/add-wallet-name-validation

Conversation

@CharlVS
Copy link
Copy Markdown
Collaborator

@CharlVS CharlVS commented Jun 17, 2025

This pull request introduces functionality to validate and rename wallet names across multiple areas of the application. It adds a new dialog for renaming wallets, integrates wallet name validation logic, and updates relevant workflows to handle invalid wallet names gracefully.

This bug stems from the newly implemented wallet storage feature in KDF which restricts wallet name characters on native platforms. This would cause issues if user is logging into or importing a wallet from an older version of the app which no longer meets the wallet name requirements.

Wallet Name Validation Enhancements:

Wallet Rename Dialog Implementation:

Integration with Wallet Workflows:

Summary by CodeRabbit

  • New Features
    • Wallets with invalid/legacy names now prompt for rename before access or login; canceling stops the action.
    • Wallet import validates names and requests a new one if invalid or taken; canceling aborts import.
    • Introduced a rename dialog with inline validation, 40-character limit, and clear error messaging.
    • Added localized English strings for name validation and rename flow (labels and guidance).

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jun 17, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Adds wallet-name validation and rename prompt across wallet handling and import flows, introduces a reusable wallet rename dialog, and adds related English translation strings.

Changes

Cohort / File(s) Summary of Changes
Translations
assets/translations/en.json
Added three keys: walletCreationNameCharactersError, renameWalletDescription, renameWalletConfirm.
Name validation and rename flow
lib/views/wallets_manager/widgets/iguana_wallets_manager.dart, lib/views/wallets_manager/widgets/wallet_import_by_file.dart
Integrated pre-login/import validation via WalletsRepository.validateWalletName; on error, opens rename dialog; aborts if canceled; updates wallet name when confirmed.
New dialog component
lib/views/wallets_manager/widgets/wallet_rename_dialog.dart
Added walletRenameDialog(...) returning Future<String?>; includes inline validation, 40-char limit, localized labels, and guarded enablement of Rename action.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant WM as Wallet Flow (Manager/Import)
  participant WR as WalletsRepository
  participant RD as Rename Dialog

  U->>WM: Start open/import wallet
  WM->>WR: validateWalletName(proposedName)
  WR-->>WM: error or ok

  alt Name invalid/in use
    WM->>RD: walletRenameDialog(initialName)
    RD-->>WM: newName or null
    opt If newName provided
      WM->>WR: validateWalletName(newName)
      WR-->>WM: ok
      WM->>WM: Apply newName
    end
    opt If canceled (null)
      WM->>U: Abort operation
    end
  else Name valid
    WM->>WM: Proceed with flow
  end

  Note over WM: If proceeding, continue to login/import logic
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • Fix/illegal import chars #2966 — Adjusts wallet-name validation and related translations, overlapping with import/creation flows and repository validation usage.

Suggested reviewers

  • smk762
  • naezith

Poem

A hop, a tap, a tidy name,
I nibble bugs and tame the frame.
If names go wild, I prompt—“Rename!”
With gentle paws I guard the same.
Thump-thump—approved! Now onward, game. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat(wallet): validate wallet name and allow import renaming" concisely and accurately summarizes the primary change in the changeset — adding wallet name validation and supporting renaming during import — and aligns with the PR objectives and modified files (validation logic, translations, rename dialog, and integration points). It is specific, actionable, and suitable for a teammate scanning the repository history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 17, 2025

Visit the preview URL for this PR (updated for commit e162ca3):

https://walletrc--pull-2792-merge-ayliimy4.web.app

(expires Thu, 02 Oct 2025 15:50:11 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc

@smk762
Copy link
Copy Markdown
Collaborator

smk762 commented Jun 20, 2025

As with the weak passwords, we need to ensure that older wallets which may include these chars are not affected. The only way to really do this is via a settings config param.

Does this affect diacritical marks or non-latin chars (e.g. Cyrillic, Coptic, Runes, Cuneiform etc)? I am certain some users do use their native script for wallet names.

@CharlVS
Copy link
Copy Markdown
Collaborator Author

CharlVS commented Jun 23, 2025

we need to ensure that older wallets which may include these chars are not affected

@smk762 We should be fine to enforce this only for new wallets. KDF does not enforce the wallet name policy for wasm, and currently, we do not have a production-deployed version of the native app (users will have to import seed manually). Is native desktop seed file export cross-compatible with KW web seed import?

@smk762
Copy link
Copy Markdown
Collaborator

smk762 commented Jul 1, 2025

Is native desktop seed file export cross-compatible with KW web seed import?

It is not. For reference, the legacy desktop necrypt / decrypt code is in https://github.com/KomodoPlatform/komodo-wallet-desktop/blob/dev/src/core/atomicdex/utilities/security.utilities.cpp#L74-L139

@CharlVS CharlVS added this to the v0.9.2 Release milestone Jul 8, 2025
@CharlVS

This comment was marked as off-topic.

@CharlVS CharlVS self-assigned this Jul 8, 2025
@CharlVS CharlVS added the QA Ready for QA Testing label Jul 8, 2025
@CharlVS CharlVS changed the title feat(wallet): restrict wallet name characters feat(wallet): validate wallet name and allow import renaming Jul 8, 2025
@CharlVS CharlVS mentioned this pull request Jul 21, 2025
4 tasks
@smk762 smk762 linked an issue Jul 22, 2025 that may be closed by this pull request
@CharlVS CharlVS assigned takenagain and unassigned CharlVS Aug 11, 2025
@CharlVS
Copy link
Copy Markdown
Collaborator Author

CharlVS commented Aug 11, 2025

@takenagain, please take this over and see it through to completion. If it needs anything beyond minor tweaks, please close this PR and create your own.

@takenagain
Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 22, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cursor[bot]

This comment was marked as outdated.

@smk762
Copy link
Copy Markdown
Collaborator

smk762 commented Sep 24, 2025

Copilot imples Implemented a wallet rename dialog with inline validation and error feedback, but I do not see this or anything to click which might spawn it.

image
  • Wallet name on creation of new allows only letters, numbers, underscores, and hyphens.

fixes "setState() or markNeedsBuild() called when widget tree was locked. This ScreenshotSensitivity widget cannot be marked as needing to build because the framework is locked."  and "DartError: setState() or markNeedsBuild() called during build.
This ScreenshotSensitivity widget cannot be marked as needing to build because the framework is already in the process of building widgets. A widget can be marked as needing to be built during the build phase only if one of its ancestors is currently building. This exception is allowed because the framework builds parent widgets before children, which means a dirty descendant will always be built. Otherwise, the framework might not visit this widget during this build phase."
@smk762
Copy link
Copy Markdown
Collaborator

smk762 commented Sep 24, 2025

  • on seed file import, if filename has bad chars, rename modal appears when clicking import button
  • rename modal applies wallet already exists validation
  • rename modal applies bad char validation
  • import cancellation functions as expected

Final issue I can see: When renaming wallet, though validation triggers on exact match, a trailing space passes. We should avoid allowing leading or trailing spaces in wallet names as it could lead to user input fails and confusion.

After import of the same name wallet (with extra space), and logging out, there was only instance of the wallet name - so I assume trimming is applied before saving - and if so, this may result in an overwrite of the existing wallet. This bug also exists in the create/import wallet form's wallet name input field.

Request that validation in rename form and create/import wallet form be performed with trimmed input.

vokoscreenNG-2025-09-24_22-45-48.mp4

@CharlVS
Copy link
Copy Markdown
Collaborator Author

CharlVS commented Sep 24, 2025

@cursor find all bugs introduced in this PR.

@cursor
Copy link
Copy Markdown

cursor bot commented Sep 24, 2025

I've thoroughly reviewed the PR context, including the description, comments, and the code changes themselves. Based on this, I've identified several bugs and potential issues introduced by this pull request:

  • Trimmed vs. Untrimmed Name Mismatch: The validation logic checks for duplicate wallet names using the untrimmed input, but the actual name saved or returned is trimmed. This inconsistency can lead to situations where a user can create a wallet named "Alice " even if "Alice" already exists, and both might resolve to "Alice" after trimming, causing potential overwrites or confusion. This affects wallet creation, simple import, and the rename dialog.
  • Duplicate Check Ignores Legacy Wallets: The _cachedWallets list, used for checking duplicate names, only contains SDK wallets and excludes legacy wallets. This means a user could rename or create a new wallet with a name that already exists as a legacy wallet, leading to conflicts.
  • Silent Duplicate Validation Skip: On the first run, if the _cachedWallets list hasn't been loaded yet, the validation process might asynchronously fetch wallets but immediately return null for the validation error, effectively skipping the duplicate check.
  • In-Memory Legacy Wallet Rename: When a legacy wallet is renamed, the wallet.name property is updated in memory but this change is not persisted to the underlying storage. This means the name will revert or diverge from the stored data on subsequent access.
  • Missing Re-validation After Rename (TOCTOU): After a user renames a wallet through the dialog, there's no re-validation step. This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability where the chosen name could become taken by another process or user while the dialog was open, leading to unexpected behavior.
  • BuildContext Usage After Awaits: In the wallet import flow, BuildContext is used after await calls without proper mounted checks, and linting suppressions are used instead of fixing the underlying issue. This can lead to exceptions if the widget is disposed before the await completes.
  • Inconsistent Validation Rule in UI Copy: The actual implementation of wallet name validation allows letters, combining marks, numbers, whitespace, underscores, and hyphens. However, the UI text (e.g., walletCreationNameCharactersError) states that names can only contain "letters, numbers, underscores, and hyphens," which is misleading as it omits spaces and non-Latin characters.
  • Rename Dialog Initially Enabled Due to Lazy Cache: If the _cachedWallets are not yet loaded when the rename dialog appears, the initial validation might incorrectly show no error, enabling the "Rename" button even if the name is actually a duplicate.

Additionally, I noted an optional heads-up regarding the parsing of wallet names from file data using split('.').first, which can truncate names unexpectedly and increase collision risk, especially when combined with the other validation issues.


Open in Cursor Open in Web

Learn more about Cursor Agents

Co-authored-by: charl <charl@vanstaden.info>
@CharlVS
Copy link
Copy Markdown
Collaborator Author

CharlVS commented Sep 24, 2025

@smk762, please test the fixes for the issues mentioned in #2792 (comment) to the extent you reasonably can

Copy link
Copy Markdown
Collaborator

@smk762 smk762 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Trimmed vs. Untrimmed Name Mismatch trims as expected

  • Inconsistent Validation Rule in UI Copy Pedantic technicality. IMO, fine as is.

  • using split('.').first, which can truncate names unexpectedly and increase collision risk - with . not allowed, this split should not truncate.

  • Missing Re-validation After Rename highly unlikely another user/process will modify the specific file in question.

  • Duplicate Check Ignores Legacy Wallets, Silent Duplicate Validation Skip unable to test in web, as CI url holds no cache. Linux desktop had more prior imported wallets, and none of the older ones had an issues with being ignored with duplicate checks.

  • BuildContext Usage After Awaits: In the wallet import flow, BuildContext is used after await calls without proper mounted checks, and linting suppressions are used instead of fixing the underlying issue. This can lead to exceptions if the widget is disposed before the await completes. Not sure how to confirm/test this, seems to be more of a code review thing. If there is a test case to cover this, please let me know.

Additionally I tested re-importation of a seed file without renaming, after deleting wallet from login menu. Confirmed that this does not trigger a rename, and imports as usual, without needing to restart app.

Approving as fit for purpose, with caveats to perform further testing and/or delegate to issues with any remaining minor niggles.

cursor[bot]

This comment was marked as outdated.

…ache warm-ups

- Remove cache warm-ups in wallet import and rename dialog
- Keep validation using fresh repository fetches
- Reverted unintended global formatting; formatted only edited files
- No Bloc introduced (repo methods suffice)
cursor[bot]

This comment was marked as outdated.

TODO: More fixes of the same warning type (stale context)
…\n\n- Replace wallet.name assignment with copyWith + setState in iguana manager\n- Use wallet.copy() to avoid shared references during login flow\n- Avoid widget.wallet.config.type mutation; pass copied wallet to onLogin\n- Avoid mutating wallet.config.seedPhrase in downloadEncryptedWallet; use working copy\n\nKeeps immutability and consistent state updates in BLoC flows.
… rename

- Combine validation and uniqueness checks before showing rename dialog
- Remove duplicate uniqueness check after validation
- Improves flow by avoiding unnecessary prompts when both checks fail
@CharlVS CharlVS merged commit fc1fe61 into dev Sep 25, 2025
10 of 16 checks passed
@CharlVS CharlVS deleted the codex/add-wallet-name-validation branch September 25, 2025 15:53
@CharlVS CharlVS mentioned this pull request Oct 5, 2025
@coderabbitai coderabbitai bot mentioned this pull request Oct 22, 2025
@smk762 smk762 mentioned this pull request Nov 23, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex QA Ready for QA Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow for renaming wallets

5 participants