Open
Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughWires the desktop wallet UI into FROST descriptor coordination: adds wallet setup UI state and flows, new wallet and descriptor-related messages, FROST node lifecycle plumbing and descriptor event handling, xpub derivation/persistence, and a KfpNode descriptor session cancellation method. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant App as Desktop App
participant FrostNode as FROST Node
participant Storage as Wallet Storage
User->>App: WalletStartSetup / configure params
App->>App: initialize SetupState (shares, tiers, network)
User->>App: WalletBeginCoordination
App->>FrostNode: ensure node & request descriptor session
FrostNode->>App: DescriptorContributionNeeded(session_id, network, initiator_pubkey)
App->>App: derive_xpub (background task)
App->>FrostNode: contribute descriptor (xpub + fingerprint)
FrostNode->>App: DescriptorContributed / DescriptorReady
FrostNode->>App: DescriptorComplete(session_id, external, internal)
App->>Storage: persist descriptor (WalletEntry)
App->>App: update SetupState → Complete / notify UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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: 1
🧹 Nitpick comments (4)
keep-desktop/src/app.rs (1)
1073-1085: Redundant error field on session failure.On error, both
s.phaseis set toCoordinating(Failed(e))ands.erroris set. Since the phase is nowCoordinating,view_setupwill renderview_coordinatingwhich shows the failure in the status badge. Thes.errorfield is only displayed inview_configure, which won't be shown. Settings.errorhere is harmless but has no visible effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1073 - 1085, The error assignment is redundant: in Message::WalletSessionStarted when matching Err(e) you already set s.phase = SetupPhase::Coordinating(DescriptorProgress::Failed(e.clone())), so remove the useless s.error = Some(e) line from the Err branch in the Screen::Wallet -> WalletScreen handling; keep the phase update (and the e.clone() used for DescriptorProgress::Failed) and do not set s.error so view_setup/view_coordinating behavior remains correct (view_configure-exclusive s.error should not be populated here).keep-desktop/src/screen/wallet.rs (1)
217-226: Encoding tier index + value as a colon-delimited string is fragile.The
format!("{i}:{v}")encoding pairs the tier index with the user-entered value. This relies onsplit_once(':')inupdate_tier_field(app.rs, line 1110). Since the input is a numeric text field, colons shouldn't appear in practice, but this implicit coupling is easy to break if inputs change.Consider defining a small struct or tuple message variant (e.g.,
WalletThresholdChanged(usize, String)) to avoid the encode/parse round-trip entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/wallet.rs` around lines 217 - 226, The code currently encodes the tier index into a colon-delimited string via format!("{i}:{v}") in the text_input closures; instead change the Message variants WalletThresholdChanged and WalletTimelockChanged to take (usize, String) (or a small struct/tuple) and update the on_input closures in the loop over setup.tiers.enumerate() to send Message::WalletThresholdChanged(i, v) and Message::WalletTimelockChanged(i, v) (use move |v| with the captured i), then remove the split_once(':') parsing in update_tier_field and handle the typed (usize, String) payload directly. This touches the text_input call sites, the Message enum variants, and the update_tier_field handler; ensure all call sites and pattern matches are updated accordingly.keep-desktop/src/frost.rs (2)
647-687: Auto-contribution flow for non-initiator peers looks correct.The function properly retrieves the last connected share info, derives the xpub, and contributes it back to the initiator. The error path correctly surfaces a
Failedprogress message.One observation: if
frost_last_shareisNone(line 653-656), the contribution is silently skipped. This could happen if the relay was reconnected without a share context. Consider logging a warning in this case.💡 Add a log on silent skip
let share = match &self.frost_last_share { Some(s) => s.clone(), - None => return iced::Task::none(), + None => { + tracing::warn!("DescriptorContributionNeeded but no active share"); + return iced::Task::none(); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/frost.rs` around lines 647 - 687, In handle_descriptor_contribution_needed, when frost_last_share is None (the early-return path that currently silently skips contributing), add a warning log so reconnections without share context are visible; locate the match on self.frost_last_share and replace the immediate return iced::Task::none() with a log::warn! or tracing::warn! call that includes context (e.g., mention session_id and network) before returning, and consider doing the same for the get_frost_node() None branch to surface missing node state.
283-342: Consider extracting parameters into a struct to reduce argument count.With 8 parameters,
spawn_frost_nodealready has#[allow(clippy::too_many_arguments)]. As this function continues to grow, grouping related parameters (e.g.,keep_arc+keep_path+share_entry, or the output refs) into structs would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/frost.rs` around lines 283 - 342, spawn_frost_node currently accepts many parameters; refactor by creating one or two small parameter structs (e.g., FrostNodeConfig grouping keep_arc: Arc<Mutex<Option<Keep>>>, keep_path: PathBuf, share_entry: ShareEntry and NetworkConfig + relay_urls into NetworkConfigWrapper, and NodeHandles grouping node_out: Arc<Mutex<Option<Arc<KfpNode>>>> and kill_switch) and update the function signature of spawn_frost_node to take these structs instead, then update the internal call to setup_frost_node (and the setup_frost_node signature if it also takes the same long list) and all callers to pass the new structs; keep existing types and semantics (Arc/Mutex, FrostChannels, etc.) intact so only parameter packing/unpacking is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-desktop/src/frost.rs`:
- Around line 853-920: handle_descriptor_complete currently returns early if the
UI is not on Screen::Wallet or setup is missing, causing successful descriptors
to be dropped; instead, always persist the descriptor via lock_keep and
keep.store_wallet_descriptor (and map_err(friendly_err)) regardless of UI state,
by first obtaining group_pubkey and network from a UI-independent source (e.g.,
cache the coordination info on App or retrieve the setup stored by session id
rather than relying on Screen::Wallet/ws.setup). After storing, conditionally
update the UI: if let Screen::Wallet(ws) = &mut self.screen and
ws.setup.as_ref().map(|s| s.session_id.as_ref()) == Some(&session_id) then set
setup.phase and push to ws.descriptors on Ok, or set DescriptorProgress::Failed
on Err; ensure you still preserve created_at and use the same
keep_core::WalletDescriptor construction and friendly_err mapping when
persisting.
---
Nitpick comments:
In `@keep-desktop/src/app.rs`:
- Around line 1073-1085: The error assignment is redundant: in
Message::WalletSessionStarted when matching Err(e) you already set s.phase =
SetupPhase::Coordinating(DescriptorProgress::Failed(e.clone())), so remove the
useless s.error = Some(e) line from the Err branch in the Screen::Wallet ->
WalletScreen handling; keep the phase update (and the e.clone() used for
DescriptorProgress::Failed) and do not set s.error so
view_setup/view_coordinating behavior remains correct (view_configure-exclusive
s.error should not be populated here).
In `@keep-desktop/src/frost.rs`:
- Around line 647-687: In handle_descriptor_contribution_needed, when
frost_last_share is None (the early-return path that currently silently skips
contributing), add a warning log so reconnections without share context are
visible; locate the match on self.frost_last_share and replace the immediate
return iced::Task::none() with a log::warn! or tracing::warn! call that includes
context (e.g., mention session_id and network) before returning, and consider
doing the same for the get_frost_node() None branch to surface missing node
state.
- Around line 283-342: spawn_frost_node currently accepts many parameters;
refactor by creating one or two small parameter structs (e.g., FrostNodeConfig
grouping keep_arc: Arc<Mutex<Option<Keep>>>, keep_path: PathBuf, share_entry:
ShareEntry and NetworkConfig + relay_urls into NetworkConfigWrapper, and
NodeHandles grouping node_out: Arc<Mutex<Option<Arc<KfpNode>>>> and kill_switch)
and update the function signature of spawn_frost_node to take these structs
instead, then update the internal call to setup_frost_node (and the
setup_frost_node signature if it also takes the same long list) and all callers
to pass the new structs; keep existing types and semantics (Arc/Mutex,
FrostChannels, etc.) intact so only parameter packing/unpacking is needed.
In `@keep-desktop/src/screen/wallet.rs`:
- Around line 217-226: The code currently encodes the tier index into a
colon-delimited string via format!("{i}:{v}") in the text_input closures;
instead change the Message variants WalletThresholdChanged and
WalletTimelockChanged to take (usize, String) (or a small struct/tuple) and
update the on_input closures in the loop over setup.tiers.enumerate() to send
Message::WalletThresholdChanged(i, v) and Message::WalletTimelockChanged(i, v)
(use move |v| with the captured i), then remove the split_once(':') parsing in
update_tier_field and handle the typed (usize, String) payload directly. This
touches the text_input call sites, the Message enum variants, and the
update_tier_field handler; ensure all call sites and pattern matches are updated
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
keep-desktop/src/frost.rs (2)
875-883:lock_keepguard held duringstore_wallet_descriptor— verify this is a fast operation.The mutex guard from
lock_keepis held while callingstore_wallet_descriptor. If this involves disk I/O (file write, fsync), it blocks the UI thread. Given thatstore_wallet_descriptorlikely writes to the vault, consider whether this should be dispatched to a blocking task like other write operations in this codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/frost.rs` around lines 875 - 883, The code currently holds the mutex guard from lock_keep(&self.keep) while calling store_wallet_descriptor(&descriptor), which may perform blocking disk I/O and stall the UI; instead, drop the guard before performing the write or run the write in a blocking thread: capture/clone the Option<Keep> (or the minimal data needed) out of the guard, release the guard, then call keep.store_wallet_descriptor(&descriptor) off the UI thread (e.g., spawn_blocking or the existing blocking task mechanism) and map errors with friendly_err into store_result so the mutex is not held during the potentially slow I/O.
46-67: Signing share zeroization covers the extracted bytes but not intermediaries.
signing_bytesis correctly wrapped inZeroizing, butsigning_share.serialize()produces an intermediate allocation (the serialized form) before conversion to[u8; 32]. That intermediate is not zeroized. Similarly,key_packageandsigning_sharethemselves hold secret material that won't be explicitly cleared.This is likely a limitation of the frost-core types not implementing
Zeroize. The current approach is reasonable given that constraint — just noting the gap for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/frost.rs` around lines 46 - 67, The serialized signing share creates a transient allocation that isn't zeroized; to fix, serialize into a zeroized buffer (e.g., use Zeroizing<Vec<u8>> or serialize directly into a Zeroizing<[u8;32]>/zeroized stack buffer) and then copy/try_into that zeroized buffer to produce signing_bytes, ensuring the intermediate Vec/serialization buffer is dropped while zeroized; also limit scope and explicitly drop or let go out of scope the key_package and signing_share as soon as signing_bytes is produced so their secret data is not retained. Target symbols: signing_share.serialize(), signing_bytes, Zeroizing, key_package, signing_share.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-desktop/src/app.rs`:
- Around line 1080-1107: The handler Message::WalletSessionStarted currently
re-reads WalletScreen.selected_share and shares to compute group_pubkey, which
can race with UI changes; instead, capture the coordination metadata
(group_pubkey and network) in begin_descriptor_coordination before spawning the
async task and change the WalletSessionStarted message payload to carry
(session_id, group_pubkey, network). Update begin_descriptor_coordination to
compute group_pubkey from WalletScreen.selected_share and shares and include
network.clone() when creating the task, have the spawned task return
(session_id, group_pubkey, network) on success, and modify the
WalletSessionStarted match arm to use those passed-in values to insert into
active_coordinations (ActiveCoordination) rather than re-reading WalletScreen
state.
In `@keep-desktop/src/frost.rs`:
- Around line 854-910: handle_descriptor_complete is skipping non-initiators
because active_coordinations is never populated in the non-initiator path;
update handle_descriptor_contribution_needed to insert an entry into
self.active_coordinations keyed by the incoming session_id using the
group_pubkey from self.frost_last_share and the provided network so later
DescriptorComplete handling can find it. Concretely, in
handle_descriptor_contribution_needed create and insert a Coordination (or the
same struct stored in active_coordinations) with group_pubkey:
self.frost_last_share.group_pubkey (or equivalent) and network: network (the
parameter), keyed by session_id ([u8;32]) before returning so
handle_descriptor_complete can remove and use it.
---
Nitpick comments:
In `@keep-desktop/src/frost.rs`:
- Around line 875-883: The code currently holds the mutex guard from
lock_keep(&self.keep) while calling store_wallet_descriptor(&descriptor), which
may perform blocking disk I/O and stall the UI; instead, drop the guard before
performing the write or run the write in a blocking thread: capture/clone the
Option<Keep> (or the minimal data needed) out of the guard, release the guard,
then call keep.store_wallet_descriptor(&descriptor) off the UI thread (e.g.,
spawn_blocking or the existing blocking task mechanism) and map errors with
friendly_err into store_result so the mutex is not held during the potentially
slow I/O.
- Around line 46-67: The serialized signing share creates a transient allocation
that isn't zeroized; to fix, serialize into a zeroized buffer (e.g., use
Zeroizing<Vec<u8>> or serialize directly into a Zeroizing<[u8;32]>/zeroized
stack buffer) and then copy/try_into that zeroized buffer to produce
signing_bytes, ensuring the intermediate Vec/serialization buffer is dropped
while zeroized; also limit scope and explicitly drop or let go out of scope the
key_package and signing_share as soon as signing_bytes is produced so their
secret data is not retained. Target symbols: signing_share.serialize(),
signing_bytes, Zeroizing, key_package, signing_share.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
keep-desktop/src/app.rs (2)
1394-1417:do_lockdoes not clearactive_coordinations.
handle_disconnect_relay(called at line 1395) clears the frost node, which prevents new descriptor events from arriving, butactive_coordinationsretains stale entries. These will persist until the next unlock session. For consistency with other state cleanup performed indo_lock, consider clearing it:self.toast = None; self.toast_dismiss_at = None; +self.active_coordinations.clear(); self.screen = Screen::Unlock(UnlockScreen::new(true));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1394 - 1417, In do_lock, after calling handle_disconnect_relay (and/or alongside other state resets), clear the stale coordination state by invoking the active_coordinations clear/reset on self (e.g., self.active_coordinations.clear() or equivalent) so active_coordinations does not retain entries across lock; update do_lock (referencing do_lock, handle_disconnect_relay, and active_coordinations) to perform this clear before returning the Task.
1211-1216: Initialreceived: 1is set before the initiator's contribution actually succeeds.The UI progress is updated to show 1 contribution received before the async
derive_xpub+request_descriptortask (lines 1222–1240) completes. If that task fails, the error path inWalletSessionStartedoverwrites the phase toFailed, so the user won't be stuck. This is a minor optimistic-UI glitch — the count briefly shows 1 even though nothing was contributed yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 1211 - 1216, The UI sets received: 1 prematurely in the Wallet screen setup block (Screen::Wallet / WalletScreen) before the async task that runs derive_xpub and request_descriptor completes; move the state update so it only increments to received: 1 after the async contribution succeeds (i.e., after derive_xpub + request_descriptor returns successfully) or update it in the success branch that currently emits WalletSessionStarted; specifically, remove or defer setting SetupPhase::Coordinating(DescriptorProgress::WaitingContributions { received: 1, ... }) at the top and instead update the WalletScreen phase to WaitingContributions with received: 1 inside the successful completion handler for the derive_xpub/request_descriptor flow (or in the code path that emits WalletSessionStarted) so the progress reflects only confirmed contributions.keep-desktop/src/frost.rs (1)
31-39:parse_bitcoin_networkmirrors the allowlist inapp.rs— keep them in sync.The accepted values here (
"bitcoin","testnet","signet","regtest") matchVALID_NETWORKSinapp.rsline 1047. If one changes, the other must follow. Consider extracting a shared constant or reusingparse_bitcoin_networkas the single source of truth for network validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/frost.rs` around lines 31 - 39, The network allowlist is duplicated between parse_bitcoin_network and VALID_NETWORKS; extract a single source of truth (e.g., move the Vec<&'static str> or array of valid names into a shared module like crate::networks::VALID_NETWORKS or expose parse_bitcoin_network from frost.rs) and have both parse_bitcoin_network and the code in app.rs reference that shared symbol instead of hardcoding values; update parse_bitcoin_network to consult the shared list (or export it and derive the match from it) and replace VALID_NETWORKS in app.rs with a reference to the shared constant or call to parse_bitcoin_network so they remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-desktop/src/app.rs`:
- Around line 1080-1107: The handler for Message::WalletSessionStarted inserts
into self.active_coordinations unconditionally even when the UI setup was
cancelled (s.setup became None), creating a dangling coordination; modify the Ok
branch so the ActiveCoordination insert only occurs when the wallet setup is
still present (the same if let Screen::Wallet(WalletScreen { setup: Some(s), ..
}) match used to set s.session_id), i.e., move or guard the
self.active_coordinations.insert(...) behind that check (or check
s.setup.is_some() before inserting) so we never register a coordination without
a corresponding setup/session_id to clean up; reference
Message::WalletSessionStarted, WalletScreen, s.setup, session_id, and
ActiveCoordination when making the change.
In `@keep-desktop/src/frost.rs`:
- Around line 41-73: The serialized signing share bytes produced by
signing_share.serialize() are not wrapped in Zeroizing and may remain in memory;
in derive_xpub wrap the Vec<u8> from signing_share.serialize() in Zeroizing
(e.g., Zeroizing::new(the_vec)) before converting to a [u8;32] so the
intermediate buffer is zeroized on drop, keeping the existing try_into()
conversion and the signing_bytes Zeroizing<[u8;32]> binding and preserving the
current map_err() error handling for invalid length.
---
Nitpick comments:
In `@keep-desktop/src/app.rs`:
- Around line 1394-1417: In do_lock, after calling handle_disconnect_relay
(and/or alongside other state resets), clear the stale coordination state by
invoking the active_coordinations clear/reset on self (e.g.,
self.active_coordinations.clear() or equivalent) so active_coordinations does
not retain entries across lock; update do_lock (referencing do_lock,
handle_disconnect_relay, and active_coordinations) to perform this clear before
returning the Task.
- Around line 1211-1216: The UI sets received: 1 prematurely in the Wallet
screen setup block (Screen::Wallet / WalletScreen) before the async task that
runs derive_xpub and request_descriptor completes; move the state update so it
only increments to received: 1 after the async contribution succeeds (i.e.,
after derive_xpub + request_descriptor returns successfully) or update it in the
success branch that currently emits WalletSessionStarted; specifically, remove
or defer setting
SetupPhase::Coordinating(DescriptorProgress::WaitingContributions { received: 1,
... }) at the top and instead update the WalletScreen phase to
WaitingContributions with received: 1 inside the successful completion handler
for the derive_xpub/request_descriptor flow (or in the code path that emits
WalletSessionStarted) so the progress reflects only confirmed contributions.
In `@keep-desktop/src/frost.rs`:
- Around line 31-39: The network allowlist is duplicated between
parse_bitcoin_network and VALID_NETWORKS; extract a single source of truth
(e.g., move the Vec<&'static str> or array of valid names into a shared module
like crate::networks::VALID_NETWORKS or expose parse_bitcoin_network from
frost.rs) and have both parse_bitcoin_network and the code in app.rs reference
that shared symbol instead of hardcoding values; update parse_bitcoin_network to
consult the shared list (or export it and derive the match from it) and replace
VALID_NETWORKS in app.rs with a reference to the shared constant or call to
parse_bitcoin_network so they remain in sync.
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
1 participant
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.
Summary
Test plan
Summary by CodeRabbit