Code simplification - #878
Merged
Merged
Conversation
Refactors `CoinControlManager` to store selected UTXOs and total in a single `SelectionState`, so reconcile updates apply atomically and UI state stays consistent. `selectedBinding` and public accessors now read/write through that state. Adds `CoinControlManagerTests` (and wires it into the Xcode project) to verify `toggleSelectAll` updates both selection count and total amount.
Run cloud-only and other-backup scans in the background so the primary inventory stays usable, and drop manual fetch, refresh, and close-detail actions for those checks.
Hide cloud-only and other-backup UI until content is ready, and give iCloud metadata listing more time before timing out.
Replace secret-bearing Debug output with redacted forms and log only onboarding variant names. Treat duplicate save actions as idempotent so rapid taps cannot reach the invalid transition path. Thanks @Jacob-bit-ux for reporting this.
Delete UniFFI exports and internal functions with no callers in Rust, Swift, or Kotlin, plus an orphan module that was never compiled. Merge duplicated helpers (timestamps, fiat currency lookup, TapSigner birthday, transaction labels, wallet backup lookup) and inline passthrough wrappers. Route the 44 hand-written stale-claim guards in the cloud backup supervisor through one predicate on the operation owner. Apply the AGENTS.md rules: `map_err_str`/`map_err_prefix` instead of closures, `pub(crate)` instead of `pub(super)`, and plain assignment instead of `drop(mem::replace(..))`. Fix redundant clones, closures, and never-failing `Result` wrappers, then enforce those three lints in `just lint-rust` instead of allowing them crate-wide. Add `BackupError::user_message` so both apps show the same safe wording without inner error payloads, and regenerate the bindings. Payjoin is left untouched on purpose; its send-initiation path is unfinished feature work.
Collapse the 33 identical stale-completion supervisor tests into one helper that still drives every real handler, delete the shadow `cove_tokio::init` shim and its 239 redundant calls, and drop the unused passkey presence queue from the mock provider. Merge the four cove-device mock keychains into one with explicit failure controls, turn copy-pasted cleanup tests into a table, and delete a test that only exercised its own test-only helper.
Delete unreferenced files, extensions, colour aliases, and orphaned view state. Drop the AmountFormatter layer and its tautological test so WalletManager calls the Rust formatting functions directly, and use the Rust unit formatter and sensitive-value mask instead of hand-rolled switches. Share one timestamp formatter across the settings screens and read backup error text from `BackupError.userMessage()`.
Delete unused composables, the deprecated SettingsItem and CustomSpacer kept alive only by their previews, the RouteHelpers object, and dead manager methods. Hoist the triplicated privateShow helper, use the Rust unit formatter instead of hand-rolled switches, and replace the three copied backup error tables with `BackupException.userMessage()`.
Read the fiat currency once per transaction-details method and pass it into the two format helpers instead of reading redb twice per value. Move the selected-currency accessor into the exported impl, share the non-empty label helper with `label()`, and replace three private timestamp wrappers with one zero-default helper. Return plain values from `chain_update` and `start_broadcast_transaction` instead of `Result` wrappers that never failed. Declare the three enforced clippy lints as workspace lints so CI and rust-analyzer see them, and drop the justfile flags. Use async closures in the supervisor stale-claim tests, match directly on the cleanup wait condition, flatten the onboarding navigation table, and remove Kotlin imports orphaned by the earlier deletions.
Delete the `AwaitingForceNewConfirmation` pending-enable session that no production path ever staged, along with its resume helper, the upload selection enum that only existed to include it, and the tests that staged it by hand. Replace the check-then-take pattern in the persisted cloud backup state with a single match so the four `unreachable!()` calls go away. Take owned bytes in the NFC parser instead of a trait whose impls were identities, drop two duplicated early returns in the fiat input handler, and model "clear the max selection" as a bool instead of `Option<Option<Amount>>` with an arm that could never fire.
Give the cloud backup supervisor one `current(claim)` step that pairs the stale-claim check with the manager upgrade and clears the operation when the manager is gone, replacing the same seven lines in 36 completion handlers. Route every manager's `listen_for_updates` through a shared `ReconcileSink` so the channel owns the single-or-many dispatch. Add a `wallet_target()` accessor for the network and mode pair that 14 sites read separately, share the startup network backoff between the fiat and fee clients, and format fiat amounts with a two-decimal `format!` instead of repairing the formatter's output; fiat display now rounds instead of truncating.
Derive the restore marker guard's cleanup state from the persisted marker phase and record the outcome once instead of four booleans. Give the detail refresh coordinator a phase enum so open, in-flight, trailing, and timer states cannot disagree. Use `OnceCell` for the lazily read write blocker, and drop the outer `Option` from the internal metadata patch since no writer ever cleared a field. Stop threading a `&mut bool` and an optional attempt id through the onboarding transition function: the restore offer is derived from the resulting command, and attempt ids are allocated unconditionally. Stamp preview wallets with the real time instead of a `u64::MAX` sentinel.
Give registrations, preparation reservations, and ordinary-close retries their own id newtypes handed out by one wrapping counter, so the deletion coordinator cannot mix the three kinds of `u64`.
Break the 209-line restore function into discovery, listing, download, apply, and status resolution. The apply loop now returns a `RestoreResolution` instead of three accumulators that the final status match had to reinterpret.
Format the bitcoin and fiat entry text and compute the fiat value in three helpers instead of repeating them across the amount, focus, unit, and currency handlers, and use a scoped block instead of an explicit `drop` when reading the entered amount.
`new_test_wallet_actor` opens the wallet database under the shared wallet-data directory, and six actor tests called it without the global test lock. Tests holding the lock wipe and recreate that directory, so a concurrent unlocked test could make the recreate fail or lose its own files. Take the lock in those tests.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
Greptile SummaryThis PR broadly simplifies shared Rust and native mobile code while preserving wallet, backup, onboarding, transaction, and formatting behavior. Since the previous review, it also:
Confidence Score: 5/5The PR appears safe to merge, with the previously reported hidden inventory failures now visibly handled and no new actionable defects identified. The resolved inventory-failure finding is addressed on both platforms with explicit failure sections and retry actions, and the Android bootstrap discovery runs asynchronously through guarded inventory workflows. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Application bootstrap completes] --> B[Sync persisted cloud state]
B --> C[Start background inventory discovery]
C --> D[Load primary backup detail]
D --> E[Discover cloud-only wallets]
D --> F[Discover backups using another key]
E --> G{Inventory result}
F --> G
G -->|Loaded and non-empty| H[Display supplemental wallet sections]
G -->|Failed| I[Display failure and Check Again]
I --> D
Reviews (2): Last reviewed commit: "Sync cloud backup state after bootstrap" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f43ebd71d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Failed cloud-only and other-key inventory loads were easy to miss because only loaded rows rendered. Surface those failures with a retry action on iOS and Android, and keep the visibility helpers shared and tested.
Trigger `cloudBackupManager.syncPersistedState()` once async bootstrap completes in `MainActivity` so returning users discover cloud-only and existing backups before opening Cloud Backup settings. Update Android process-death UI coverage to assert silent wallet inventory discovery runs during bootstrap. In onboarding Rust tests, switch unexpected-state panics to `flow.kind()` and expose `FlowState::kind()` as `pub(crate)` to keep diagnostics useful without leaking sensitive state details, while strengthening redaction assertions across additional debug surfaces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.