test: implementation of basic actual e2e testing for DET

[PastaPastaPasta]

Summary by CodeRabbit

• New Features

  • Added input hints/placeholders across wallets, identities, contracts, and token search; updated contract action label to "Fetch Contracts".
  • Exposed identities module for broader integration.

• Tests

  • Added a comprehensive end-to-end test suite with phased workflows, reusable UI harness and helpers, and a new CI workflow to run E2E journeys.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a feature-gated e2e testing system: a new Cargo test target and feature flag, e2e-only accessors in the UI/context, a TestContext and robust harness with retry/error handling, a phased end-to-end test suite, UI hint text tweaks, and CI workflow to run the E2E tests.

Changes

Cohort / File(s)	Summary
Cargo Configuration Cargo.toml	Added e2e feature, [[test]] target tests/e2e/main.rs requiring the feature, and a [patch."https://github.com/dashpay/rust-dashcore"] block overriding multiple dashcore crates to a git rev.
App Context Accessors src/context/mod.rs	Added #[cfg(feature = "e2e")] public accessors db() and wallets() to expose internal state for tests.
Module Visibility src/ui/mod.rs	Changed identities module from pub(crate) to pub to allow external test access.
UI Input Hints & Labels src/ui/contracts_documents/add_contracts_screen.rs, src/ui/identities/add_existing_identity_screen.rs, src/ui/tokens/tokens_screen/keyword_search.rs, src/ui/wallets/import_mnemonic_screen.rs	Switched TextEdit usage to builder-style with hint_text for multiple inputs (Contract ID, Identity ID, DPNS name, token search, wallet name). Changed button label "Add Contracts" → "Fetch Contracts".
E2E-only Screen Helpers src/ui/identities/add_new_identity_screen/mod.rs, src/ui/identities/register_dpns_name_screen.rs, src/ui/wallets/import_mnemonic_screen.rs	Added #[cfg(feature = "e2e")] impls exposing screen state and setters (e.g., step(), funding_method(), set_funding_amount(), set_alias_input(), set_name_input(), set_seed_phrase_words(), set_alias(), trigger_save()). Made successful_qualified_identity_id pub(crate).
E2E Helpers & Harness tests/e2e/helpers/mod.rs, tests/e2e/helpers/context.rs, tests/e2e/helpers/harness.rs	Added TestContext (persistent per-test state) and a comprehensive Harness: timing constants, error classification, wait/poll utilities, navigation/input helpers, retry/error handling, SPV readiness gating, and emergency cleanup routines.
Phased E2E Tests tests/e2e/main.rs, tests/e2e/phases/... (multiple files)	Replaced prior e2e layout with a phase-based structure (smoke, setup, wallet_ui, wallet, platform, tokens, identity, dpns, teardown) and a single orchestrating test e2e_testnet_journey() that executes phases with panic-safe cleanup.
New Kit Tests tests/kittest/main.rs, tests/kittest/interactions.rs	Added an egui kittest module interactions.rs (500+ lines) exercising welcome screen, navigation, stability, rendering across sizes, and various UI interactions; registered modules in tests/kittest/main.rs.
Removed Legacy E2E Tests tests/e2e/helpers.rs (old), tests/e2e/navigation.rs, tests/e2e/wallet_flows.rs	Deleted prior TestHarness helper and legacy navigation and wallet_flow test files in favor of the new harness and phased tests.
CI Workflow .github/workflows/e2e.yml	Added GitHub Actions workflow to run E2E tests with environment gating for E2E_WALLET_MNEMONIC, tooling installs, caching, and artifact upload of e2e logs.
Tests Entrypoints tests/e2e/mod.rs, tests/e2e/phases/mod.rs	Removed old module layout; added phases submodules and exported phase modules for the new orchestration.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Harness as E2E Harness
participant UI as App UI / Harness -> AppState
participant SPV as SPV/backend
participant DB as Database
Harness->>UI: drive UI actions (navigate, type, click)
UI->>UI: update AppState / render frames
UI->>SPV: start/check SPV sync, request balance
SPV->>DB: read/write wallet/state
SPV-->>UI: SPV status / balances
UI-->>Harness: labels / results observed

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• test: E2E Testing Roadmap — End-Goal State & Phased Plan #599: Implements the phased, feature-gated e2e testing infrastructure and harness described in that issue.

Poem

