Conversation
- Add MASTER_SEED_LABEL ("master_") and LEGACY_LIQUID_SEED_LABEL ("liquid_")
constants alongside MNEMONICS_FOLDER_NAME for labelling mnemonic files.
- Extend vault-only filter in from_datadir_with_password_filtered to skip
files containing "-master_" in addition to "-liquid_", so unified master
seed files are excluded from vault-signer reads.
- Add HotSigner::nostr_identity_keys — derives NIP-06 key pair at
m/44'/1237'/0'/0'/0' per the unified-master-seed design.
- Add HotSigner::try_clone — reconstructs an equivalent signer from the
stored mnemonic; useful when the same key material is needed by a second
consumer (e.g. installer re-using cube master seed as dev vault signer).
- Fix stale/duplicate doc comments on store_encrypted and
from_datadir_with_password.
… try_clone - Add MASTER_SEED_LABEL and LEGACY_LIQUID_SEED_LABEL constants for filename-based signer type identification (replaces raw string matching) - from_datadir_with_password_filtered: rename skip_liquid -> vault_only; use constants in the filename filter instead of raw string literals - HotSigner::try_clone(): infers network from master_xpriv instead of requiring caller to pass it (prevents wrong-network mistakes) - Add test_try_clone_fingerprint_matches test
…ner_fingerprint - Add serde alias for backward compat with existing settings.json files - Add with_master_signer builder; keep with_liquid_signer as deprecated shim - Update all call-sites across launcher.rs, breez/mod.rs, liquid/settings.rs, gui/tab.rs - Add test_cube_settings_alias_backward_compat to guard serde alias
Regtest, Testnet, Testnet4 and Signet are dev/test environments; enforcing the tier-based Cube limit there is counterproductive.
- Installer::new accepts developer_mode; uses master seed clone when true - Signer::new (existing constructor) used to wrap cloned HotSigner - SelectKeySource gates hot-signer option on developer_mode field - Hot-signer button hidden in production mode (requires HW wallet) - DefineDescriptor threads developer_mode through to SelectKeySource - tab.rs: loader/app call-sites read developer_mode from GlobalSettings
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughRenames and refactors "hot signer" → "master signer" across core and GUI, adds passkey-based master-key derivation (webview + macOS native), introduces installer developer_mode, implements PRF-derived MasterSigner construction and try_clone, and adds a master-seed backup wizard and related UI/state plumbing. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User UI
participant App as App (Launcher)
participant Web as Webview Ceremony
participant Core as MasterSigner logic
participant Store as Settings Storage
UI->>App: User starts passkey registration
App->>Web: create webview with CeremonyMode::Register
Web->>UI: show webview (user completes OS/Web flow)
Web->>App: IPC -> {"registered", credential_id, prf_output}
App->>Core: MasterSigner::from_prf_output(network, prf_output)
Core-->>App: MasterSigner instance
App->>Store: create CubeSettings.with_master_signer(..).with_passkey(..)
Store-->>App: persisted settings
App->>UI: navigate to new Cube / show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coincube-gui/src/gui/tab.rs (1)
398-413:⚠️ Potential issue | 🟡 MinorRun
cargo fmton this file before merge.CI is already red on Line 405, Line 544, and Line 585 for
cargo fmt -- --check.Also applies to: 537-552, 585-590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/gui/tab.rs` around lines 398 - 413, The code in the loader view branch (loader::Message::View(loader::ViewMessage::SetupVault)) has formatting violations around the Installer::new(...) call; run rustfmt (cargo fmt) on coincube-gui/src/gui/tab.rs and reformat the block so argument indentation and trailing commas match project style (including the multi-line Installer::new call and nearby ranges around lines shown); then re-run cargo fmt -- --check to ensure lines 405, 537-552, and 585-590 are fixed before merging.
🧹 Nitpick comments (1)
coincube-gui/src/installer/step/descriptor/editor/key.rs (1)
1198-1202: Consider removing unreachable non-dev subtitle branch
widget_generate_hot_key()is only included whenself.developer_modeis true (Line 1067), so the non-dev subtitle branch is effectively dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/installer/step/descriptor/editor/key.rs` around lines 1198 - 1202, The subtitle conditional contains a dead branch because widget_generate_hot_key() is only compiled/used when self.developer_mode is true; simplify by removing the unreachable else branch: set subtitle to the dev-mode message unconditionally when developer_mode is true (or replace the whole conditional with a single constant/dev message), and remove the unused non-dev string so the code around subtitle, widget_generate_hot_key, and any related UI text only contains the developer-mode message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coincube-core/src/signer.rs`:
- Around line 221-226: The code in signer.rs unconditionally skips files whose
filenames contain "-master_seed" (MASTER_SEED_LABEL) when vault_only is true,
preventing disk-based master-seed loading even in dev scenarios; modify the skip
logic to respect an explicit developer override (e.g., add a boolean parameter
like allow_dev_master_seed to/from_datadir_vault_only or a mode flag passed into
the file-iteration function) so that when the override is true you do not skip
files matching MASTER_SEED_LABEL or LEGACY_LIQUID_SEED_LABEL; update the caller
(from_datadir_vault_only in wallet.rs) to pass the appropriate override in dev
mode and document the new parameter so master-seed files can be loaded from disk
only when explicitly allowed.
In `@coincube-gui/src/app/breez/mod.rs`:
- Around line 67-73: The map_err closure currently only maps MnemonicStorage
NotFound to BreezError::SignerNotFound but misses the
coincube_core::signer::SignerError::SignerNotFound variant; update the match in
the map_err to include a branch for
coincube_core::signer::SignerError::SignerNotFound(fingerprint) and return
BreezError::SignerNotFound(fingerprint or master_signer_fingerprint as
appropriate), keeping the existing MnemonicStorage(io_err) =>
BreezError::SignerNotFound(master_signer_fingerprint) behavior and leaving other
errors mapped to BreezError::SignerError(e.to_string()).
In `@coincube-gui/src/installer/mod.rs`:
- Around line 143-165: When developer_mode is true but cloning the master signer
(the master_signer computed before assigning signer) fails, do not proceed with
developer_mode in downstream behavior; instead either return an error / abort
installation or override developer_mode to false for this Installer instance so
DefineDescriptor and UI logic don't treat the wallet as
unified-master-seed-derived. Locate the signer creation logic (the
developer_mode branch that builds master_signer and uses unwrap_or_else with
Signer::generate) and after detecting master_signer.is_none() either propagate
an error (fail closed) or set developer_mode = false (downgrade to normal mode)
before constructing Installer and before calling DefineDescriptor so the
hot-signer UI path is not offered when the master signer couldn't be cloned.
- Around line 145-165: The code block constructing `signer` is not
rustfmt-formatted; run `cargo fmt` to fix whitespace/line breaks so CI passes,
ensuring the `signer` initialization (the `developer_mode` conditional,
`master_signer` construction from `breez_client`/`liquid_signer()`, and the
`Signer::generate(network)` fallback) matches rustfmt style; reformat the hunk
and commit the formatted changes.
In `@coincube-gui/src/installer/step/descriptor/editor/key.rs`:
- Around line 1067-1069: CI reports a rustfmt failure in the let binding for
hot_signer in key.rs; run rustfmt (cargo fmt) to reformat the file and ensure
the expression assigning hot_signer (the let hot_signer = (self.developer_mode
&& !self.keys.contains_key(&hot_signer_fg) && safety_net_token.is_none())
binding) is formatted according to rustfmt rules (or manually adjust the line
breaks/parentheses to match rustfmt style) before pushing the change.
In `@coincube-gui/src/installer/step/descriptor/editor/mod.rs`:
- Around line 829-833: The test creates a Signet descriptor but generates a
Bitcoin signer; update the Signer generation to use Network::Signet so the
fixture uses a Signet signer. Locate the Sandbox<DefineDescriptor> construction
calling DefineDescriptor::new and change the Signer::generate(Network::Bitcoin)
call to Signer::generate(Network::Signet) (keeping the
Arc::new(Mutex::new(...)). This ensures the signer network matches the
descriptor network.
In `@coincube-gui/src/launcher.rs`:
- Around line 248-250: Formatting in coincube-gui/src/launcher.rs is failing
rustfmt; run rustfmt (cargo fmt) and reformat the file so expressions like the
at_limit computation and subsequent if block follow rustfmt style (ensure
spacing around && and alignment in the expression using cube_count, limit,
self.network, and Network::Bitcoin), and apply the same formatting fixes to the
similar blocks referenced around the later occurrences (the other uses of
at_limit-like checks). After formatting, re-run cargo fmt -- --check to confirm
the file is compliant.
---
Outside diff comments:
In `@coincube-gui/src/gui/tab.rs`:
- Around line 398-413: The code in the loader view branch
(loader::Message::View(loader::ViewMessage::SetupVault)) has formatting
violations around the Installer::new(...) call; run rustfmt (cargo fmt) on
coincube-gui/src/gui/tab.rs and reformat the block so argument indentation and
trailing commas match project style (including the multi-line Installer::new
call and nearby ranges around lines shown); then re-run cargo fmt -- --check to
ensure lines 405, 537-552, and 585-590 are fixed before merging.
---
Nitpick comments:
In `@coincube-gui/src/installer/step/descriptor/editor/key.rs`:
- Around line 1198-1202: The subtitle conditional contains a dead branch because
widget_generate_hot_key() is only compiled/used when self.developer_mode is
true; simplify by removing the unreachable else branch: set subtitle to the
dev-mode message unconditionally when developer_mode is true (or replace the
whole conditional with a single constant/dev message), and remove the unused
non-dev string so the code around subtitle, widget_generate_hot_key, and any
related UI text only contains the developer-mode message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bad9b136-c982-4bb1-8a18-46b89b7b12ae
📒 Files selected for processing (9)
coincube-core/src/signer.rscoincube-gui/src/app/breez/mod.rscoincube-gui/src/app/settings/mod.rscoincube-gui/src/app/state/liquid/settings.rscoincube-gui/src/gui/tab.rscoincube-gui/src/installer/mod.rscoincube-gui/src/installer/step/descriptor/editor/key.rscoincube-gui/src/installer/step/descriptor/editor/mod.rscoincube-gui/src/launcher.rs
Comment on lines
+221
to
+226
| // Skip Liquid and master-seed mnemonics when in vault-only mode. | ||
| if vault_only { | ||
| if let Some(filename) = path.file_name().and_then(|n| n.to_str()) { | ||
| if filename.contains("-liquid_") { | ||
| if filename.contains(&format!("-{}", LEGACY_LIQUID_SEED_LABEL)) | ||
| || filename.contains(&format!("-{}", MASTER_SEED_LABEL)) | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites of from_datadir_vault_only =="
rg -nP --type=rust -C3 '\bfrom_datadir_vault_only\s*\('
echo
echo "== Call sites of from_datadir_with_password_filtered =="
rg -nP --type=rust -C3 '\bfrom_datadir_with_password_filtered\s*\('
echo
echo "== Developer mode checks near mnemonic-loading paths =="
rg -nP --type=rust -C4 'developer_mode|vault_only'Repository: coincubetech/coincube
Length of output: 35676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking ALL imports/usages of from_datadir methods ==="
rg -nP --type=rust '\bfrom_datadir\b' --context 2
echo
echo "=== Checking definitions and doc comments for from_datadir methods ==="
rg -nP --type=rust 'pub fn from_datadir' -A 3Repository: coincubetech/coincube
Length of output: 2261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading signer.rs around line 304 for method definitions ==="
sed -n '295,320p' coincube-core/src/signer.rs
echo
echo "=== Reading signer.rs around line 221 for the vault_only logic ==="
sed -n '215,235p' coincube-core/src/signer.rs
echo
echo "=== Searching for any comments about master-seed loading intent ==="
rg -nP --type=rust -B 2 -A 2 'master.*seed|MASTER_SEED' coincube-core/src/signer.rs | head -50
echo
echo "=== Check if developer_mode is ever passed to signer loading methods ==="
rg -nP --type=rust 'developer_mode.*from_datadir|from_datadir.*developer_mode'Repository: coincubetech/coincube
Length of output: 4451
vault_only flag unconditionally excludes master-seed files from disk, with no developer-mode override
Lines 221–226 skip files containing -master_seed whenever vault_only == true. Since the only production call site is from_datadir_vault_only() (wallet.rs:154), master-seed files cannot be loaded from disk in any context—including developer mode.
This conflicts with the stated intent in the code comment at signer.rs:376–377 ("the installer re-using the cube's master seed as the vault hot-signer in dev mode"). While dev mode in the installer does load a master signer, it sources it from breez_client.liquid_signer() rather than from disk. If disk-based master-seed loading is needed in dev mode, a new code path or parameter is required to bypass the unconditional skip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coincube-core/src/signer.rs` around lines 221 - 226, The code in signer.rs
unconditionally skips files whose filenames contain "-master_seed"
(MASTER_SEED_LABEL) when vault_only is true, preventing disk-based master-seed
loading even in dev scenarios; modify the skip logic to respect an explicit
developer override (e.g., add a boolean parameter like allow_dev_master_seed
to/from_datadir_vault_only or a mode flag passed into the file-iteration
function) so that when the override is true you do not skip files matching
MASTER_SEED_LABEL or LEGACY_LIQUID_SEED_LABEL; update the caller
(from_datadir_vault_only in wallet.rs) to pass the appropriate override in dev
mode and document the new parameter so master-seed files can be loaded from disk
only when explicitly allowed.
Feature/passkey
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
coincube-gui/src/services/passkey/macos.rs (3)
52-55: Document the origin and purpose of the PRF salt.The salt "NYOASTRTSAOYN" appears to be a specific constant from the Breez passkey-login spec. Consider adding a reference URL or more context about why this specific salt value is used, as it's a security-critical parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/services/passkey/macos.rs` around lines 52 - 55, Update the PRF_SALT constant's documentation to state its origin and purpose: add a short comment above PRF_SALT explaining that the bytes correspond to the ASCII string "NYOASTRTSAOYN", that this value is mandated by the Breez passkey-login spec, and include a reference URL (or issue/spec section) to the official Breez spec for future auditing; ensure the comment also notes that this is a security-critical parameter and should not be changed without consulting the referenced spec.
136-161: Fallback hidden window creation may leak if retain/release is mismanaged.The fallback window creation calls
retainexplicitly but relies onRetained::from_rawto take ownership. Iffrom_rawfails (returnsNone), the manual retain isn't balanced. However, theexpectcall makes this a panic scenario, so in practice the leak won't occur. Consider adding a comment explaining the retain/ownership transfer.📝 Add clarifying comment
// Retain the window and return as Retained<RuntimeNSObject> // (which is what ASPresentationAnchor aliases to). + // The explicit retain call is balanced by Retained::from_raw + // taking ownership of the +1 reference count. let _: () = msg_send![window, retain];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/services/passkey/macos.rs` around lines 136 - 161, The fallback NSWindow allocation calls msg_send![window, retain] then immediately hands the pointer to Retained::from_raw(window as *mut NSObject) with an expect, which implicitly transfers ownership; add a brief clarifying comment around the retain + Retained::from_raw sequence (referencing window, alloc, msg_send!, retain, Retained::from_raw and the expect) that explains the manual retain is required to transfer ownership into Retained and that the subsequent expect will panic on failure (preventing an unbalanced retain from silently leaking); optionally mention that if you change from_raw to a fallible path in future you'll need to balance the retain/release.
319-324: Authentication flow not yet implemented — add tracking issue or TODO context.The
authenticatemethod currently returns an error. Consider adding a GitHub issue reference or more context about when this will be implemented, as passkey-based Cubes will need re-authentication support.Do you want me to open a GitHub issue to track the implementation of native macOS passkey authentication?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/services/passkey/macos.rs` around lines 319 - 324, The authenticate function currently returns a hardcoded Err; add a clear TODO comment above or inside pub fn authenticate(rp_id: &str, _credential_id: &[u8]) stating the planned work and linking to a tracking GitHub issue (e.g., "TODO: Implement native macOS passkey authentication — tracking: GH-<number> or https://..."), include a short description of expected behavior (assertion flow, re-authentication for passkey-based Cubes) and update the error message to reference the tracking issue so callers see context; ensure the comment references the function name authenticate and the parameters rp_id/_credential_id for discoverability.coincube-gui/src/launcher.rs (1)
344-445: Consider zeroizing the PIN after use in the async task.The
pinvariable is moved into the async block and used for mnemonic encryption. Afterstore_encryptedcompletes, the PIN remains in memory until the async block completes. Consider wrappingpininZeroizing<String>for defense-in-depth.♻️ Optional: Wrap PIN in Zeroizing
- let pin = if passkey_mode { - String::new() - } else { - self.create_cube_pin.value() - }; + let pin = Zeroizing::new(if passkey_mode { + String::new() + } else { + self.create_cube_pin.value() + });Then use
&*pinwhere a&stris needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/launcher.rs` around lines 344 - 445, The async closure captures and moves the plain `pin` into the Task, leaving it resident until the future completes; wrap the PIN in a zeroizing container (e.g., zeroize::Zeroizing<String>) before it is moved into the async block and pass a string slice via &*pin where a &str is required, then let the Zeroizing wrapper go out of scope (or explicitly drop it) immediately after the call to MasterSigner::store_encrypted to ensure the memory is zeroed; update references in this block (the async move closure, uses of `pin`, and the call to MasterSigner::store_encrypted) to use the Zeroizing wrapper and deref to &str as needed.coincube-gui/src/services/passkey/mod.rs (2)
186-188:Arc<PasskeyCeremonyChannel>with non-Send/Sync type triggers Clippy lint.The
#[allow(clippy::arc_with_non_send_sync)]suppresses the lint, butmpsc::Senderandmpsc::Receiverare indeed notSync. Since the channel is only used within the same thread (main thread for UI), this is safe, but consider usingstd::sync::mpscalternatives or documenting whyArcis needed here (likely for the IPC closure capture).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/services/passkey/mod.rs` around lines 186 - 188, The field channel is wrapped in Arc and suppresses clippy for non-Send/Sync because PasskeyCeremonyChannel contains non-Sync mpsc endpoints; since it's only used on the UI main thread, replace Arc with a single-threaded pointer (e.g., use Rc or Rc<RefCell<PasskeyCeremonyChannel>>) instead of Arc::new(...) to avoid the lint and make intent explicit, updating imports and any closure captures that assume Arc, or alternatively add a short doc comment above the channel field explaining why Arc is required and that the usage is single-threaded so the clippy allow is safe (referencing PasskeyCeremonyChannel and the channel field).
264-271: Error classification via substring matching may be fragile.Matching error messages with
contains("cancelled")orcontains("NotAllowedError")depends on the exact wording from the ceremony page. If the page's error messages change, these classifications could break.Consider defining error codes in the IPC protocol for more robust classification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coincube-gui/src/services/passkey/mod.rs` around lines 264 - 271, The current error handling in CeremonyIpcMessage::Error classifies failures by substring matching (e.g., checking message.contains("cancelled") or "NotAllowedError") which is fragile; update the IPC protocol and the CeremonyIpcMessage::Error variant to include a discrete error code/enum (e.g., ErrorCode) or numeric code alongside the message, then change the match in the handler to switch on that error code and map to PasskeyError::Cancelled, PasskeyError::PrfNotSupported, or PasskeyError::CeremonyFailed(message) accordingly so classification no longer depends on brittle substrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coincube-core/src/signer.rs`:
- Around line 390-400: The try_clone implementation incorrectly maps
master_xpriv.network via NetworkKind to only Bitcoin vs Testnet, which
misclassifies Signet/Regtest as Testnet; update the Signer struct to persist the
exact bitcoin::Network used when created (e.g., add a field like
original_network or network: bitcoin::Network), set that field wherever Signer
is constructed, and then change try_clone to call
Self::from_mnemonic(self.network, self.mnemonic.clone()) instead of inferring
from master_xpriv.network so the cloned signer preserves the exact network
(retain references to try_clone, master_xpriv.network, NetworkKind, and
from_mnemonic to locate edits).
In `@coincube-gui/src/app/state/vault/psbt.rs`:
- Around line 927-935: The current error path returns WalletError::MasterSigner
with the message "Master signer not loaded" even for signing failures; change
the error to be signing-specific so load vs sign are distinguishable. Update the
branch that returns Err(WalletError::MasterSigner("Master signer not
loaded".to_string()).into()) (and the earlier map_err wrapping signing failures)
to either: 1) introduce a new variant like WalletError::MasterSignerSign(String)
and use it for signing errors, or 2) keep WalletError::MasterSigner but use
distinct messages (e.g. "Master signer failed to sign PSBT: …" vs "Master signer
not loaded") so callers (e.g. code in wallet.rs) can render correct text; adjust
the map_err and the tuple-returning branch accordingly to reference the new
variant/message.
In `@coincube-gui/src/feature_flags.rs`:
- Around line 93-105: The test passkey_disabled_by_default asserts that the
compile-time constant PASSKEY_ENABLED is false, which breaks builds that
intentionally enable passkeys; remove the failing unit test function named
passkey_disabled_by_default from feature_flags.rs (or alternatively move this
"default is off" assertion into CI/build scripts instead of the test suite) so
that PASSKEY_ENABLED can be true in enabled builds without causing cargo test to
fail.
In `@coincube-gui/src/installer/step/descriptor/editor/border_wallet_wizard.rs`:
- Around line 238-243: The code panics on a poisoned Mutex via
expect("poisoned") when accessing self.signer; replace the expect with handling
that maps the lock error into a user-facing error (assign to self.error) and
then return the Err result so the UI stays alive. Concretely, change the signer
lock path used around self.signer so that a poisoned lock produces a formatted
error string assigned to self.error (or returned as Err) instead of calling
expect, and then proceed to call derive_grid_recovery_phrase() only on a
successfully-locked signer; keep references to self.signer,
derive_grid_recovery_phrase(), and self.error when implementing this flow so the
Generate Phrase button shows the error rather than crashing the app.
In `@coincube-gui/src/services/passkey/mod.rs`:
- Around line 15-19: The hardcoded ceremony URL CEREMONY_BASE_URL =
"https://coincube.io/passkey" (and RP_ID = "coincube.io") points to a
non-existent endpoint and will break the passkey flow; make the ceremony
endpoint and RP_ID configurable (e.g., read from env/config) and default to a
safe local/dev URL for non-production, replace all direct uses of
CEREMONY_BASE_URL and RP_ID in this module with the config-backed values (load
during init in the passkey module), and ensure any production deploy sets RP_ID
to the actual domain that will host the ceremony page.
---
Nitpick comments:
In `@coincube-gui/src/launcher.rs`:
- Around line 344-445: The async closure captures and moves the plain `pin` into
the Task, leaving it resident until the future completes; wrap the PIN in a
zeroizing container (e.g., zeroize::Zeroizing<String>) before it is moved into
the async block and pass a string slice via &*pin where a &str is required, then
let the Zeroizing wrapper go out of scope (or explicitly drop it) immediately
after the call to MasterSigner::store_encrypted to ensure the memory is zeroed;
update references in this block (the async move closure, uses of `pin`, and the
call to MasterSigner::store_encrypted) to use the Zeroizing wrapper and deref to
&str as needed.
In `@coincube-gui/src/services/passkey/macos.rs`:
- Around line 52-55: Update the PRF_SALT constant's documentation to state its
origin and purpose: add a short comment above PRF_SALT explaining that the bytes
correspond to the ASCII string "NYOASTRTSAOYN", that this value is mandated by
the Breez passkey-login spec, and include a reference URL (or issue/spec
section) to the official Breez spec for future auditing; ensure the comment also
notes that this is a security-critical parameter and should not be changed
without consulting the referenced spec.
- Around line 136-161: The fallback NSWindow allocation calls msg_send![window,
retain] then immediately hands the pointer to Retained::from_raw(window as *mut
NSObject) with an expect, which implicitly transfers ownership; add a brief
clarifying comment around the retain + Retained::from_raw sequence (referencing
window, alloc, msg_send!, retain, Retained::from_raw and the expect) that
explains the manual retain is required to transfer ownership into Retained and
that the subsequent expect will panic on failure (preventing an unbalanced
retain from silently leaking); optionally mention that if you change from_raw to
a fallible path in future you'll need to balance the retain/release.
- Around line 319-324: The authenticate function currently returns a hardcoded
Err; add a clear TODO comment above or inside pub fn authenticate(rp_id: &str,
_credential_id: &[u8]) stating the planned work and linking to a tracking GitHub
issue (e.g., "TODO: Implement native macOS passkey authentication — tracking:
GH-<number> or https://..."), include a short description of expected behavior
(assertion flow, re-authentication for passkey-based Cubes) and update the error
message to reference the tracking issue so callers see context; ensure the
comment references the function name authenticate and the parameters
rp_id/_credential_id for discoverability.
In `@coincube-gui/src/services/passkey/mod.rs`:
- Around line 186-188: The field channel is wrapped in Arc and suppresses clippy
for non-Send/Sync because PasskeyCeremonyChannel contains non-Sync mpsc
endpoints; since it's only used on the UI main thread, replace Arc with a
single-threaded pointer (e.g., use Rc or Rc<RefCell<PasskeyCeremonyChannel>>)
instead of Arc::new(...) to avoid the lint and make intent explicit, updating
imports and any closure captures that assume Arc, or alternatively add a short
doc comment above the channel field explaining why Arc is required and that the
usage is single-threaded so the clippy allow is safe (referencing
PasskeyCeremonyChannel and the channel field).
- Around line 264-271: The current error handling in CeremonyIpcMessage::Error
classifies failures by substring matching (e.g., checking
message.contains("cancelled") or "NotAllowedError") which is fragile; update the
IPC protocol and the CeremonyIpcMessage::Error variant to include a discrete
error code/enum (e.g., ErrorCode) or numeric code alongside the message, then
change the match in the handler to switch on that error code and map to
PasskeyError::Cancelled, PasskeyError::PrfNotSupported, or
PasskeyError::CeremonyFailed(message) accordingly so classification no longer
depends on brittle substrings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9bada8c-d52f-4d84-9871-578fd251cdc9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
.env.examplecoincube-core/Cargo.tomlcoincube-core/src/border_wallet/mod.rscoincube-core/src/descriptors/mod.rscoincube-core/src/signer.rscoincube-gui/Cargo.tomlcoincube-gui/src/app/breez/client.rscoincube-gui/src/app/breez/mod.rscoincube-gui/src/app/cache.rscoincube-gui/src/app/mod.rscoincube-gui/src/app/settings/mod.rscoincube-gui/src/app/state/liquid/overview.rscoincube-gui/src/app/state/liquid/settings.rscoincube-gui/src/app/state/mod.rscoincube-gui/src/app/state/settings/general.rscoincube-gui/src/app/state/settings/mod.rscoincube-gui/src/app/state/vault/overview.rscoincube-gui/src/app/state/vault/psbt.rscoincube-gui/src/app/view/liquid/settings.rscoincube-gui/src/app/view/message.rscoincube-gui/src/app/view/mod.rscoincube-gui/src/app/view/settings/backup.rscoincube-gui/src/app/view/settings/general.rscoincube-gui/src/app/view/settings/mod.rscoincube-gui/src/app/view/vault/overview.rscoincube-gui/src/app/view/vault/psbt.rscoincube-gui/src/app/wallet.rscoincube-gui/src/feature_flags.rscoincube-gui/src/gui/tab.rscoincube-gui/src/installer/descriptor.rscoincube-gui/src/installer/message.rscoincube-gui/src/installer/mod.rscoincube-gui/src/installer/step/descriptor/editor/border_wallet_wizard.rscoincube-gui/src/installer/step/descriptor/editor/key.rscoincube-gui/src/installer/step/descriptor/editor/mod.rscoincube-gui/src/installer/step/mnemonic.rscoincube-gui/src/installer/step/share_xpubs.rscoincube-gui/src/installer/view/mod.rscoincube-gui/src/launcher.rscoincube-gui/src/lib.rscoincube-gui/src/loader.rscoincube-gui/src/services/mod.rscoincube-gui/src/services/passkey/macos.rscoincube-gui/src/services/passkey/mod.rscoincube-gui/src/signer.rscoincube-gui/test_assets/global_settings.jsoncoincube-ui/src/component/hw.rsdocs/TRY.mdtests/tools/taproot_signer/README.mdtests/tools/taproot_signer/src/main.rs
✅ Files skipped from review due to trivial changes (9)
- coincube-core/Cargo.toml
- tests/tools/taproot_signer/src/main.rs
- tests/tools/taproot_signer/README.md
- docs/TRY.md
- coincube-gui/src/lib.rs
- .env.example
- coincube-gui/src/app/state/settings/mod.rs
- coincube-gui/test_assets/global_settings.json
- coincube-gui/src/services/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- coincube-gui/src/app/breez/mod.rs
- coincube-gui/src/installer/step/descriptor/editor/mod.rs
- coincube-gui/src/installer/mod.rs
- coincube-gui/src/gui/tab.rs
- coincube-gui/src/app/state/liquid/settings.rs
- coincube-gui/src/installer/step/descriptor/editor/key.rs
coincube-gui/src/installer/step/descriptor/editor/border_wallet_wizard.rs
Show resolved
Hide resolved
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.
As discussed..
Now we have a single master seed that is used across Liquid, Mostro and for Vault
Note: For vault it's enabled only when developer mode is true
Do Not Merge right now..
I am thoroughly testing the code right now in regtest setup
Summary by CodeRabbit
New Features
Improvements