Add externalId to Wallet and migrate wallet IDs#1623
Conversation
Introduces an externalId field to the Wallet model and related records, updates all usages to support the new identifier, and adds a migration to convert existing wallet IDs to a new WalletIdentifier format. Also updates child table references and current wallet preference accordingly. AssetMarket and PriceRecord are updated to support new all-time high/low percentage fields.
Summary of ChangesHello @gemcoder21, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how wallets are identified and managed within the system by introducing an Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant change by adding an externalId to the Wallet model and migrating existing wallet IDs to a new deterministic format. The changes span across data models, keystore interactions, and database migrations. While the overall approach is sound, I've identified some critical issues in the database migration script (WalletIdMigration.swift) that could lead to data inconsistency or incomplete migration. Specifically, there's a non-deterministic query and a case where wallets could be silently skipped during migration. I've also noted an issue with error handling in the migration registration. Please review the detailed comments.
| (SELECT a.address FROM \(AccountRecord.databaseTableName) a | ||
| WHERE a.walletId = w.id LIMIT 1) as address, | ||
| (SELECT a.chain FROM \(AccountRecord.databaseTableName) a | ||
| WHERE a.walletId = w.id LIMIT 1) as chain |
There was a problem hiding this comment.
The subqueries to fetch address and chain for single-chain wallets use LIMIT 1 without an ORDER BY clause. This can lead to two critical issues:
- Non-determinism: The database can return any account if multiple exist for a wallet, leading to inconsistent
newIdgeneration across different migration runs. - Inconsistency: The
addressandchaincould be fetched from two different account records, resulting in an invalid combination (e.g., a Bitcoin chain with an Ethereum address).
To fix this, you should add a deterministic ORDER BY clause to both subqueries to ensure you consistently pick the same account for both address and chain.
| (SELECT a.address FROM \(AccountRecord.databaseTableName) a | |
| WHERE a.walletId = w.id LIMIT 1) as address, | |
| (SELECT a.chain FROM \(AccountRecord.databaseTableName) a | |
| WHERE a.walletId = w.id LIMIT 1) as chain | |
| (SELECT a.address FROM \(AccountRecord.databaseTableName) a | |
| WHERE a.walletId = w.id ORDER BY a.chain, a.address LIMIT 1) as address, | |
| (SELECT a.chain FROM \(AccountRecord.databaseTableName) a | |
| WHERE a.walletId = w.id ORDER BY a.chain, a.address LIMIT 1) as chain |
| try? db.alter(table: WalletRecord.databaseTableName) { | ||
| $0.add(column: WalletRecord.Columns.externalId.name, .text) | ||
| } |
There was a problem hiding this comment.
The use of try? for the database alteration can suppress important errors. If adding the externalId column fails for any reason (e.g., disk space, permissions), the error will be ignored. The subsequent migration step, which depends on this column, will then fail, potentially leaving the database in an inconsistent state. It's safer to let the error propagate by using try so the entire migration transaction is rolled back.
| try? db.alter(table: WalletRecord.databaseTableName) { | |
| $0.add(column: WalletRecord.Columns.externalId.name, .text) | |
| } | |
| try db.alter(table: WalletRecord.databaseTableName) { | |
| $0.add(column: WalletRecord.Columns.externalId.name, .text) | |
| } |
| let newId: String | ||
| switch type { | ||
| case .multicoin: | ||
| guard let address: String = row["ethereumAddress"] else { return nil } |
There was a problem hiding this comment.
For .multicoin wallets, the migration relies on finding an Ethereum address to generate the new ID. If a multicoin wallet exists but does not have an Ethereum account, row["ethereumAddress"] will be nil. This causes the guard to fail, and compactMap will silently discard this wallet from the migration process. The wallet will not be migrated, which could lead to it being unusable or causing data inconsistencies later. The migration should handle this case more gracefully, for example by logging a warning and assigning a temporary ID, or by throwing an error to halt the migration until the issue is resolved. Silently skipping wallets is dangerous.
Introduces a keystoreId property to the Wallet extension, returning externalId if available, otherwise falling back to id.
Introduces comprehensive tests for WalletIdMigration covering various wallet types, duplicate handling, child table updates, and preference migration. Updates Wallet.mock to support externalId and refines migration logic to temporarily disable foreign key constraints during ID updates.
Registers migrations to add an externalId column to the wallet table and to migrate wallet IDs to the WalletIdentifier format.
WalletIdMigration now takes a UserDefaults instance for preference migration, improving testability and flexibility. Tests updated to use a mock UserDefaults, and WalletStore gains a setOrder method for setting wallet order in tests.
Replaces wallet type-based ID logic with WalletIdentifier throughout keystore and primitives. Updates import and migration flows to use the new identifier format, ensuring consistent wallet identification. Adjusts tests and migration logic to support the new ID scheme and migrates wallet preferences accordingly.
Reordered and updated the list of test targets in unit_frameworks.xctestplan to reflect current project structure. Added, removed, and rearranged test targets for improved test coverage and organization.
Adds a check to return early from the migrate function if no wallet mappings are found, preventing unnecessary execution of migration steps.
Updated notification services and store to pass notifications directly without mapping wallet IDs, simplifying the API and reducing unnecessary transformations. Adjusted related methods and models to accommodate this change.
1 participant
Introduces an externalId field to the Wallet model and related records, updates all usages to support the new identifier, and adds a migration to convert existing wallet IDs to a new WalletIdentifier format. Also updates child table references and current wallet preference accordingly. AssetMarket and PriceRecord are updated to support new all-time high/low percentage fields.