🐇
Hops through screens with tiny paws,
Phases lined up, testing laws.
Wallets, names, and SPV tune,
Harness hums beneath the moon.
E2E journey—hop, test, swoon!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly summarizes the main change: implementation of basic end-to-end testing for DET, which aligns with the substantive additions of E2E test infrastructure, phases, helpers, and harness throughout the codebase.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch e2e

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/ui/wallets/wallets_screen/dialogs.rs`:
- Around line 1121-1125: The method close_receive_dialog currently uses
#[allow(dead_code)] and pub(super), making it unreachable from E2E tests;
replace the attribute with #[cfg(feature = "e2e")] and widen visibility to
pub(crate) (or pub) so tests in tests/e2e can call it, keeping the
implementation that resets self.receive_dialog = ReceiveDialogState::default();
ensure the feature gate matches other test helpers in the PR (feature = "e2e").

---

Duplicate comments:
In `@Cargo.toml`:
- Around line 104-110: The Cargo.toml configuration is correct: keep the feature
declaration `e2e = []` and the test target block with name "e2e" and path
"tests/e2e/main.rs" as-is; no code changes are required because the E2E test
target follows the repo convention and has no runtime dependencies, enabling
`--all-features` runs successfully.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (7)

src/ui/mod.rs (1)

85-85: pub mod identities widens the public API unconditionally — consider gating behind the e2e feature.

The PR deliberately avoids the verbose #[cfg(feature = "e2e")] pub / #[cfg(not(feature = "e2e"))] pub(crate) pair, but making the whole identities module permanently pub exposes it to all downstream crates (not just test builds). The other production-only module wallets remains pub(crate), creating an inconsistency.

If this intentional trade-off is accepted, consider a short doc comment capturing that decision.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/mod.rs` at line 85, The identities module is being made public
unconditionally via the declaration pub mod identities which widens the public
API inconsistent with the production-only wallets module; either gate identities
behind the e2e feature (use cfg(feature = "e2e") so identities is exported only
when the e2e feature is enabled) or explicitly document the intentional API
exposure by adding a short doc comment above pub mod identities explaining why
it must remain public across builds; update the identities declaration or add
the doc comment accordingly to restore consistency with wallets and make the
intent clear.

tests/e2e/helpers/harness.rs (1)

296-332: handle_retry_error: settle steps run unconditionally on the final attempt before the terminal panic.

On the path where attempt == PLATFORM_MAX_RETRIES and the error is retryable, lines 320–324 still dismiss the dialog and run SETTLE_STEPS * attempt frames before the panic on line 327–331. Those frames are wasted since nothing reads their state. Consider short-circuiting the last-attempt panic earlier (before backoff):

♻️ Proposed simplification

     if !category.is_retryable() {
         panic!("{} failed with non-retryable error: {}", operation, error_text);
     }

+    if attempt == PLATFORM_MAX_RETRIES {
+        panic!(
+            "{} failed after {} retries. Last error: {}",
+            operation, PLATFORM_MAX_RETRIES, error_text
+        );
+    }
+
     dismiss_if_present(harness);
     if pop_screen {
         harness.state_mut().screen_stack.pop();
     }
     harness.run_steps(SETTLE_STEPS * attempt as usize);
