refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk>#600
Merged
lklimek merged 4 commits intoFeb 18, 2026
Conversation
Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek
Contributor
|
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. 📝 WalkthroughWalkthroughReplaces RwLock-based SDK storage and access with ArcSwap and load()/store() usage across AppContext and backend tasks; updates call sites to use the new access pattern and passes an explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend_task/identity/register_dpns_name.rs (1)
181-224:⚠️ Potential issue | 🟡 MinorRedundant SDK load mid-operation — potential split-brain within a single registration.
The function already receives
sdk: &Sdk(line 27), which is used for theput_to_platformcalls (lines 141, 153). Loading a second SDK instance at line 181 means the pre-order/domain writes and the subsequent DPNS-name fetch + identity refresh could target different SDK configurations if the SDK is atomically swapped between the two loads (e.g., during a network config change). Use the existing parameter throughout instead.Additionally,
sdk_guardis now a misleading name — it is a plainSdkclone, not a lock guard.🛠️ Proposed fix
- let sdk_guard = self.sdk.load().as_ref().clone(); - - let owned_dpns_names = Document::fetch_many(&sdk_guard, dpns_names_document_query) + let owned_dpns_names = Document::fetch_many(sdk, dpns_names_document_query) .await ... - let refreshed_identity = dash_sdk::platform::Identity::fetch_by_identifier( - &sdk_guard, + let refreshed_identity = dash_sdk::platform::Identity::fetch_by_identifier( + sdk, qualified_identity.identity.id(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/register_dpns_name.rs` around lines 181 - 224, The code is loading a new SDK instance via self.sdk.load().as_ref().clone() into sdk_guard mid-operation which risks split-brain; replace that load with the existing sdk parameter passed into the function (use the same Sdk used for put_to_platform calls) and stop creating a second clone, and also rename the local variable (if kept) to something non-misleading (e.g., sdk or sdk_clone) so it’s clear it’s the same Sdk value rather than a lock guard; update all usages in this block (Document::fetch_many, Identity::fetch_by_identifier, and any subsequent calls that reference sdk_guard) to use the existing sdk variable.src/context/mod.rs (1)
519-521:⚠️ Potential issue | 🟡 MinorStale TODO references the removed
sdk.read().unwrap()API.🧹 Proposed fix
- // TODO: Use self.sdk.read().unwrap().version() instead of hardcoding + // TODO: Use self.sdk.load().version() instead of hardcoding🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` around lines 519 - 521, The TODO in the default_platform_version function references the removed sdk.read().unwrap() API; update the comment to reflect the current SDK accessor and how to obtain the version (e.g., call the new SDK version method instead of sdk.read().unwrap().version()), or remove the stale TODO entirely; locate default_platform_version and replace the line "// TODO: Use self.sdk.read().unwrap().version() instead of hardcoding" with a concise, accurate note like "// TODO: use the SDK's current version accessor (e.g., self.sdk.version())" or delete the TODO if no longer needed.
🧹 Nitpick comments (6)
src/backend_task/identity/refresh_loaded_identities_dpns_names.rs (1)
37-37: Renamesdk_guard→sdkto reflect the post-migration type.The variable now holds a cloned
Sdkvalue, not a lock guard. The namesdk_guardis a relic of theRwLockera and is misleading. The sibling filestop_up_identity.rsandfund_platform_address_from_*already usesdkconsistently.♻️ Proposed rename
- let sdk_guard = self.sdk.load().as_ref().clone(); + let sdk = self.sdk.load().as_ref().clone(); - let owned_dpns_names = Document::fetch_many(&sdk_guard, dpns_names_document_query) + let owned_dpns_names = Document::fetch_many(&sdk, dpns_names_document_query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/refresh_loaded_identities_dpns_names.rs` at line 37, The local variable named `sdk_guard` is misleading because after the migration it holds a cloned Sdk instance rather than a lock guard; rename `sdk_guard` to `sdk` in refresh_loaded_identities_dpns_names (replace occurrences where `sdk_guard` is used, e.g., assignments and subsequent method calls) to match the actual type and align with sibling files like top_up_identity.rs and fund_platform_address_from_*. Ensure all references are updated to the new identifier and that imports/borrows remain correct.src/backend_task/identity/withdraw_from_identity.rs (1)
24-24: Renamesdk_guard→sdk— same stale naming issue; the variable holds a clonedSdk, not a guard.♻️ Proposed rename
- let sdk_guard = self.sdk.load().as_ref().clone(); + let sdk = self.sdk.load().as_ref().clone();Update the two
&sdk_guardreferences on lines 34 and 89 to&sdk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/withdraw_from_identity.rs` at line 24, The local variable named `sdk_guard` in withdraw_from_identity.rs is misnamed because it holds a cloned `Sdk` rather than a guard; rename `sdk_guard` to `sdk` where it's declared (the `let sdk_guard = self.sdk.load().as_ref().clone();` line) and update all usages (e.g., the two `&sdk_guard` occurrences later in this function) to `&sdk` so the identifier correctly reflects the `Sdk` value being used.src/backend_task/identity/load_identity.rs (1)
288-290: Renamesdk_guard→sdk_inner/sdkand note dual-SDK usage.Two concerns:
sdk_guardno longer names a lock guard; rename tosdk(or e.g.sdk_innerto distinguish from thesdk: &Sdkparameter on line 42).This function already receives an
sdk: &Sdkparameter (used on lines 75 and 155) and now also loads a separateSdkclone fromself.sdkat line 288 for the DPNS fetch. These will be the same SDK unless a config swap happens between the two loads — an extremely unlikely but non-zero race. Consider passing the parametersdkthrough for the DPNS fetch to guarantee consistency, or document why the function requires two separate SDK sources.♻️ Proposed rename (minimal fix)
- let sdk_guard = self.sdk.load().as_ref().clone(); + let sdk_inner = self.sdk.load().as_ref().clone(); - let maybe_owned_dpns_names = Document::fetch_many(&sdk_guard, dpns_names_document_query) + let maybe_owned_dpns_names = Document::fetch_many(&sdk_inner, dpns_names_document_query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/load_identity.rs` around lines 288 - 290, Rename the local variable sdk_guard to sdk_inner (or simply sdk) to reflect it is not a lock guard and update references around Document::fetch_many and maybe_owned_dpns_names; then remove the dual-SDK race by using the function parameter sdk: &Sdk for the DPNS fetch (i.e., call Document::fetch_many with the existing sdk reference used earlier at lines referencing sdk: &Sdk) or, if retaining self.sdk is required, add a brief comment next to sdk_inner explaining why a fresh clone from self.sdk is intentionally used and acknowledge the tiny race. Ensure all references to sdk_guard are updated to the new name and that Document::fetch_many uses the consistent SDK source.src/backend_task/identity/transfer.rs (1)
21-21: Renamesdk_guard→sdk— same stale naming issue as inrefresh_loaded_identities_dpns_names.rs.♻️ Proposed rename
- let sdk_guard = self.sdk.load().as_ref().clone(); + let sdk = self.sdk.load().as_ref().clone();And update the two uses of
&sdk_guardto&sdkbelow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/transfer.rs` at line 21, Rename the local variable sdk_guard to sdk in the transfer code (change let sdk_guard = self.sdk.load().as_ref().clone(); to let sdk = self.sdk.load().as_ref().clone();) and update the two downstream usages that reference &sdk_guard to &sdk so all references match the new name; ensure no other references to sdk_guard remain and the cloned value and lifetimes are preserved when replacing identifiers.src/backend_task/mod.rs (1)
314-353: SDK loaded unconditionally but not threaded through all dispatch branches.
run_platform_info_task(line 344) andrun_wallet_task(line 350) ignore thesdkobtained at line 314 and each perform their own internalself.sdk.load(). This incurs two loads per invocation for those branches. WithArcSwapthe cost is negligible, but the pattern is inconsistent: ideally all task branches should either receive the SDK from the dispatch site or load it themselves — not both.♻️ Option A — pass sdk to all branches uniformly
Update
run_platform_info_taskand wallet task methods to acceptsdk: &Sdkand remove their internal loads, mirroring howrun_contract_task,run_identity_task, etc. already work.♻️ Option B — load lazily inside each task, remove line 314
Remove the top-level load and let each task function own its SDK load, consistent with how
run_wallet_taskalready behaves today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/mod.rs` around lines 314 - 353, The dispatch loads sdk into the local variable sdk but then calls run_platform_info_task and run_wallet_task which reload the SDK internally; update those branches to pass &sdk (i.e., call self.run_platform_info_task(platform_info_task, &sdk).await and self.run_wallet_task(wallet_task, &sdk).await), then modify the signatures of run_platform_info_task and run_wallet_task to accept sdk: &Sdk and remove their internal self.sdk.load() calls so they use the provided sdk instead.src/context/wallet_lifecycle.rs (1)
481-481: Considerload_full()overload().as_ref().clone()for a cheaper clone.
load_full()clones the innerArc<Sdk>(atomic counter increment) rather than cloning theSdkvalue. Usage sites would change from&sdk→&*sdk.♻️ Proposed refactor
- let sdk = self.sdk.load().as_ref().clone(); + let sdk = self.sdk.load_full(); // Arc<Sdk>Then update the usage:
- match get_transaction_info(&sdk, &txid).await { + match get_transaction_info(&*sdk, &txid).await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/wallet_lifecycle.rs` at line 481, Replace the expensive clone of the inner Sdk value by using load_full() so you clone the Arc<Sdk> instead of the Sdk itself: change the call site that currently uses self.sdk.load().as_ref().clone() to call self.sdk.load_full() (or otherwise call load_full() on the same atomic holder), then adjust subsequent usages that expect a &Sdk to borrow from the Arc by using &*sdk (or sdk.as_ref()) where sdk is the Arc<Sdk>; this affects the variable created from that load and any later references to it (e.g., where you previously wrote &sdk).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend_task/identity/load_identity_from_wallet.rs`:
- Around line 118-120: In load_user_identity_from_wallet, remove the redundant
self.sdk.load() usage and the sdk_guard variable and use the incoming sdk: &Sdk
parameter for the DPNS fetch instead; specifically replace the call that sets
sdk_guard (currently from self.sdk.load()) and the subsequent
Document::fetch_many(&sdk_guard, dpns_names_document_query) with
Document::fetch_many(sdk, dpns_names_document_query) (or otherwise pass
sdk.as_ref() if the signature requires), and drop any unnecessary clone to
ensure the same SDK instance (no divergent second load) is used throughout the
function.
In `@src/context/mod.rs`:
- Line 25: Reorder the use statements in src/context/mod.rs so the import of
ArcSwap (use arc_swap::ArcSwap;) appears before crossbeam_channel to satisfy
alphabetical ordering (e.g., move the ArcSwap line above any crossbeam_channel
imports), then run cargo +nightly fmt (and cargo clippy if desired) to apply
formatting and ensure the CI formatting check passes.
---
Outside diff comments:
In `@src/backend_task/identity/register_dpns_name.rs`:
- Around line 181-224: The code is loading a new SDK instance via
self.sdk.load().as_ref().clone() into sdk_guard mid-operation which risks
split-brain; replace that load with the existing sdk parameter passed into the
function (use the same Sdk used for put_to_platform calls) and stop creating a
second clone, and also rename the local variable (if kept) to something
non-misleading (e.g., sdk or sdk_clone) so it’s clear it’s the same Sdk value
rather than a lock guard; update all usages in this block (Document::fetch_many,
Identity::fetch_by_identifier, and any subsequent calls that reference
sdk_guard) to use the existing sdk variable.
In `@src/context/mod.rs`:
- Around line 519-521: The TODO in the default_platform_version function
references the removed sdk.read().unwrap() API; update the comment to reflect
the current SDK accessor and how to obtain the version (e.g., call the new SDK
version method instead of sdk.read().unwrap().version()), or remove the stale
TODO entirely; locate default_platform_version and replace the line "// TODO:
Use self.sdk.read().unwrap().version() instead of hardcoding" with a concise,
accurate note like "// TODO: use the SDK's current version accessor (e.g.,
self.sdk.version())" or delete the TODO if no longer needed.
---
Nitpick comments:
In `@src/backend_task/identity/load_identity.rs`:
- Around line 288-290: Rename the local variable sdk_guard to sdk_inner (or
simply sdk) to reflect it is not a lock guard and update references around
Document::fetch_many and maybe_owned_dpns_names; then remove the dual-SDK race
by using the function parameter sdk: &Sdk for the DPNS fetch (i.e., call
Document::fetch_many with the existing sdk reference used earlier at lines
referencing sdk: &Sdk) or, if retaining self.sdk is required, add a brief
comment next to sdk_inner explaining why a fresh clone from self.sdk is
intentionally used and acknowledge the tiny race. Ensure all references to
sdk_guard are updated to the new name and that Document::fetch_many uses the
consistent SDK source.
In `@src/backend_task/identity/refresh_loaded_identities_dpns_names.rs`:
- Line 37: The local variable named `sdk_guard` is misleading because after the
migration it holds a cloned Sdk instance rather than a lock guard; rename
`sdk_guard` to `sdk` in refresh_loaded_identities_dpns_names (replace
occurrences where `sdk_guard` is used, e.g., assignments and subsequent method
calls) to match the actual type and align with sibling files like
top_up_identity.rs and fund_platform_address_from_*. Ensure all references are
updated to the new identifier and that imports/borrows remain correct.
In `@src/backend_task/identity/transfer.rs`:
- Line 21: Rename the local variable sdk_guard to sdk in the transfer code
(change let sdk_guard = self.sdk.load().as_ref().clone(); to let sdk =
self.sdk.load().as_ref().clone();) and update the two downstream usages that
reference &sdk_guard to &sdk so all references match the new name; ensure no
other references to sdk_guard remain and the cloned value and lifetimes are
preserved when replacing identifiers.
In `@src/backend_task/identity/withdraw_from_identity.rs`:
- Line 24: The local variable named `sdk_guard` in withdraw_from_identity.rs is
misnamed because it holds a cloned `Sdk` rather than a guard; rename `sdk_guard`
to `sdk` where it's declared (the `let sdk_guard =
self.sdk.load().as_ref().clone();` line) and update all usages (e.g., the two
`&sdk_guard` occurrences later in this function) to `&sdk` so the identifier
correctly reflects the `Sdk` value being used.
In `@src/backend_task/mod.rs`:
- Around line 314-353: The dispatch loads sdk into the local variable sdk but
then calls run_platform_info_task and run_wallet_task which reload the SDK
internally; update those branches to pass &sdk (i.e., call
self.run_platform_info_task(platform_info_task, &sdk).await and
self.run_wallet_task(wallet_task, &sdk).await), then modify the signatures of
run_platform_info_task and run_wallet_task to accept sdk: &Sdk and remove their
internal self.sdk.load() calls so they use the provided sdk instead.
In `@src/context/wallet_lifecycle.rs`:
- Line 481: Replace the expensive clone of the inner Sdk value by using
load_full() so you clone the Arc<Sdk> instead of the Sdk itself: change the call
site that currently uses self.sdk.load().as_ref().clone() to call
self.sdk.load_full() (or otherwise call load_full() on the same atomic holder),
then adjust subsequent usages that expect a &Sdk to borrow from the Arc by using
&*sdk (or sdk.as_ref()) where sdk is the Arc<Sdk>; this affects the variable
created from that load and any later references to it (e.g., where you
previously wrote &sdk).
- Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Addressed all CodeRabbit findings in 863310d:
Skipped |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/backend_task/mod.rs (2)
314-314: Considerload_full()overload().as_ref().clone()for a cheaper SDK handle.
self.sdk.load().as_ref().clone()clones theSdkstruct (bumping every innerArcfield's refcount), whileself.sdk.load_full()returnsArc<Sdk>with a single atomic refcount increment. The PR motivation acknowledgesSdk::cloneis cheap, butload_full()is unambiguously cheaper and more idiomatic forArcSwapusage. Call sites would change&sdktosdk.as_ref().This applies across all files in the PR that use
self.sdk.load().as_ref().clone().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/mod.rs` at line 314, Replace usages of self.sdk.load().as_ref().clone() with the cheaper Arc handle returned by self.sdk.load_full(), updating call sites to use sdk.as_ref() instead of &sdk; specifically, change the code in functions referencing Sdk (e.g., the line using let sdk = self.sdk.load().as_ref().clone()) to call self.sdk.load_full() so you only perform a single atomic refcount increment on the Arc rather than cloning the inner Sdk, and adjust any subsequent uses from &sdk to sdk.as_ref().
314-351:run_wallet_taskis the only arm that doesn't receive the already-loaded&sdk.The
sdkat line 314 is loaded before thematchbut not forwarded torun_wallet_task, which reloads the SDK independently inside each wallet method. This is a minor consistency gap — wallet tasks could theoretically observe a different SDK version than the one used by the other match arms in the samerun_backend_taskcall. Passing&sdkintorun_wallet_taskwould align it with every other arm.♻️ Suggested refactor
- async fn run_wallet_task( - self: &Arc<Self>, - task: WalletTask, - ) -> Result<BackendTaskSuccessResult, String> { + async fn run_wallet_task( + self: &Arc<Self>, + task: WalletTask, + sdk: &dash_sdk::Sdk, + ) -> Result<BackendTaskSuccessResult, String> {And in
run_backend_task:- BackendTask::WalletTask(wallet_task) => self.run_wallet_task(wallet_task).await, + BackendTask::WalletTask(wallet_task) => self.run_wallet_task(wallet_task, &sdk).await,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/mod.rs` around lines 314 - 351, The wallet match arm should use the already-loaded sdk instead of reloading it: change the BackendTask::WalletTask arm to call self.run_wallet_task(wallet_task, &sdk). Update the run_wallet_task signature to accept an additional &sdk parameter and propagate that &sdk into any internal wallet helper methods (remove or stop reloading the SDK inside run_wallet_task and its callees). Ensure all internal uses reference the passed &sdk and update any other call sites of run_wallet_task to the new signature.src/backend_task/identity/refresh_loaded_identities_dpns_names.rs (1)
37-39: Move SDK load outside the loop for a consistent snapshot across the batch.The SDK is re-cloned on every iteration. Moving it before the loop avoids redundant loads and ensures all identities in the batch use the same
Sdkinstance, even if a config swap races mid-loop.♻️ Suggested refactor
+ let sdk = self.sdk.load().as_ref().clone(); + for mut qualified_identity in qualified_identities { let identity_id = qualified_identity.identity.id(); // Fetch DPNS names using SDK let dpns_names_document_query = DocumentQuery { /* ... */ }; - let sdk = self.sdk.load().as_ref().clone(); - let owned_dpns_names = Document::fetch_many(&sdk, dpns_names_document_query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/refresh_loaded_identities_dpns_names.rs` around lines 37 - 39, The code currently calls self.sdk.load().as_ref().clone() inside the loop, causing a new Sdk clone per iteration; move the SDK snapshot out of the loop so the entire batch uses the same instance: perform let sdk = self.sdk.load().as_ref().clone(); once before iterating, then use that sdk variable when calling Document::fetch_many(&sdk, ...) and any other per-identity operations; ensure ownership/borrowing still compiles (clone once, reuse) and remove the in-loop sdk clone.src/context/mod.rs (2)
309-309: Narrow TOCTOU window betweenload()andset_context_provider()— acceptable but worth noting.
sdk.load()returns a snapshot; if another thread callsstore()between theload()andset_context_provider(), the provider is set on the staleSdkinstance. This is the same race window that existed with a read-lock (which also didn't prevent a concurrent write after release), and bothset_core_backend_modeandreinit_core_client_and_sdkare triggered from user-initiated config changes—unlikely to race. No action needed now, but if concurrent config changes become possible, consider load-then-CAS or a sequencing mutex around provider swaps.Also applies to: 368-368, 511-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` at line 309, There is a TOCTOU race between calling app_context.sdk.load() and then set_context_provider() on the returned snapshot; to fix it, replace the two-step load+mutate pattern with an atomic compare-and-swap loop or a sequencing mutex so the provider is only set on the current Sdk instance: use app_context.sdk.compare_and_swap(old, new) (or equivalent AtomicRef/Arc swap) or introduce a dedicated mutex used by set_context_provider, set_core_backend_mode, and reinit_core_client_and_sdk to serialize provider swaps and reinitializations; update the logic at the sites using app_context.sdk.load().set_context_provider(...) (also the occurrences referenced at the other lines) so you either perform a CAS update of the Sdk or acquire the sequencing lock before modifying the provider.
519-520: TODO comment is unrealizable in aconst fn.
default_platform_versionisconst fn— it cannot callself.sdk.load().version()(noself, no runtime calls). Consider updating the TODO to reflect a realistic path, e.g., making the function non-const and accepting an&AppContext, or removing it if the hardcoded mapping is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` around lines 519 - 520, The TODO in the const fn default_platform_version(network: &Network) is impossible because const fns cannot access runtime self/sdk data; change the function to a non-const fn (remove const) and accept the runtime context needed (e.g., fn default_platform_version(ctx: &AppContext, network: &Network) -> &PlatformVersion or otherwise accept &self/sdk) and update all callers to pass the context, or if the hardcoded mapping is intentional keep it const and remove/replace the TODO comment to reflect that; ensure you update the function signature and all usages of default_platform_version accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend_task/identity/refresh_loaded_identities_dpns_names.rs`:
- Around line 37-39: The code currently calls self.sdk.load().as_ref().clone()
inside the loop, causing a new Sdk clone per iteration; move the SDK snapshot
out of the loop so the entire batch uses the same instance: perform let sdk =
self.sdk.load().as_ref().clone(); once before iterating, then use that sdk
variable when calling Document::fetch_many(&sdk, ...) and any other per-identity
operations; ensure ownership/borrowing still compiles (clone once, reuse) and
remove the in-loop sdk clone.
In `@src/backend_task/mod.rs`:
- Line 314: Replace usages of self.sdk.load().as_ref().clone() with the cheaper
Arc handle returned by self.sdk.load_full(), updating call sites to use
sdk.as_ref() instead of &sdk; specifically, change the code in functions
referencing Sdk (e.g., the line using let sdk =
self.sdk.load().as_ref().clone()) to call self.sdk.load_full() so you only
perform a single atomic refcount increment on the Arc rather than cloning the
inner Sdk, and adjust any subsequent uses from &sdk to sdk.as_ref().
- Around line 314-351: The wallet match arm should use the already-loaded sdk
instead of reloading it: change the BackendTask::WalletTask arm to call
self.run_wallet_task(wallet_task, &sdk). Update the run_wallet_task signature to
accept an additional &sdk parameter and propagate that &sdk into any internal
wallet helper methods (remove or stop reloading the SDK inside run_wallet_task
and its callees). Ensure all internal uses reference the passed &sdk and update
any other call sites of run_wallet_task to the new signature.
In `@src/context/mod.rs`:
- Line 309: There is a TOCTOU race between calling app_context.sdk.load() and
then set_context_provider() on the returned snapshot; to fix it, replace the
two-step load+mutate pattern with an atomic compare-and-swap loop or a
sequencing mutex so the provider is only set on the current Sdk instance: use
app_context.sdk.compare_and_swap(old, new) (or equivalent AtomicRef/Arc swap) or
introduce a dedicated mutex used by set_context_provider, set_core_backend_mode,
and reinit_core_client_and_sdk to serialize provider swaps and
reinitializations; update the logic at the sites using
app_context.sdk.load().set_context_provider(...) (also the occurrences
referenced at the other lines) so you either perform a CAS update of the Sdk or
acquire the sequencing lock before modifying the provider.
- Around line 519-520: The TODO in the const fn
default_platform_version(network: &Network) is impossible because const fns
cannot access runtime self/sdk data; change the function to a non-const fn
(remove const) and accept the runtime context needed (e.g., fn
default_platform_version(ctx: &AppContext, network: &Network) ->
&PlatformVersion or otherwise accept &self/sdk) and update all callers to pass
the context, or if the hardcoded mapping is intentional keep it const and
remove/replace the TODO comment to reflect that; ensure you update the function
signature and all usages of default_platform_version accordingly.
…ration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/context/mod.rs (1)
352-368: Redundant double read-lock onspv_context_provider.
spv_context_provideris read-locked twice (lines 352–360 forbind_app_context, then 361–367 for theclone) — both operations could be done under a single guard, reducing lock overhead.♻️ Suggested refactor
- if let Err(e) = self - .spv_context_provider - .read() - .map_err(|_| "SPV provider lock poisoned".to_string()) - .and_then(|provider| provider.bind_app_context(Arc::clone(self))) - { - tracing::error!("Failed to bind SPV provider: {}", e); - return; - } - let provider = match self.spv_context_provider.read() { - Ok(p) => p.clone(), - Err(_) => { - tracing::error!("SPV provider lock poisoned"); - return; - } - }; - self.sdk.load().set_context_provider(provider); + let provider = match self.spv_context_provider.read() { + Ok(p) => p.clone(), + Err(_) => { + tracing::error!("SPV provider lock poisoned"); + return; + } + }; + if let Err(e) = provider.bind_app_context(Arc::clone(self)) { + tracing::error!("Failed to bind SPV provider: {}", e); + return; + } + self.sdk.load().set_context_provider(provider);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/mod.rs` around lines 352 - 368, You currently acquire a read lock on spv_context_provider twice — once to call bind_app_context(Arc::clone(self)) and again to clone the provider for sdk.load().set_context_provider(...); instead acquire the read guard once (via self.spv_context_provider.read()), map the error similarly, call provider.bind_app_context(Arc::clone(self)) on that same guard, clone the provider from the guard into a local variable, drop the guard, and then call self.sdk.load().set_context_provider(cloned_provider) so you only hold a single read lock during both operations and avoid redundant locking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/context/mod.rs`:
- Around line 23-25: Remove the stray "[duplicate_comment]" marker from the
review comment so the import block remains clean and unambiguous; locate the
import lines (use arc_swap::ArcSwap;, use connection_status::ConnectionStatus;,
use crossbeam_channel::{Receiver, Sender};) and delete the duplicate-comment
token or duplicate review entry so only the resolved import-order message
remains.
---
Nitpick comments:
In `@src/context/mod.rs`:
- Around line 352-368: You currently acquire a read lock on spv_context_provider
twice — once to call bind_app_context(Arc::clone(self)) and again to clone the
provider for sdk.load().set_context_provider(...); instead acquire the read
guard once (via self.spv_context_provider.read()), map the error similarly, call
provider.bind_app_context(Arc::clone(self)) on that same guard, clone the
provider from the guard into a local variable, drop the guard, and then call
self.sdk.load().set_context_provider(cloned_provider) so you only hold a single
read lock during both operations and avoid redundant locking.
Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Addressed remaining nitpicks in cb1dfcd:
Remaining suggestions I'm deliberately not implementing:
|
lklimek
approved these changes
Feb 18, 2026
lklimek
pushed a commit
that referenced
this pull request
Feb 18, 2026
* refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
added a commit
that referenced
this pull request
Feb 23, 2026
…601) * docs: add unified message display design documents Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add MessageType::Warning * chore: initial implementation * chore: docs * chore: self review * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> (#600) * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove unused Insight API and show_in_ui config fields (#597) * refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com> * build: add Flatpak packaging and CI workflow (#589) * build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: fix build * Update src/app.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: peer review * feat: add collapsible details and suggestion support to MessageBanner Enhance BannerState with optional details (collapsible technical info) and suggestion (inline recovery hint) fields. BannerHandle gains with_details() and with_suggestion() builder methods and is now Clone. The details section renders collapsed by default with a "Show details" toggle and scrollable monospace content area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: correct MessageBanner dedup, replace_global reset, and doc drift - set_global now updates message_type on deduplicated banners - replace_global resets details/suggestion/details_expanded on replacement - Fix doc comment referencing set_text instead of set_message - Update architecture.md BannerState struct with details/suggestion fields - Update architecture.md render_banner signature and BannerHandle method table Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: log everything that is displayed by the MessageBanner * fix(ui): address PR #601 review items 12-16 for MessageBanner Add BannerState::new() and reset_to() helpers to eliminate duplicated field initialization across set_message, set_global, and replace_global. Fix dedup paths that forgot to update created_at/auto_dismiss_after/ show_elapsed (items 13-14). Fix concurrent refresh showing premature success by tracking pending_refresh_count (item 15). Remove unreachable match arm and tighten wildcard to guard on identity refresh tasks only (item 16). Update architecture docs to reflect current code (item 12). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): clarify display_message contract for Success/Info in MintTokensScreen Add explicit comment documenting that Success/Info types require no local state change since the global banner handles display. Addresses PR #601 review comment about asymmetric handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): use distinct icon for Error vs Warning banners Error banners now show ❌ (cross mark) instead of ⚠ (warning sign), so colorblind users can distinguish severity by shape, not just color. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): update banner test to expect new Error icon The Error icon changed from ⚠ to ❌ in the previous commit. Update test_banner_renders_all_types to match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update docs/ai-design/2026-02-17-unified-messages/architecture.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org> Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lklimek
added a commit
that referenced
this pull request
Mar 3, 2026
* docs: add unified message display design documents Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add MessageType::Warning * chore: initial implementation * chore: docs * chore: self review * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> (#600) * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove unused Insight API and show_in_ui config fields (#597) * refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com> * build: add Flatpak packaging and CI workflow (#589) * build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: fix build * chore: use new error handling everywhere - not self reviewed * chore: use message banner to show progress * fix: start elapsed counter at 1s instead of 0s Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: rabbit review * Update src/app.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: peer review * chore: fix build errors * fix(ui): make MessageBanner::set_global truly idempotent set_global() no longer resets timestamps, auto-dismiss timer, or logged flag when a banner with identical text already exists. This makes it safe to call every frame without log spam or timer restarts. Cherry-picked from origin/fix/spv-peer-timeout (08e3b3b). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address code review findings R01, R03, R06, R09, R10 - R01: Replace expect("No key selected") with graceful match + error banner in 11 token screens to prevent panics on missing signing key - R03: Remove dead backend_message field from AddExistingIdentityScreen - R06: Replace is_some() + unwrap() with idiomatic if-let-Some pattern in 10 token screens; use is_some_and() in structs.rs - R09: Add use imports for MessageBanner in 5 dashpay screens, replacing 22 fully-qualified crate::ui::components::MessageBanner:: calls - R10: Replace custom_dash_qt_error_message inline rendering with MessageBanner::set_global in network_chooser_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings SEC-08, SEC-09, RUST-015, SEC-05, SEC-07 - SEC-08: Restore safe if-let-Some pattern in WithdrawalScreen::refresh() to prevent double unwrap() panic on DB error or deleted identity - SEC-09: Restore original DB lookup in SendPaymentScreen::load_contact_info() replacing hardcoded "alice.dash" mock data - RUST-015: Revert unimplemented!() back to ui.label() in update_token_config MarketplaceTradeMode arm - SEC-05: Add success banners for contact request accept/reject in ContactRequests::display_task_result - SEC-07: Add MessageBanner::clear_all_global() and call it from AppState::change_network() to prevent stale banners leaking across network switches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: update coding conventions and message display guidance Add fallible constructor rule (Result<Self, ...> when they can fail), rename section to "General rules", and document MessageBanner idempotency (no guard needed for set_global). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace expect/panic with graceful error handling (SEC-10) Replace all expect() calls in token screen constructors and confirmation handlers with MessageBanner error display. Constructors handle errors internally and return Self with degraded state, keeping create_screen() clean. refresh() methods now show errors via MessageBanner instead of tracing-only logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): accept impl Display/Debug in MessageBanner API Change MessageBanner public methods to accept `impl Display` for message text and `impl Debug` for details, instead of `&str`. Remove needless `&format!(...)` borrows across 27 call sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): remove error Failed to get best chain lock for mainnet, testnet, devnet, and local Fixes #633 * feat(ui): add automatic connection status banners Display persistent MessageBanner notifications based on network connection state transitions. Mode-aware messages guide users toward the right recovery action (RPC vs SPV). - Disconnected (RPC): "Disconnected — check that Dash Core is running" - Disconnected (SPV): "Disconnected — check your internet connection" - Syncing (RPC): "Syncing with Dash Core…" - Syncing (SPV): "SPV sync in progress…" - Synced: banner cleared Uses Option<OverallConnectionState> for change detection, with None as initial/post-network-switch sentinel to force first evaluation. Closes #667 (partial — action links deferred to follow-up) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): pass TaskError directly to with_details() to avoid double-formatting The previous code used `format!("{err:?}")` which produced a String, then `with_details()` applied `{:#?}` again — wrapping the output in quotes and escaping inner characters. Passing `&err` directly lets Debug format once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): correct copy-paste error messages in token screens Replace "Burning" error messages that were copy-pasted from burn screen into freeze, destroy, and resume token screens with contextually correct messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): restore lost success/error messages in 5 screens Replace display_message() calls with MessageBanner::set_global() in screens where display_message() is now a side-effect-only handler and no longer displays messages directly. Affected screens: create_asset_lock_screen, wallets_screen (MineBlocks), address_table (export error), profile_screen (validation), contact_details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace unwrap/expect with graceful error handling Replace double unwrap in transfer_screen refresh() with unwrap_or_else + MessageBanner error display, matching the pattern from withdraw_screen. SEC-002 tokens_screen skipped: the .expect() calls are only on compile-time embedded image data (include_bytes!) which is safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): migrate masternode_list_diff_screen to global MessageBanner Replace ~15 local ui_state.message assignments and custom render_message_banner() with MessageBanner::set_global() via the display_message() trait method. Remove the message field from UiState and the unused Color32 import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve banner eviction logging and atomics - Upgrade BANNER_KEY_COUNTER from Relaxed to SeqCst ordering for future-proofing against multi-threaded usage - Log evicted banners at warn level in set_global() and replace_global() - Add comment explaining why show_global() always writes back Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove resolved TODO.md All items tracked in the unified message display TODO have been addressed or moved to the review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(ui): add INTENTIONAL markers and API documentation - Document why with_details() accepts Debug (not Display): structured error context is more useful in diagnostic details panes - Document replace_global() fallback-to-add behavior as intentional - Add INTENTIONAL(SEC-003) marker for developer mode error details - Add INTENTIONAL(SEC-004) marker for BannerHandle Send+Sync safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared token validation and fix ordering - Add validate_signing_key() helper in tokens/mod.rs to eliminate duplicated signing key validation across 12 token screens - Move signing key validation BEFORE WaitingForResult state transition so users see immediate errors instead of loading spinner then error - Replace is_err()/unwrap() anti-pattern with idiomatic let-else blocks in freeze, mint, transfer, destroy_frozen_funds, unfreeze screens Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): return Result from get_selected_wallet Replace &mut Option<String> error out-parameter with idiomatic Result<Option<Arc<RwLock<Wallet>>>, String>. Update 26+ callsites across identity, token, DashPay, and contract screens. Callsite patterns: unwrap_or_else with MessageBanner for user-visible errors, unwrap_or(None) where errors were previously silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): resolve duplicate imports and clippy warnings Remove duplicate MessageBanner imports in create_asset_lock_screen and wallets_screen/mod. Fix needless_borrows_for_generic_args clippy lints in profile_screen, transfer_screen, and wallets_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: reorder imports in masternode_list_diff_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from MessageBanner migration audit Apply 13 triaged review fixes plus 1 bug fix across 22 files: - Remove dead error state fields (backend_message, error_message, Error variant) - Replace .expect() panics with graceful fallback + MessageBanner in token screens - Fix missing MessageBanner::show_global() on contracts documents screen - Migrate DocumentActionScreen inline errors to MessageBanner - Replace unwrap_or(None) with error-reporting fallback in DashPay screens - Fix replace_global idempotency and use relaxed atomic ordering in banner - Extract shared set_error_banner helper for 8 token screens - Restore correct Some(0) wallet index semantics - Document BannerHandle lifecycle in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate token description length before sending to Platform (#530) * fix: validate token description length before sending to Platform Descriptions must be either empty or 3-100 characters long. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): validate token description by char count, not byte length String::len() counts UTF-8 bytes, causing multi-byte characters (CJK, emoji) to be miscounted against the 3–100 limit. Switch to chars().count() and update all UI labels to surface the minimum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> * refactor(ui): consolidate banner extension traits into message_banner Move BannerHandleExt and ResultBannerExt from banner_ext.rs into message_banner.rs where they belong. Merge take_and_clear() into OptionBannerExt trait (impl for Option<BannerHandle>) alongside or_show_error() for Option. Remove the separate banner_ext module and Clearable helper trait for simplicity. Apply review findings: DRY patterns (take_and_clear, or_show_error, load_identities_with_banner), fix .expect() panics in constructors, restore known_identities in refresh(), narrow pub field visibility, add ScreenLike doc comments, and update CLAUDE.md conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * doc(tmp): review guide for pr 604 * fix(ui): address review findings from grumpy-review iteration 1 - Replace .expect() panics in TransferTokensScreen and ClaimTokensScreen constructors with graceful degradation via Option<QualifiedIdentity> and MessageBanner error display (PROJ-001 HIGH) - Fix CLAUDE.md referencing non-existent BannerHandleExt trait name, corrected to OptionBannerExt (PROJ-002 MEDIUM) - Update set_global to preserve message_type when same text appears with different severity (RUST-001 MEDIUM) - Standardize display_message to handle both Error and Warning across all 11 token screens (RUST-002 MEDIUM) - Replace 21 manual take().clear() patterns with take_and_clear() across 6 files (RUST-003 MEDIUM) - Remove unused OptionBannerExt::or_show_error method (RUST-004 MEDIUM) - Migrate update_token_config from old error_message pattern to set_error_banner closure (RUST-005 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(ui): replace per-frame QualifiedIdentity clone with borrow Use .as_ref() instead of .clone() in the ui() identity guard of TransferTokensScreen and ClaimTokensScreen. QualifiedIdentity (Identity + KeyStorage + BTreeMap + Vec) was being cloned 60x/sec; now only borrowed for display, with clones deferred to button-click paths that actually need ownership. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add OptionResultExt::or_show_error for Option<T> Mirrors ResultBannerExt::or_show_error but for Option<T>: if None, displays a global error banner with the given message. Enables concise patterns like: identities.first().cloned().or_show_error(ctx, "No identities loaded") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from grumpy-review iteration 2 - Standardize display_message to handle both Error and Warning across 13 non-token screens that were missed in iteration 1 (PROJ-001 MEDIUM) - Replace .expect() panic in AddKeyScreen::refresh() with graceful or_show_error() + unwrap_or_default() (PROJ-002 MEDIUM) - Rename OptionResultExt to OptionBannerShowExt to avoid confusion with ResultBannerExt (RUST-001 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): handle Warning in add_new_identity_screen display_message Missed in the previous sweep — standardize display_message to handle both Error and Warning, matching all other screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): standardize display_message side-effect patterns across screens Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(deps): update dashpay/platform to rev 570e3af0 Adapt to breaking changes in rust-dashcore (a05d256f → 2824e52a): - Replace removed FeeLevel enum with FeeRate::normal() direct calls - Replace removed WalletManager::create_unsigned_payment_transaction() with TransactionBuilder + WalletManager::get_change_address() - Replace removed DashSpvClientInterface/DashSpvClientCommand with direct Arc<SpvClient> for quorum lookups via get_quorum_at_height() - Replace removed start()+monitor_network() with client.run(token) - Add .await to now-async subscribe_sync/network/progress methods - Replace removed SyncState::Initializing with SyncState::WaitForEvents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: log backtrace on panic * fix: panic in asset locks * fix(ui): address PR #604 review comments (CMT-001, CMT-002, CMT-003) - Fix QR scanner form reset matching wrong result type (CMT-001) - Remove dangerous identity fallback in token transfer screen (CMT-002) - Add fee-aware validation before credit transfers (CMT-003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(spv): replace AsyncRwLock with ArcSwapOption for SPV client reference The SPV client reference only needs atomic set/clear (on start/stop) and wait-free reads (quorum lookups). ArcSwapOption is a better fit than AsyncRwLock<Option<Arc<...>>> — no lock contention, no async in blocking context, and consistent with how AppContext already uses ArcSwap for the SDK. Also fixes stale doc comment referencing removed DashSpvClientInterface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #604 review comments iteration 2 (transfer_tokens_screen) - Remove duplicate conflicting banner on missing identity (CMT-003) - Use generic banner messages with with_details() for errors (CMT-002) - Fix refresh to match specific token by contract+position (CMT-001) - Document error banner pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * doc: review guide updated * fix(ui): apply grumpy-review triage fixes — core infrastructure Address findings from code review triage (SEC-002, RUST-003, RUST-012, PROJ-005): preserve error context in payment tx builder, add INTENTIONAL comment for Debug-formatting accepted risk, remove dead import in SPV manager, fix CLAUDE.md error banner documentation, and add OptionBannerExt replace/replace_with_elapsed convenience methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): apply grumpy-review triage fixes — screen-level changes RUST-010: Add wallet_open_attempted guard to ~30 screens preventing per-frame try_open_wallet_no_password calls. Guard resets on wallet or identity selection change. PROJ-001: Migrate register/update contract and document action screens from BroadcastStatus::Broadcasting(u64) to refresh_banner with elapsed time display. RUST-001: Surface database errors via .or_show_error() in contacts_list instead of silently swallowing with let _ =. RUST-005: tokens_screen display_message only clears operation_banner on Error/Warning, preserving it on Success/Info. RUST-007: Remove dead Error(String, Instant) variant from TransitionBroadcastStatus in transition_visualizer_screen. RUST-009: Make identity Optional in claim_tokens_screen with degraded state handling when identity not found locally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): handle OverallConnectionState::Error in connection banner Cover the new Error variant added by #650 (SPV sync error state) in the connection banner match arm. Shows an error banner directing users to the connection status tooltip for details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: update Cargo.lock * build: reduce dependency debuginfo to fix <unknown> panic backtraces std's backtrace symbolizer (gimli) silently fails to resolve symbols when parsing ~790MB of DWARF debug sections in our 1.1GB debug binary, producing backtraces full of <unknown> frames. Set `debug = "line-tables-only"` for dependency crates while keeping full debuginfo for our own code. This cuts DWARF from 790MB to ~354MB and binary size from 1.1GB to 600MB, letting gimli actually resolve panic backtraces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org> Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Paul DeLucia <69597248+pauldelucia@users.noreply.github.com>
lklimek
added a commit
that referenced
this pull request
Mar 18, 2026
* docs: add unified message display design documents Add UX specification, technical architecture, and HTML mockup for the MessageBanner component that will replace the ~50 ad-hoc error/message display implementations across screens with a single reusable component. Key design decisions: - Per-screen MessageBanner with show()/set_message() API - All colors via DashColors (zero hardcoded Color32 values) - 4 severity levels: Error, Warning, Success, Info - Auto-dismiss for Success/Info (5s), persistent for Error/Warning - Follows Component Design Pattern conventions (private fields, builder, show) - No changes to BackendTask/TaskResult/AppState architecture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add MessageType::Warning * chore: initial implementation * chore: docs * chore: self review * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> (#600) * refactor(context): replace RwLock<Sdk> with ArcSwap<Sdk> Sdk is internally thread-safe (Arc, ArcSwapOption, atomics) and all methods take &self. The RwLock was adding unnecessary contention across backend tasks. Using ArcSwap instead of plain Sdk because reinit_core_client_and_sdk() needs to atomically swap the entire Sdk instance when config changes. ArcSwap provides lock-free reads with atomic swap for the rare write. Suggested-by: lklimek * fix: address CodeRabbit review findings for ArcSwap migration - Fix import ordering: move arc_swap::ArcSwap before crossbeam_channel - Remove redundant SDK loads in load_identity_from_wallet, register_dpns_name, and load_identity — use the sdk parameter already passed to these functions - Fix stale TODO referencing removed sdk.read().unwrap() API - Rename sdk_guard → sdk in transfer, withdraw_from_identity, and refresh_loaded_identities_dpns_names (no longer lock guards) - Pass &sdk to run_platform_info_task from dispatch site instead of reloading internally - Fix leftover sdk.write() call in context_provider.rs (RwLock remnant) - Add missing Color32 import in wallets dialogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address remaining CodeRabbit review feedback on ArcSwap migration - Move SDK load outside for loop in refresh_loaded_identities_dpns_names.rs so it's loaded once for the batch instead of on each iteration - Update stale TODO comment in default_platform_version() to reflect that this is a free function with no sdk access Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate double read-lock on spv_context_provider Clone the SPV provider in a single lock acquisition, then bind app context on the clone instead of locking twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove unused Insight API and show_in_ui config fields (#597) * refactor: remove unused Insight API references The `insight_api_url` field in `NetworkConfig` and its associated `insight_api_uri()` method were never used in production code (both marked `#[allow(dead_code)]`). Remove the field, method, config entries, env example lines, and related tests. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * refactor: remove unused `show_in_ui` field from NetworkConfig The `show_in_ui` field was defined on `NetworkConfig` and serialized in `save()`, but never read by any production code to control network visibility. Remove the field, its serialization, env example lines, and test references. https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn * fix: add missing `Color32` import in wallet dialogs https://claude.ai/code/session_01HWPmCJHT8KTZGP9bFiksjn --------- Co-authored-by: Claude <noreply@anthropic.com> * build: add Flatpak packaging and CI workflow (#589) * build: remove snap version * build: add Flatpak packaging and CI workflow Add Flatpak build manifest, desktop entry, AppStream metadata, and GitHub Actions workflow for building and distributing Flatpak bundles. Uses freedesktop 25.08 runtime with rust-stable and llvm21 extensions. No application source code changes required - works in SPV mode by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: address review findings for Flatpak packaging - Pin GitHub Actions to commit SHAs for supply chain security - Upgrade softprops/action-gh-release from v1 to v2.2.2 - Remove redundant --socket=x11 (fallback-x11 suffices) - Remove duplicate tag trigger preventing double builds on release - Remove duplicate env vars inherited from top-level build-options - Add Flatpak build artifacts to .gitignore - Add bugtracker URL to AppStream metainfo - Remove deprecated <categories> from metainfo (use .desktop instead) - Add Terminal=false and Keywords to desktop entry - Add disk space check after SDK install in CI - Rename artifact to include architecture suffix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: simplify CI workflows for Linux-only releases - Remove "Free disk space" step from flatpak and release workflows - Remove Windows and macOS builds from release workflow - Use "ubuntu" runner image instead of pinned versions - Clean up unused matrix.ext references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: attach to existing releases instead of creating new ones - Replace release-creating job with attach-to-release (only on release event) - Add 14-day retention for build artifacts - On tag push or workflow_dispatch, only upload artifacts (no release) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: restore release.yml to original v1.0-dev version The release workflow changes were out of scope for the Flatpak PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review comments - Fix CRLF line endings in Flatpak manifest (convert to LF) - Set app_id on ViewportBuilder to match desktop StartupWMClass - Use --locked flag for reproducible cargo builds in Flatpak - Rename --repo=repo to --repo=flatpak-repo to match .gitignore - Add architecture note for protoc x86_64 binary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add Flatpak install instructions to README Add a dedicated section for installing via Flatpak on Linux, clarify that prerequisites are only needed for building from source, and rename "Installation" to "Build from Source" for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: match StartupWMClass to Flatpak app_id Use reverse-DNS format org.dash.DashEvoTool to match the Wayland app_id set via ViewportBuilder::with_app_id(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use ** glob for branch trigger to match feat/flatpak Single * doesn't match path separators in GitHub Actions branch filters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add aarch64 Flatpak build and caching to CI - Add matrix strategy for parallel x86_64 and aarch64 builds - Patch protoc URL/sha256 per architecture at build time - Cache .flatpak-builder directory keyed on arch + manifest + lockfile - Pin actions/cache to SHA Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: convert desktop and metainfo files to LF line endings Flatpak builder validates desktop files and rejects CRLF line endings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cancel in-progress Flatpak builds on new push Add concurrency group keyed on git ref so a new push cancels any running build for the same branch or release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings for Flatpak packaging - Remove unnecessary --filesystem=xdg-config/dash-evo-tool:create (Flatpak already redirects XDG_CONFIG_HOME to sandbox) - Add categories and keywords to AppStream metainfo for discoverability - Update README with both x86_64/aarch64 install commands, uninstall instructions, and Flatpak data path note - Clarify aarch64 comment in manifest to reference CI sed patching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: workflow timeout and perms * fix: move permissions to job level in Flatpak workflow Step-level permissions are not valid in GitHub Actions. Move contents: write to the job level where it is needed for release attachment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * build: cache Cargo registry and target in Flatpak CI Bind-mount host-side cargo-cache and cargo-target directories into the Flatpak build sandbox so CARGO_HOME and target/ persist across builds. Uses split restore/save with cleanup of incremental and registry/src (similar to Swatinem/rust-cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope cargo cache bind-mount sed to build-args only The previous sed matched --share=network in both finish-args and build-args, corrupting finish-args. Use a sed range to only target the build-args section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Apply suggestions from code review --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * chore: fix build * chore: use new error handling everywhere - not self reviewed * chore: use message banner to show progress * fix: start elapsed counter at 1s instead of 0s Aligns elapsed display with the countdown timer which already adds 1 to avoid showing "0s" immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: rabbit review * Update src/app.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: peer review * chore: fix build errors * fix(ui): make MessageBanner::set_global truly idempotent set_global() no longer resets timestamps, auto-dismiss timer, or logged flag when a banner with identical text already exists. This makes it safe to call every frame without log spam or timer restarts. Cherry-picked from origin/fix/spv-peer-timeout (08e3b3b). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address code review findings R01, R03, R06, R09, R10 - R01: Replace expect("No key selected") with graceful match + error banner in 11 token screens to prevent panics on missing signing key - R03: Remove dead backend_message field from AddExistingIdentityScreen - R06: Replace is_some() + unwrap() with idiomatic if-let-Some pattern in 10 token screens; use is_some_and() in structs.rs - R09: Add use imports for MessageBanner in 5 dashpay screens, replacing 22 fully-qualified crate::ui::components::MessageBanner:: calls - R10: Replace custom_dash_qt_error_message inline rendering with MessageBanner::set_global in network_chooser_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings SEC-08, SEC-09, RUST-015, SEC-05, SEC-07 - SEC-08: Restore safe if-let-Some pattern in WithdrawalScreen::refresh() to prevent double unwrap() panic on DB error or deleted identity - SEC-09: Restore original DB lookup in SendPaymentScreen::load_contact_info() replacing hardcoded "alice.dash" mock data - RUST-015: Revert unimplemented!() back to ui.label() in update_token_config MarketplaceTradeMode arm - SEC-05: Add success banners for contact request accept/reject in ContactRequests::display_task_result - SEC-07: Add MessageBanner::clear_all_global() and call it from AppState::change_network() to prevent stale banners leaking across network switches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: update coding conventions and message display guidance Add fallible constructor rule (Result<Self, ...> when they can fail), rename section to "General rules", and document MessageBanner idempotency (no guard needed for set_global). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace expect/panic with graceful error handling (SEC-10) Replace all expect() calls in token screen constructors and confirmation handlers with MessageBanner error display. Constructors handle errors internally and return Self with degraded state, keeping create_screen() clean. refresh() methods now show errors via MessageBanner instead of tracing-only logging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): accept impl Display/Debug in MessageBanner API Change MessageBanner public methods to accept `impl Display` for message text and `impl Debug` for details, instead of `&str`. Remove needless `&format!(...)` borrows across 27 call sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): remove error Failed to get best chain lock for mainnet, testnet, devnet, and local Fixes #633 * feat(ui): add automatic connection status banners Display persistent MessageBanner notifications based on network connection state transitions. Mode-aware messages guide users toward the right recovery action (RPC vs SPV). - Disconnected (RPC): "Disconnected — check that Dash Core is running" - Disconnected (SPV): "Disconnected — check your internet connection" - Syncing (RPC): "Syncing with Dash Core…" - Syncing (SPV): "SPV sync in progress…" - Synced: banner cleared Uses Option<OverallConnectionState> for change detection, with None as initial/post-network-switch sentinel to force first evaluation. Closes #667 (partial — action links deferred to follow-up) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): pass TaskError directly to with_details() to avoid double-formatting The previous code used `format!("{err:?}")` which produced a String, then `with_details()` applied `{:#?}` again — wrapping the output in quotes and escaping inner characters. Passing `&err` directly lets Debug format once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): correct copy-paste error messages in token screens Replace "Burning" error messages that were copy-pasted from burn screen into freeze, destroy, and resume token screens with contextually correct messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): restore lost success/error messages in 5 screens Replace display_message() calls with MessageBanner::set_global() in screens where display_message() is now a side-effect-only handler and no longer displays messages directly. Affected screens: create_asset_lock_screen, wallets_screen (MineBlocks), address_table (export error), profile_screen (validation), contact_details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): replace unwrap/expect with graceful error handling Replace double unwrap in transfer_screen refresh() with unwrap_or_else + MessageBanner error display, matching the pattern from withdraw_screen. SEC-002 tokens_screen skipped: the .expect() calls are only on compile-time embedded image data (include_bytes!) which is safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): migrate masternode_list_diff_screen to global MessageBanner Replace ~15 local ui_state.message assignments and custom render_message_banner() with MessageBanner::set_global() via the display_message() trait method. Remove the message field from UiState and the unused Color32 import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): improve banner eviction logging and atomics - Upgrade BANNER_KEY_COUNTER from Relaxed to SeqCst ordering for future-proofing against multi-threaded usage - Log evicted banners at warn level in set_global() and replace_global() - Add comment explaining why show_global() always writes back Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove resolved TODO.md All items tracked in the unified message display TODO have been addressed or moved to the review findings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(ui): add INTENTIONAL markers and API documentation - Document why with_details() accepts Debug (not Display): structured error context is more useful in diagnostic details panes - Document replace_global() fallback-to-add behavior as intentional - Add INTENTIONAL(SEC-003) marker for developer mode error details - Add INTENTIONAL(SEC-004) marker for BannerHandle Send+Sync safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared token validation and fix ordering - Add validate_signing_key() helper in tokens/mod.rs to eliminate duplicated signing key validation across 12 token screens - Move signing key validation BEFORE WaitingForResult state transition so users see immediate errors instead of loading spinner then error - Replace is_err()/unwrap() anti-pattern with idiomatic let-else blocks in freeze, mint, transfer, destroy_frozen_funds, unfreeze screens Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): return Result from get_selected_wallet Replace &mut Option<String> error out-parameter with idiomatic Result<Option<Arc<RwLock<Wallet>>>, String>. Update 26+ callsites across identity, token, DashPay, and contract screens. Callsite patterns: unwrap_or_else with MessageBanner for user-visible errors, unwrap_or(None) where errors were previously silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): resolve duplicate imports and clippy warnings Remove duplicate MessageBanner imports in create_asset_lock_screen and wallets_screen/mod. Fix needless_borrows_for_generic_args clippy lints in profile_screen, transfer_screen, and wallets_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: reorder imports in masternode_list_diff_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from MessageBanner migration audit Apply 13 triaged review fixes plus 1 bug fix across 22 files: - Remove dead error state fields (backend_message, error_message, Error variant) - Replace .expect() panics with graceful fallback + MessageBanner in token screens - Fix missing MessageBanner::show_global() on contracts documents screen - Migrate DocumentActionScreen inline errors to MessageBanner - Replace unwrap_or(None) with error-reporting fallback in DashPay screens - Fix replace_global idempotency and use relaxed atomic ordering in banner - Extract shared set_error_banner helper for 8 token screens - Restore correct Some(0) wallet index semantics - Document BannerHandle lifecycle in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate token description length before sending to Platform (#530) * fix: validate token description length before sending to Platform Descriptions must be either empty or 3-100 characters long. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): validate token description by char count, not byte length String::len() counts UTF-8 bytes, causing multi-byte characters (CJK, emoji) to be miscounted against the 3–100 limit. Switch to chars().count() and update all UI labels to surface the minimum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> * refactor(ui): consolidate banner extension traits into message_banner Move BannerHandleExt and ResultBannerExt from banner_ext.rs into message_banner.rs where they belong. Merge take_and_clear() into OptionBannerExt trait (impl for Option<BannerHandle>) alongside or_show_error() for Option. Remove the separate banner_ext module and Clearable helper trait for simplicity. Apply review findings: DRY patterns (take_and_clear, or_show_error, load_identities_with_banner), fix .expect() panics in constructors, restore known_identities in refresh(), narrow pub field visibility, add ScreenLike doc comments, and update CLAUDE.md conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * doc(tmp): review guide for pr 604 * fix(ui): address review findings from grumpy-review iteration 1 - Replace .expect() panics in TransferTokensScreen and ClaimTokensScreen constructors with graceful degradation via Option<QualifiedIdentity> and MessageBanner error display (PROJ-001 HIGH) - Fix CLAUDE.md referencing non-existent BannerHandleExt trait name, corrected to OptionBannerExt (PROJ-002 MEDIUM) - Update set_global to preserve message_type when same text appears with different severity (RUST-001 MEDIUM) - Standardize display_message to handle both Error and Warning across all 11 token screens (RUST-002 MEDIUM) - Replace 21 manual take().clear() patterns with take_and_clear() across 6 files (RUST-003 MEDIUM) - Remove unused OptionBannerExt::or_show_error method (RUST-004 MEDIUM) - Migrate update_token_config from old error_message pattern to set_error_banner closure (RUST-005 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(ui): replace per-frame QualifiedIdentity clone with borrow Use .as_ref() instead of .clone() in the ui() identity guard of TransferTokensScreen and ClaimTokensScreen. QualifiedIdentity (Identity + KeyStorage + BTreeMap + Vec) was being cloned 60x/sec; now only borrowed for display, with clones deferred to button-click paths that actually need ownership. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add OptionResultExt::or_show_error for Option<T> Mirrors ResultBannerExt::or_show_error but for Option<T>: if None, displays a global error banner with the given message. Enables concise patterns like: identities.first().cloned().or_show_error(ctx, "No identities loaded") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address review findings from grumpy-review iteration 2 - Standardize display_message to handle both Error and Warning across 13 non-token screens that were missed in iteration 1 (PROJ-001 MEDIUM) - Replace .expect() panic in AddKeyScreen::refresh() with graceful or_show_error() + unwrap_or_default() (PROJ-002 MEDIUM) - Rename OptionResultExt to OptionBannerShowExt to avoid confusion with ResultBannerExt (RUST-001 MEDIUM) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): handle Warning in add_new_identity_screen display_message Missed in the previous sweep — standardize display_message to handle both Error and Warning, matching all other screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): standardize display_message side-effect patterns across screens Guard side effects with Error|Warning match, use take_and_clear(), and remove redundant MessageBanner::set_global() call in 4 screens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(deps): update dashpay/platform to rev 570e3af0 Adapt to breaking changes in rust-dashcore (a05d256f → 2824e52a): - Replace removed FeeLevel enum with FeeRate::normal() direct calls - Replace removed WalletManager::create_unsigned_payment_transaction() with TransactionBuilder + WalletManager::get_change_address() - Replace removed DashSpvClientInterface/DashSpvClientCommand with direct Arc<SpvClient> for quorum lookups via get_quorum_at_height() - Replace removed start()+monitor_network() with client.run(token) - Add .await to now-async subscribe_sync/network/progress methods - Replace removed SyncState::Initializing with SyncState::WaitForEvents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: log backtrace on panic * fix: panic in asset locks * fix(ui): address PR #604 review comments (CMT-001, CMT-002, CMT-003) - Fix QR scanner form reset matching wrong result type (CMT-001) - Remove dangerous identity fallback in token transfer screen (CMT-002) - Add fee-aware validation before credit transfers (CMT-003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(spv): replace AsyncRwLock with ArcSwapOption for SPV client reference The SPV client reference only needs atomic set/clear (on start/stop) and wait-free reads (quorum lookups). ArcSwapOption is a better fit than AsyncRwLock<Option<Arc<...>>> — no lock contention, no async in blocking context, and consistent with how AppContext already uses ArcSwap for the SDK. Also fixes stale doc comment referencing removed DashSpvClientInterface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #604 review comments iteration 2 (transfer_tokens_screen) - Remove duplicate conflicting banner on missing identity (CMT-003) - Use generic banner messages with with_details() for errors (CMT-002) - Fix refresh to match specific token by contract+position (CMT-001) - Document error banner pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [claudesquad] update from 'testing research' on 02 Mar 26 13:25 CET (paused) * test: add backend E2E test harness and SPV wallet test Add test-only accessors (db(), wallets()) on AppContext gated behind cfg(test/testing), fix compilation errors in the backend-e2e test (private field access, unused import), and apply nightly rustfmt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add DASH_EVO_DATA_DIR env var to override app data directory Allows tests and CI to redirect all app data (database, SPV chain state, .env config) to a temp directory. The backend-e2e test harness now uses this to achieve full data isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add backend E2E test framework with shared state and funded wallets Evolve the prototype backend E2E test harness into a reusable framework: - LazyLock shared BackendTestContext with persistent workdir, SPV, and framework wallet (funded via E2E_WALLET_MNEMONIC or testnet faucet) - Task runner wrapper, polling wait helpers, faucet HTTP client - Identity key derivation helpers for wallet-funded registration - Six test scenarios: SPV wallet, identity create, identity withdraw, send/receive funds, fetch contracts, register DPNS name - Move default_identity_key_specs() from UI to backend_task::identity (domain logic, not UI concern) and make IdentityKeys fields pub - Add dashpay_contract_id() test accessor to AppContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio::sync::OnceCell instead of LazyLock for async init LazyLock triggers synchronously inside the #[tokio::test] runtime, causing "Cannot start a runtime from within a runtime" when the init function calls block_on(). Switch to tokio::sync::OnceCell with an async init() method so shared state initialization runs cooperatively within the existing tokio runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): handle persistent DB and SPV balance sync in E2E harness - Wait for SPV to sync existing wallet balance before checking if faucet funding is needed (pre-funded wallets need time to discover on-chain UTXOs) - Handle "already imported" error gracefully when framework wallet exists in persistent DB from a previous run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): wait for spendable balance and retry sends in E2E harness The SPV wallet reports total balance (including unconfirmed) but only confirmed/IS-locked UTXOs are available for transaction building. This caused "Insufficient funds" errors when tests tried to spend immediately after receiving funds. - Add wait_for_spendable_balance() that checks confirmed_balance_duffs() and triggers reconcile_spv_wallets() on each poll iteration - Add retry logic (5 attempts, 10s backoff) to create_funded_test_wallet() for sends that fail with InsufficientFunds - Wait for framework wallet change output to become spendable after each send so subsequent calls don't fail - Add wait_for_spendable_balance() before identity registration in all identity/DPNS tests - Add send_with_retry() helper in send_funds test - Add developer-facing README.md for the test framework Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): use tokio-shared-rt for shared runtime and sweep orphaned wallets Replace per-test tokio runtimes with tokio-shared-rt's global shared runtime to prevent SPV background tasks from dying between test functions. Add automatic orphaned wallet fund recovery during setup — wallets persist in DB, so on next run the harness sweeps funds back to the framework wallet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle OverallConnectionState::Error in connection banner Semantic merge conflict from v1.0-dev: PR #650 added an Error variant to OverallConnectionState, which our connection banner match didn't cover. Show an error banner when SPV sync enters the error state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs(test): improve backend-e2e documentation and funding UX - Add backend E2E section to CLAUDE.md pointing to the full README - Document .env file handling (project root vs workdir) and precedence - Fix test attribute in README: tokio::test → tokio_shared_rt::test(shared) - Update init sequence to reflect current code (spendable wait, orphan sweep) - Document automatic cleanup-on-init of orphaned test wallets - Raise minimum balance threshold from 1 to 10 tDASH - Always panic with receive address when faucet fails and balance is below minimum (previously only panicked on zero balance) - Show receive address in spendable-balance timeout warning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: patch SPV UTXO spendability flags before coin selection Upstream key-wallet-manager (rust-dashcore) never sets is_confirmed or is_instantlocked on UTXOs, but CoinSelector requires one of them. This caused "No UTXOs available for selection" errors despite having balance. Workaround infers status from block inclusion (height > 0 → confirmed, height == 0 → IS-locked). Ref: dashpay/rust-dashcore#514 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: harden backend E2E tests with retry logic and calibrated amounts - Increase funding amounts to avoid insufficient-funds flakes - Add 3-attempt retry for identity registration (chain height sync) - Retry on "No UTXOs" in send_with_retry alongside "Insufficient" - Wait for spendable balance in create_funded_test_wallet before return - Add CI workflow for backend E2E tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update test command in backend E2E workflow * ci: merge backend-e2e workflow into tests workflow (#727) * Initial plan * ci: merge backend-e2e workflow into tests workflow as an additional step Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * test: add cleanup_only noop test to sweep orphaned wallets Add a standalone test that triggers BackendTestContext initialization, which runs cleanup_test_wallets() as its final step. This can be run as a dedicated CI step after the E2E suite to sweep orphaned wallet funds back to the framework wallet. Run with: cargo test --test backend-e2e --all-features -- --ignored --nocapture cleanup_only Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add cleanup step for E2E test wallets Added a cleanup step for E2E test wallets in the workflow. * Simplify E2E test workflow conditions Removed conditional checks for E2E_WALLET_MNEMONIC in test steps. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR #673 review comments from triage Production code: - Extract patch_utxo_spendability_flags() helper to deduplicate workaround in estimate_fallback_amount and build_unsigned_payment_tx - Add IdentityKeys::new() constructor - Make wallet+address persistence atomic via store_wallet_with_addresses() - Add pending_wallet_selection after wallet creation Test code: - Add DPNS registration retry for identity propagation delay - Use u64 hex for DPNS name uniqueness (CMT-023) - Calibrate test funding amounts per reviewer feedback - Add fragility note on string-match wallet detection (CMT-006) - Add TODO for identity fund withdrawal in cleanup (CMT-032) - Add INTENTIONAL comment for bounded channel design (CMT-017) - Add balance assertion for return leg in send_funds test - Log reconcile_spv_wallets errors in wait helpers - Use is_ok_and for clarity in spv_wallet test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: remove UTXO spendability workaround, update platform dep Remove `patch_utxo_spendability_flags()` that faked IS-locked status on mempool UTXOs. Wait for upstream fix (dashpay/rust-dashcore#514) to properly set is_confirmed/is_instantlocked flags on UTXOs. Also: - Update dashpay/platform rev to aa86b74f7e2 - Adapt to upstream API: FeeLevel→FeeRate, remove NetworkExt import - Fix retry to catch "No UTXOs" errors in test harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: deduplicate default_identity_key_specs Move the single canonical copy to backend_task::identity::mod and have the UI screen import it, eliminating ~240 lines of duplicated function and tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci: temporarily disable backend E2E tests in CI The backend E2E tests need updates after the TaskError migration (#739) changed AppContext field visibility. Commenting out the CI steps until the tests are adapted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: type-safe wallet creation and identity key visibility - Restrict IdentityKeys fields to pub(crate) to prevent private key exposure outside the crate - Change register_wallet() to return TaskError instead of String, using proper rusqlite error matching via is_unique_constraint_violation() and a new WalletAlreadyImported variant - Change Wallet::new_from_seed() to accept Option<&Secret> for password instead of Option<&str>, keeping sensitive data in the Secret wrapper - Change Wallet::new_from_seed() to return TaskError instead of String, with a new WalletKeyDerivationFailed variant for derivation errors - Move build_identity_registration() and get_receive_address() from test helpers to production code in src/backend_task/identity/mod.rs - Extract is_unique_constraint_violation() to src/database/mod.rs as a shared pub(crate) utility, removing the duplicate in import_mnemonic_screen - Update all callers in add_new_wallet_screen and import_mnemonic_screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: address PR #673 review comments for E2E test framework - Move framework modules (harness, task_runner, wait, funding, cleanup, identity_helpers) into tests/backend-e2e/framework/ subdirectory - Make E2E_WALLET_MNEMONIC required (panic with instructions if unset) - Remove auto-faucet from initialization flow (keep as helper) - Remove retry loops in identity_create and identity_withdraw tests - Remove unnecessary wait_for_spendable_balance calls (already done by create_funded_test_wallet) - Replace all println!/eprintln! with tracing macros - Initialize tracing subscriber in harness init - Add "No spendable funds" and "spendable" to send retry conditions - Remove stale "other agent" NOTE comments from identity_helpers - Consolidate funding logic (harness delegates to funding module) - Update README for required mnemonic and new directory structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: reconcile production and test code after type-safe refactor Adapt test framework to production API changes: - Use IdentityKeys::new() constructor (fields now pub(crate)) - Match TaskError::WalletAlreadyImported variant instead of string - Allow dead_code on faucet helpers (kept but not auto-called) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: thread data_dir as explicit parameter to eliminate env var dependency Add `data_dir: PathBuf` field to `AppContext` and thread it through `Config::load_from()`, `Config::save()`, `SpvManager::new()`, `start_dash_qt()`, and `create_dash_core_config_if_not_exists()`. This enables E2E tests to specify their data directory without mutating process-wide environment variables, making parallel test execution safe. The `DASH_EVO_DATA_DIR` env var is still checked in production via `app_user_data_dir_path()`, but the resolved path is threaded through as a value rather than re-read from env on every call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): simplify funded wallet creation and make spendable balance check reliable - Remove retry loop from create_funded_test_wallet; wait for full amount_duffs instead of 1 duff in spendable balance check - Add Wallet::spv_confirmed_balance() that returns None when SPV hasn't synced yet (no max_balance fallback) - Use spv_confirmed_balance() in wait_for_spendable_balance so the wait never gets false positives from the fallback - Remove --test-threads=1 requirement from README (unsafe set_var was the only reason; data_dir is now threaded explicitly) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove send retry loop, handle PRIMARY KEY constraints, isolate SPV test dirs - Remove send_with_retry() from send_funds.rs; use wait_for_spendable_balance before each send instead - Add SQLITE_CONSTRAINT_PRIMARYKEY (1555) to uniqueness check alongside SQLITE_CONSTRAINT_UNIQUE (2067) - Use tempfile::TempDir in SPV tests instead of fixed /tmp/spv-test path to prevent state leaks and support concurrent test runs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Pasta Lil Claw <pasta+claw@dashboost.org> Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Paul DeLucia <69597248+pauldelucia@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: lklimek <lklimek@users.noreply.github.com>
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.
Summary
Replace
RwLock<Sdk>withArcSwap<Sdk>inAppContext, eliminating unnecessary lock contention across all backend tasks.Motivation
Sdkis internally thread-safe:Arc<DapiClient>for the inner connection poolArcSwapOptionforcontext_provider(lock-free atomic swap)&self, never&mut selfCloneis cheap (Arc clones)Every backend task was acquiring a read lock (
sdk.read().unwrap()) just to call&selfmethods on something already thread-safe. The only actual mutation was inreinit_core_client_and_sdk()which replaces the entireSdkinstance when config changes.Solution
ArcSwap<Sdk>provides lock-free reads via.load()(returns a guard that derefs to&Sdk) and atomic swap via.store()for the rare config-change pathsdk.read().unwrap()/sdk.read().map_err(...)?.clone()simplified tosdk.load()orsdk.load().as_ref().clone()arc-swap = "1"dependency (lightweight, zero-dep crate)Changes by category
{ let g = self.sdk.read().unwrap(); g.clone() }self.sdk.load().as_ref().clone()self.sdk.read().map_err(...)?.method()self.sdk.load().method()self.sdk.write()?.set_context_provider(p)self.sdk.load().set_context_provider(p)*sdk_lock = new_sdkself.sdk.store(Arc::new(new_sdk))Note
cargo checkwas NOT run (no Rust toolchain set up for this CI-less repo on this machine). The changes are mechanical — same patterns applied consistently across all 21 files. CI will verify.Suggested-by: @lklimek
/cc @PastaPastaPasta @QuantumExplorer
Summary by CodeRabbit
Refactor
Chores
Style