-
-    if attempt == PLATFORM_MAX_RETRIES {
-        panic!(
-            "{} failed after {} retries. Last error: {}",
-            operation, PLATFORM_MAX_RETRIES, error_text
-        );
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/helpers/harness.rs` around lines 296 - 332, In handle_retry_error,
avoid running dismiss_if_present and harness.run_steps on the final attempt by
moving the terminal-attempt check (attempt == PLATFORM_MAX_RETRIES) to occur
before calling dismiss_if_present / pop_screen / harness.run_steps; if the
attempt equals PLATFORM_MAX_RETRIES, immediately panic with the same message
(using operation, PLATFORM_MAX_RETRIES and error_text) so you short-circuit the
wasted SETTLE_STEPS backoff instead of executing dismiss_if_present, popping the
screen, or calling harness.run_steps.

tests/e2e/phases/phase_07_teardown.rs (1)

26-30: Print the test summary before asserting SPV reached a terminal state.

If this assertion panics, the entire E2E summary block (lines 54–92) is skipped, making it harder to diagnose which phase produced useful results before teardown failed.

♻️ Proposed fix

+    // ─── 3a. Emit summary before any assertions so it's always visible ──
+    // (moved summary block here — see below)

     assert!(
         spv_stopped,
         "SPV must reach a terminal state after stop (still active after {}s)",
         SPV_STOP_TIMEOUT.as_secs()
     );

Move or duplicate the summary print block (or at least the key eprintln! lines) to before this assertion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/phases/phase_07_teardown.rs` around lines 26 - 30, Print the E2E
summary before asserting SPV reached a terminal state by moving or duplicating
the existing summary eprintln! lines (the summary print block that currently
runs later) to just before the assert! that checks spv_stopped; specifically,
emit the same eprintln! summary output prior to the assert!(spv_stopped, "SPV
must reach a terminal state after stop (still active after {}s)",
SPV_STOP_TIMEOUT.as_secs()) so that the summary is printed even if the assertion
panics.

tests/e2e/main.rs (1)

29-30: Nit: Phase banner says "Wallet UI + Balance Display" but calls phase_01_faucet.

The human-readable label suggests balance display, while the module name says "faucet." This could confuse someone reading test output vs. source. Consider aligning the banner with the module name or vice-versa.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/main.rs` around lines 29 - 30, The printed phase banner text "\n===
Phase 1: Wallet UI + Balance Display ===" is inconsistent with the invoked test
module phases::phase_01_faucet::run; update one to match the other so output and
implementation align—either change the banner string to reflect the faucet
(e.g., "Phase 1: Faucet") or rename/use the correct module/function
(phase_01_balance or similar) so the println! and phases::phase_01_faucet::run
refer to the same concept.

tests/e2e/phases/phase_06_dpns.rs (1)

77-91: The nth(1) selector to skip the breadcrumb is fragile.

If a UI refactor changes the ordering or adds another "Register Name" label, this silently clicks the wrong element. Consider querying by a more specific attribute (e.g., button role or a unique test id) if egui_kittest supports it, or at least add a comment anchoring what nth(0) and nth(1) correspond to today.

That said, the assertion on reg_count >= 2 provides a reasonable guard, so this is fine for now.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/phases/phase_06_dpns.rs` around lines 77 - 91, The test uses
query_all_by_label("Register Name").nth(1) which is fragile; replace the
positional selector with a more specific query (e.g., a button role or a test
id) or at minimum add a clarifying comment and stronger assertion: update the
call site that currently uses query_all_by_label("Register
Name").nth(1).expect(...).click() to instead target a unique selector supported
by egui_kittest (prefer a role-based or data-testid equivalent) or, if not
available, add a comment explaining that nth(0) is the breadcrumb and nth(1) is
the action button and tighten the pre-check (replace reg_count >= 2 with an
explicit check that the second element is a button) so the intent is clear and
the click cannot silently target the wrong element.

tests/e2e/phases/phase_00_setup.rs (1)

94-104: Mnemonic is validated twice — here and in main.rs.

The env var is read and validated both in main.rs (line 14) and here (line 96-97). The duplication is intentional (fail-fast in main + phase-local access), but worth a brief comment noting the redundancy is deliberate so a future maintainer doesn't remove the early check.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/phases/phase_00_setup.rs` around lines 94 - 104, The mnemonic/env
var validation in run (tests/e2e/phases/phase_00_setup.rs) duplicates the check
performed in main.rs; add a concise comment above the block in the run function
explaining this duplication is deliberate (fail-fast check in main + phase-local
validation) so future maintainers don't remove the early check; reference the
run function and the mnemonic variable/words validation to make the intent
clear.

tests/e2e/phases/phase_01_faucet.rs (1)

45-57: Balance parsing relies on exact label format — consider a note or regex.

The string-slicing approach (find("Balance:") → find("DASH") → trim().parse()) works for the current label format but will silently break if the format changes (e.g., extra spaces, currency symbol, thousands separator). This is acceptable for E2E tests, but a brief inline comment documenting the expected format (which is already partially there on line 43) helps future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/phases/phase_01_faucet.rs` around lines 45 - 57, Add a brief inline
comment above the ui_balance parsing block (referencing balance_label and the
ui_balance computation) that documents the exact expected label format
("Balance: <number> DASH"), noting that the code expects no thousands separators
and a decimal point for fractions; optionally mention that if format may change
in future consider switching to a regex. This clarifies assumptions for
maintainers without altering the current parsing logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/phases/phase_05_identity.rs`:
- Around line 50-70: The test currently can skip exercising the "no-key click
rejected" path when the action button is absent; update the test around
has_action_button so it either asserts the action button is present before
attempting the click (ensuring the rejection is exercised) or logs which branch
ran; specifically modify the block that computes has_action_button and the
subsequent branch that clicks (referencing has_action_button,
query_all_by_label("Create Identity"), set_wallet_funded_ready, and
assert_identity_step with WalletFundedScreenStep::ReadyToCreate) to fail the
test if the button is missing or to add a distinct println! indicating "action
button absent, click skipped" so CI output shows whether the click was
exercised.
- Around line 11-25: The client-side validation tests are currently gated by
ensure_spv_tx_ready; move the call to ensure_spv_tx_ready so
run_validation_tests(harness) executes unconditionally first in the run function
(keep the identity-creation skip prints as-is). Specifically, in the run
function that accepts Harness<'_, AppState> and TestContext, call
run_validation_tests(harness) before calling ensure_spv_tx_ready(harness, ctx)
so the pure client-side checks run regardless of SPV sync state.

---

Duplicate comments:
In `@tests/e2e/phases/phase_04_tokens.rs`:
- Around line 48-63: The timeout handling in phase_04_tokens.rs is correct:
ensure the code using TOKEN_SEARCH_TIMEOUT, PLATFORM_MAX_RETRIES, attempt, and
completed continues to retry when !completed and only calls panic when attempt
== PLATFORM_MAX_RETRIES; no code change required—leave the println/continue and
the conditional panic as-is to preserve retry behavior.

---

Nitpick comments:
In `@src/ui/mod.rs`:
- Line 85: The identities module is being made public unconditionally via the
declaration pub mod identities which widens the public API inconsistent with the
production-only wallets module; either gate identities behind the e2e feature
(use cfg(feature = "e2e") so identities is exported only when the e2e feature is
enabled) or explicitly document the intentional API exposure by adding a short
doc comment above pub mod identities explaining why it must remain public across
builds; update the identities declaration or add the doc comment accordingly to
restore consistency with wallets and make the intent clear.

In `@tests/e2e/helpers/harness.rs`:
- Around line 296-332: In handle_retry_error, avoid running dismiss_if_present
and harness.run_steps on the final attempt by moving the terminal-attempt check
(attempt == PLATFORM_MAX_RETRIES) to occur before calling dismiss_if_present /
pop_screen / harness.run_steps; if the attempt equals PLATFORM_MAX_RETRIES,
immediately panic with the same message (using operation, PLATFORM_MAX_RETRIES
and error_text) so you short-circuit the wasted SETTLE_STEPS backoff instead of
executing dismiss_if_present, popping the screen, or calling harness.run_steps.

In `@tests/e2e/main.rs`:
- Around line 29-30: The printed phase banner text "\n=== Phase 1: Wallet UI +
Balance Display ===" is inconsistent with the invoked test module
phases::phase_01_faucet::run; update one to match the other so output and
implementation align—either change the banner string to reflect the faucet
(e.g., "Phase 1: Faucet") or rename/use the correct module/function
(phase_01_balance or similar) so the println! and phases::phase_01_faucet::run
refer to the same concept.

In `@tests/e2e/phases/phase_00_setup.rs`:
- Around line 94-104: The mnemonic/env var validation in run
(tests/e2e/phases/phase_00_setup.rs) duplicates the check performed in main.rs;
add a concise comment above the block in the run function explaining this
duplication is deliberate (fail-fast check in main + phase-local validation) so
future maintainers don't remove the early check; reference the run function and
the mnemonic variable/words validation to make the intent clear.

In `@tests/e2e/phases/phase_01_faucet.rs`:
- Around line 45-57: Add a brief inline comment above the ui_balance parsing
block (referencing balance_label and the ui_balance computation) that documents
the exact expected label format ("Balance: <number> DASH"), noting that the code
expects no thousands separators and a decimal point for fractions; optionally
mention that if format may change in future consider switching to a regex. This
clarifies assumptions for maintainers without altering the current parsing
logic.

In `@tests/e2e/phases/phase_06_dpns.rs`:
- Around line 77-91: The test uses query_all_by_label("Register Name").nth(1)
which is fragile; replace the positional selector with a more specific query
(e.g., a button role or a test id) or at minimum add a clarifying comment and
stronger assertion: update the call site that currently uses
query_all_by_label("Register Name").nth(1).expect(...).click() to instead target
a unique selector supported by egui_kittest (prefer a role-based or data-testid
equivalent) or, if not available, add a comment explaining that nth(0) is the
breadcrumb and nth(1) is the action button and tighten the pre-check (replace
reg_count >= 2 with an explicit check that the second element is a button) so
the intent is clear and the click cannot silently target the wrong element.

In `@tests/e2e/phases/phase_07_teardown.rs`:
- Around line 26-30: Print the E2E summary before asserting SPV reached a
terminal state by moving or duplicating the existing summary eprintln! lines
(the summary print block that currently runs later) to just before the assert!
that checks spv_stopped; specifically, emit the same eprintln! summary output
prior to the assert!(spv_stopped, "SPV must reach a terminal state after stop
(still active after {}s)", SPV_STOP_TIMEOUT.as_secs()) so that the summary is
printed even if the assertion panics.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/e2e/helpers/harness.rs (1)

372-376: ensure_spv_tx_ready: .unwrap() on RwLock reads gives opaque panic messages.

Line 374 already uses a descriptive .expect(...) for the wallet lookup; the two .unwrap() calls on the surrounding locks are inconsistent and would emit a cryptic "poisoned lock" message with no context if they ever fail.

♻️ Proposed fix

-    let wallets = app_ctx.wallets().read().unwrap();
+    let wallets = app_ctx
+        .wallets()
+        .read()
+        .expect("wallets RwLock poisoned in ensure_spv_tx_ready");
     let wallet = wallets
         .get(ctx.seed_hash())
         .expect("Test wallet must exist in AppContext during tx phases");
-    let w = wallet.read().unwrap();
+    let w = wallet.read().expect("wallet RwLock poisoned in ensure_spv_tx_ready");

Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool codebase, particularly to avoid panics with .expect() and instead propagate errors properly" — while this is test code where panicking is acceptable, using .expect() with descriptive messages is still preferable over .unwrap() for diagnostic clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/helpers/harness.rs` around lines 372 - 376, Replace the cryptic
.unwrap() calls on the RwLock reads in ensure_spv_tx_ready with descriptive
.expect(...) messages: when reading app_ctx.wallets() replace .read().unwrap()
with .read().expect("failed to acquire read lock on AppContext wallets RwLock in
ensure_spv_tx_ready"), and when reading the returned wallet replace
.read().unwrap() with .read().expect("failed to acquire read lock on Wallet in
ensure_spv_tx_ready"); keep the existing .expect(...) on
wallets.get(ctx.seed_hash()) as-is so all lock failures produce clear diagnostic
messages referencing ensure_spv_tx_ready, app_ctx.wallets() and wallet.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/helpers/harness.rs`:
- Around line 325-343: Move the final-attempt panic check so it occurs before
wasting frames in harness.run_steps: after calling dismiss_if_present(harness)
and optionally popping the screen (harness.state_mut().screen_stack.pop()),
check if attempt == PLATFORM_MAX_RETRIES and panic with the same message if
true; only call harness.run_steps(SETTLE_STEPS * attempt as usize) when not on
the final retry. This keeps the existing dismiss/pop behavior but avoids running
extra frames before the final panic.

---

Nitpick comments:
In `@tests/e2e/helpers/harness.rs`:
- Around line 372-376: Replace the cryptic .unwrap() calls on the RwLock reads
in ensure_spv_tx_ready with descriptive .expect(...) messages: when reading
app_ctx.wallets() replace .read().unwrap() with .read().expect("failed to
acquire read lock on AppContext wallets RwLock in ensure_spv_tx_ready"), and
when reading the returned wallet replace .read().unwrap() with
.read().expect("failed to acquire read lock on Wallet in ensure_spv_tx_ready");
keep the existing .expect(...) on wallets.get(ctx.seed_hash()) as-is so all lock
failures produce clear diagnostic messages referencing ensure_spv_tx_ready,
app_ctx.wallets() and wallet.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/kittest/interactions.rs (1)

29-33: enable_welcome_screen hardcodes mainnet_app_context regardless of current network.

If the app is configured for testnet/devnet, the welcome screen created here always gets mainnet context. For the current UI-text/click tests this is benign, but it's a subtle divergence. The current_app_context() method already exists on AppState and returns &Arc<AppContext>, respecting the configured network via the chosen_network field.

♻️ Suggested alternative

 fn enable_welcome_screen(harness: &mut Harness<'_, dash_evo_tool::app::AppState>) {
-    let ctx = harness.state().mainnet_app_context.clone();
+    let ctx = harness.state().current_app_context().clone();
     harness.state_mut().show_welcome_screen = true;
     harness.state_mut().welcome_screen = Some(WelcomeScreen::new(ctx));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/kittest/interactions.rs` around lines 29 - 33, enable_welcome_screen
currently always uses mainnet_app_context; change it to use
AppState::current_app_context() so it respects chosen_network. In the function
enable_welcome_screen (and where it calls WelcomeScreen::new(ctx)), replace
harness.state().mainnet_app_context.clone() with
harness.state().current_app_context().clone() (or obtain a &Arc via
current_app_context() and clone that) and keep the existing assignments to
show_welcome_screen and welcome_screen.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/main.rs`:
- Line 9: Update the doc comment that shows the run command so it includes the
Cargo feature flag: change the existing example "E2E_WALLET_MNEMONIC=\"...\"
cargo test --test e2e -- --ignored --nocapture" to include --all-features before
the test-args delimiter (i.e. "cargo test --test e2e --all-features -- --ignored
--nocapture") so the e2e feature is enabled when running the test; edit the run
command string in the tests/e2e/main.rs doc comment accordingly.

---

Nitpick comments:
In `@tests/kittest/interactions.rs`:
- Around line 29-33: enable_welcome_screen currently always uses
mainnet_app_context; change it to use AppState::current_app_context() so it
respects chosen_network. In the function enable_welcome_screen (and where it
calls WelcomeScreen::new(ctx)), replace
harness.state().mainnet_app_context.clone() with
harness.state().current_app_context().clone() (or obtain a &Arc via
current_app_context() and clone that) and keep the existing assignments to
show_welcome_screen and welcome_screen.

[coderabbitai]

🧹 Nitpick comments (2)

src/ui/wallets/wallets_screen/dialogs.rs (1)

170-172: Consider using DashColors::ERROR for theme consistency.

The send_dialog.error block a few lines below (Lines 195–213) uses DashColors::ERROR for the same semantic role. The address-error label here stays on a raw (255, 100, 100) with no dark-mode awareness, which is a minor inconsistency.

🎨 Proposed alignment with the theme

+        let dark_mode = ctx.style().visuals.dark_mode;
         if let Some(error) = &self.send_dialog.address_error {
-            ui.colored_label(egui::Color32::from_rgb(255, 100, 100), error);
+            ui.colored_label(DashColors::error_color(dark_mode), error);
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/wallets/wallets_screen/dialogs.rs` around lines 170 - 172, Replace the
hard-coded color in the address-error label with the themed color constant:
where you call ui.colored_label(...) for self.send_dialog.address_error, use
DashColors::ERROR (the same theme used by the send_dialog.error block) so the
label respects dark/light modes and theme consistency; update the call around
send_dialog.address_error to use DashColors::ERROR instead of
Color32::from_rgb(255, 100, 100).

tests/e2e/main.rs (1)

44-47: Consider a brief // TODO linking Phase 6 to a tracking issue.

The skip comment explains why Phase 6 is disabled, but there's no reference to a tracking issue or milestone for re-enabling it once SPV mempool support lands. A // TODO(#issue) would make it discoverable and prevent it from going stale.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/main.rs` around lines 44 - 47, Add a short TODO comment next to the
Phase 6 skip comment/println that references the issue or milestone that will
track re-enabling DPNS name registration (e.g., // TODO(`#1234`): re-enable Phase
6 when SPV mempool support lands or include the issue URL); place it immediately
above or on the same line as the println!("\n=== Phase 6: DPNS Name Registration
— SKIPPED ===") so the skip is discoverable and linked to the tracking ticket.

🤖 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/ui/wallets/wallets_screen/dialogs.rs`:
- Around line 170-172: Replace the hard-coded color in the address-error label
with the themed color constant: where you call ui.colored_label(...) for
self.send_dialog.address_error, use DashColors::ERROR (the same theme used by
the send_dialog.error block) so the label respects dark/light modes and theme
consistency; update the call around send_dialog.address_error to use
DashColors::ERROR instead of Color32::from_rgb(255, 100, 100).

In `@tests/e2e/main.rs`:
- Around line 44-47: Add a short TODO comment next to the Phase 6 skip
comment/println that references the issue or milestone that will track
re-enabling DPNS name registration (e.g., // TODO(`#1234`): re-enable Phase 6 when
SPV mempool support lands or include the issue URL); place it immediately above
or on the same line as the println!("\n=== Phase 6: DPNS Name Registration —
SKIPPED ===") so the skip is discoverable and linked to the tracking ticket.

[thepastaclaw]

Clippy failure is a stale cache issue — the Color32 import was already removed in 62d88875 (confirmed: baa746b9:dialogs.rs:19 no longer has it). CI just needs a re-run.

/rerun

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/ui/identities/add_new_identity_screen/mod.rs (1)

95-95: Keep test-only identity state encapsulated behind e2e accessors.

Line 95 widens successful_qualified_identity_id to pub(crate) for all builds. Prefer keeping the field private and exposing a feature-gated getter so non-e2e code can’t start depending on internal state.

Proposed change

-    pub(crate) successful_qualified_identity_id: Option<Identifier>,
+    successful_qualified_identity_id: Option<Identifier>,

 #[cfg(feature = "e2e")]
 impl AddNewIdentityScreen {
+    pub fn successful_qualified_identity_id(&self) -> Option<&Identifier> {
+        self.successful_qualified_identity_id.as_ref()
+    }
+
     pub fn step(&self) -> &Arc<RwLock<WalletFundedScreenStep>> {
         &self.step
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/identities/add_new_identity_screen/mod.rs` at line 95, The field
successful_qualified_identity_id is currently exposed pub(crate) but should be
private; make it private (remove pub(crate)) and add a feature-gated accessor
for e2e tests: add a getter method named successful_qualified_identity_id on the
same struct annotated with #[cfg(feature = "e2e")] and make it pub(crate) (or
public as needed for tests) that returns the field (either by cloning
Option<Identifier> or returning Option<&Identifier> depending on Identifier's
ownership). This keeps the field encapsulated while allowing only code compiled
with the "e2e" feature to read it.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Cargo.toml`:
- Line 90: The unexpected_cfgs whitelist currently only allows "testing" and
causes warnings for uses of #[cfg(feature = "e2e")]; update the unexpected_cfgs
configuration to include "e2e" in the whitelist array (i.e., add the string
"e2e" alongside "testing" in the whitelist entry referenced by the
unexpected_cfgs config in Cargo.toml) so the feature is allowed where used in
src/ui/... and src/context/mod.rs.

---

Nitpick comments:
In `@src/ui/identities/add_new_identity_screen/mod.rs`:
- Line 95: The field successful_qualified_identity_id is currently exposed
pub(crate) but should be private; make it private (remove pub(crate)) and add a
feature-gated accessor for e2e tests: add a getter method named
successful_qualified_identity_id on the same struct annotated with #[cfg(feature
= "e2e")] and make it pub(crate) (or public as needed for tests) that returns
the field (either by cloning Option<Identifier> or returning Option<&Identifier>
depending on Identifier's ownership). This keeps the field encapsulated while
allowing only code compiled with the "e2e" feature to read it.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between baa746b and 36c6c0f.

📒 Files selected for processing (8)

• Cargo.toml
• src/context/mod.rs
• src/ui/contracts_documents/add_contracts_screen.rs
• src/ui/identities/add_existing_identity_screen.rs
• src/ui/identities/add_new_identity_screen/mod.rs
• src/ui/identities/register_dpns_name_screen.rs
• src/ui/mod.rs
• tests/kittest/main.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• src/ui/identities/register_dpns_name_screen.rs
• tests/kittest/main.rs
• src/ui/contracts_documents/add_contracts_screen.rs
• src/ui/mod.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the Cargo.toml file, specifically around the lines mentioned
cat -n Cargo.toml | sed -n '85,120p'

Repository: dashpay/dash-evo-tool

Length of output: 1908

---

🏁 Script executed:

# Search for any usage of #[cfg(feature = "e2e")] in the codebase
rg '#\[cfg\(feature\s*=\s*"e2e"' -A 2

Repository: dashpay/dash-evo-tool

Length of output: 979

---

🏁 Script executed:

# Also search for cfg! macro usage with e2e feature
rg 'cfg!\s*\(\s*feature\s*=\s*"e2e"' -A 2

Repository: dashpay/dash-evo-tool

Length of output: 47

---

Whitelist the new e2e feature in unexpected_cfgs checks.

The codebase uses #[cfg(feature = "e2e")] in multiple source files (src/ui/wallets/import_mnemonic_screen.rs, src/ui/identities/register_dpns_name_screen.rs, src/ui/identities/add_new_identity_screen/mod.rs, src/context/mod.rs), but line 116 only whitelists "testing". This triggers unexpected_cfgs warnings and will fail stricter CI runs.

Proposed fix

 [lints.rust.unexpected_cfgs]
 level = "warn"
-check-cfg = ["cfg(tokio_unstable)", "cfg(feature, values(\"testing\"))"]
+check-cfg = ["cfg(tokio_unstable)", "cfg(feature, values(\"testing\", \"e2e\"))"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Cargo.toml` at line 90, The unexpected_cfgs whitelist currently only allows
"testing" and causes warnings for uses of #[cfg(feature = "e2e")]; update the
unexpected_cfgs configuration to include "e2e" in the whitelist array (i.e., add
the string "e2e" alongside "testing" in the whitelist entry referenced by the
unexpected_cfgs config in Cargo.toml) so the feature is allowed where used in
src/ui/... and src/context/mod.rs.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/e2e.yml:
- Line 67: Replace the direct cargo invocation with the repository-safe wrapper:
instead of running the command string 'cargo test --features="e2e" --test e2e --
--ignored --nocapture | tee /tmp/e2e.log', invoke the wrapper script
'scripts/safe-cargo.sh' to execute the exact same cargo arguments (e.g. run
'scripts/safe-cargo.sh cargo test --features="e2e" --test e2e -- --ignored
--nocapture | tee /tmp/e2e.log'); update the workflow step that currently runs
the 'cargo test ...' command so all E2E tests use the safe-cargo.sh wrapper.
- Line 34: Replace the deprecated GitHub Action usage by updating the workflow
step that references actions-rs/toolchain@v1 to use
dtolnay/rust-toolchain@stable instead; locate the uses: entry for
actions-rs/toolchain@v1 in the e2e.yml workflow and swap it to
dtolnay/rust-toolchain@stable so the action runs on supported runners.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36c6c0f and d77c384.

📒 Files selected for processing (1)

• .github/workflows/e2e.yml

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

actions-rs/toolchain@v1 deprecated incompatible GitHub Actions runners 2024 2025

💡 Result:

• actions-rs/toolchain@v1 is an archived/unmaintained action (repo archived Oct 13, 2023) so it has not been updated for runner-runtime changes. [1]
• Its action.yml declares runs: using: 'node12', meaning it depends on the Node 12 JavaScript action runtime. [2]
• GitHub Actions deprecated Node 12 and began enforcing Node 16 instead of Node 12 starting June 14, 2023 (with only a temporary opt-out via ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION). This is why actions-rs/toolchain@v1 started triggering deprecation warnings / breakage on newer runners. [3]
• GitHub then moved from Node 16 → Node 20 (default switch June 30, 2024) and ultimately removed Node 16 from runners on Nov 12, 2024, further tightening compatibility for old JavaScript actions. [4] [5]

What to do instead: migrate off actions-rs/toolchain@v1 to a maintained Rust toolchain installer such as dtolnay/rust-toolchain. [6]

Sources: [1] [2] [3] [4] [5] [6]

---

🌐 Web query:

dtolnay rust-toolchain GitHub Actions recommended replacement for actions-rs

💡 Result:

The commonly recommended replacement for the deprecated actions-rs/toolchain action is dtolnay/rust-toolchain. (github.com)

Example:

steps:
  - uses: actions/checkout@v5
  - uses: dtolnay/rust-toolchain@stable   # or `@nightly`, or `@1.89.0`, etc.
  - run: cargo test --all-features

dtolnay/rust-toolchain installs Rust via rustup, and you can add components / targets via inputs if needed. (github.com)

Citations:

• 1: https://github.com/dtolnay/rust-toolchain?utm_source=openai
• 2: https://github.com/dtolnay/rust-toolchain?utm_source=openai

---

Replace actions-rs/toolchain@v1 with dtolnay/rust-toolchain@stable.

The actions-rs/toolchain repository was archived on October 13, 2023 and relies on the Node 12 JavaScript action runtime. GitHub Actions removed support for Node 12 on November 12, 2024, causing this action to fail on current runners. Use the maintained dtolnay/rust-toolchain@stable action instead:

- uses: dtolnay/rust-toolchain@stable

🧰 Tools

🪛 actionlint (1.7.11)

[error] 34-34: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e.yml at line 34, Replace the deprecated GitHub Action
usage by updating the workflow step that references actions-rs/toolchain@v1 to
use dtolnay/rust-toolchain@stable instead; locate the uses: entry for
actions-rs/toolchain@v1 in the e2e.yml workflow and swap it to
dtolnay/rust-toolchain@stable so the action runs on supported runners.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use scripts/safe-cargo.sh for the E2E test command.

Line 67 calls cargo directly, which violates the workflow security guideline and bypasses CI secret scrubbing.

🔧 Proposed fix

-          cargo test --features="e2e" --test e2e -- --ignored --nocapture | tee /tmp/e2e.log
+          scripts/safe-cargo.sh test --test e2e --all-features -- --ignored --nocapture | tee /tmp/e2e.log

As per coding guidelines: "Use scripts/safe-cargo.sh in GitHub Actions instead of calling cargo directly to prevent build scripts from accessing CI credentials."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e.yml at line 67, Replace the direct cargo invocation
with the repository-safe wrapper: instead of running the command string 'cargo
test --features="e2e" --test e2e -- --ignored --nocapture | tee /tmp/e2e.log',
invoke the wrapper script 'scripts/safe-cargo.sh' to execute the exact same
cargo arguments (e.g. run 'scripts/safe-cargo.sh cargo test --features="e2e"
--test e2e -- --ignored --nocapture | tee /tmp/e2e.log'); update the workflow
step that currently runs the 'cargo test ...' command so all E2E tests use the
safe-cargo.sh wrapper.

[thepastaclaw]

Review Gate

Commit: d77c384e

• ✅ Debounce: 71052m ago (need 30m)

• ⏳ CI checks: build failure: Clippy Report, Clippy, Test Suite

• ✅ CodeRabbit review: comment found

• ✅ Off-peak hours: off-peak (06:44 PM PT Thursday)

• Run review now (check to override)