refactor: split context.rs into focused submodules

[PastaPastaPasta]

Summary

• Splits the monolithic 1795-line context.rs into a context/ module with 5 focused submodules
• Each submodule groups related AppContext methods by concern:
  • mod.rs (543 lines): struct definition, constructor, config/SDK/fee methods
  • wallet_lifecycle.rs (572 lines): SPV management, wallet bootstrap, address registration, reconciliation
  • identity_db.rs (205 lines): identity CRUD, DPNS voting, contested names
  • contract_token_db.rs (198 lines): contract/token database operations, wallet removal
  • settings_db.rs (84 lines): settings cache and persistence
  • transaction_processing.rs (231 lines): transaction/asset lock finality handling, DAPI queries
• Pure extraction — no logic changes, no behavior changes

Test plan

• cargo build — clean, zero warnings
• cargo clippy --all-features --all-targets -- -D warnings — passes
• cargo fmt --all -- --check — passes
• Verify app starts and connects normally
• Verify wallet operations (create, unlock, lock) work as before

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Reorganized internal application context structure into focused modules for improved code maintainability and organization. Core functionality for wallet management, identity handling, contract operations, settings management, and transaction processing remains unchanged.

[coderabbitai]

📝 Walkthrough

Walkthrough

The AppContext module was refactored from a single 1795-line monolithic file into a modular structure with separate files for domain-specific functionality: contract/token operations, identity/DPNS management, settings caching, transaction processing, and wallet/SPV lifecycle orchestration.

Changes

Cohort / File(s)	Summary
AppContext Core Structure src/context/mod.rs	Reintroduces the AppContext struct with initialization logic, backend switching (SPV/RPC), fee estimation, provider binding, and platform version mapping. Centralizes runtime state management.
Data Access Layers src/context/contract_token_db.rs, src/context/identity_db.rs, src/context/settings_db.rs	New modules providing public API methods for contract/token CRUD operations, identity/alias/DPNS management with wallet loading, and settings persistence with caching/invalidation logic.
Transaction & Wallet Processing src/context/transaction_processing.rs, src/context/wallet_lifecycle.rs	New modules handling transaction finality updates, asset lock processing, DAPI transaction lookups, and comprehensive SPV/wallet lifecycle (unlock/lock, address sync, reconciliation, bootstrapping).
Original Context File src/context.rs	Entire file removed (1795 lines). Functionality redistributed across new modular files in the context subdirectory.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Poem

🐰 One big file became a warren,
Modules tucked in organized cells,
Wallets, identities, tokens all sorted,
AppContext refactored and well—
A burrow of clarity, hopping with purpose! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.32% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: splitting a large context.rs file into focused submodules, which is the primary objective of this refactoring PR.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr/context-refactor

---

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: 3

🤖 Fix all issues with AI agents

In `@src/context/contract_token_db.rs`:
- Around line 122-128: The serialization block that calls
bincode::encode_to_vec(&token_configuration, config) currently swallows errors
and returns Ok(()) on failure; change it to propagate the serialization error
(or log it and return Err) instead of returning success: replace the .ok() +
else { return Ok(()); } pattern with proper error handling (e.g., map_err to
convert the bincode error into the function's error type and return Err after
logging with the module/process logger). Do the same for the duplicated pattern
in the contracts serialization code (the bincode::encode_to_vec usage in the
other file) so callers cannot believe the token/contract was persisted when
serialization failed.

In `@src/context/mod.rs`:
- Around line 235-238: AppContext is being initialized with devnet_name set to
None; instead set it from the loaded network_config by assigning devnet_name:
network_config.devnet_name.clone(). Update the AppContext initializer (where
AppContext { network, developer_mode: AtomicBool::new(developer_mode_enabled),
devnet_name: ... }) to use network_config.devnet_name.clone() so the context
matches other uses (e.g., core_cookie_path) and correctly carries the devnet
identification.
- Around line 212-224: Replace the brittle tuple index access in the
saved_core_backend_mode computation by destructuring the tuple returned from
get_settings(), e.g. map(|(field1, field2, field3, field4, field5, field6,
field7, core_backend_mode, field9, field10, field11, field12)|
core_backend_mode) so you explicitly extract the core_backend_mode value; keep
the surrounding chain
(db.get_settings().ok().flatten().map(...).unwrap_or(CoreBackendMode::Rpc.as_u8()))
and the developer_mode_enabled conditional that forces RPC mode, and reference
the same symbols (get_settings, saved_core_backend_mode, CoreBackendMode,
developer_mode_enabled) when making the change.

🧹 Nitpick comments (10)

src/context/transaction_processing.rs (1)

66-99: Silently discarded DB errors for DashPay payment records.

Lines 74 and 82 use let _ = to discard errors from update_highest_receive_index and save_payment. While this appears to be pre-existing behavior being moved, silently dropping these errors makes debugging payment-tracking issues difficult. Consider at least logging when these operations fail.

Example: log instead of discard

-                    let _ = self.db.update_highest_receive_index(
+                    if let Err(e) = self.db.update_highest_receive_index(
                         &owner_id,
                         &contact_id,
                         address_index + 1,
-                    );
+                    ) {
+                        tracing::warn!("Failed to update highest receive index: {}", e);
+                    }

src/context/contract_token_db.rs (2)

24-66: Unnecessary Arc::clone before .as_ref().clone().

Arc::clone(&self.dpns_contract).as_ref().clone() bumps the reference count only to immediately dereference and deep-clone the DataContract. You can simplify to (*self.dpns_contract).clone() (or self.dpns_contract.as_ref().clone()). This pattern is repeated for all five system contracts.

Example for one contract (apply to all five)

-        let dpns_contract = QualifiedContract {
-            contract: Arc::clone(&self.dpns_contract).as_ref().clone(),
-            alias: Some("dpns".to_string()),
-        };
+        let dpns_contract = QualifiedContract {
+            contract: (*self.dpns_contract).clone(),
+            alias: Some("dpns".to_string()),
+        };

---

146-173: TOCTOU in remove_wallet: existence check and removal use separate lock acquisitions.

The read lock (line 148) verifies the wallet exists, but the write lock (line 161) is acquired later. Between the two, another thread could remove the same wallet. In practice this is low-risk since wallet removal is a rare user action, but worth noting. A simpler approach would be to acquire the write lock once upfront.

Simplified single-lock approach

     pub fn remove_wallet(&self, seed_hash: &WalletSeedHash) -> Result<(), String> {
-        {
-            let wallets = self
-                .wallets
-                .read()
-                .map_err(|_| "Failed to access wallets".to_string())?;
-            if !wallets.contains_key(seed_hash) {
-                return Err("Wallet not found".to_string());
-            }
-        }
-
         self.db
             .remove_wallet(seed_hash, &self.network)
             .map_err(|e| e.to_string())?;
 
         let mut wallets = self
             .wallets
             .write()
             .map_err(|_| "Failed to update wallets".to_string())?;
 
-        wallets.remove(seed_hash);
+        if wallets.remove(seed_hash).is_none() {
+            // Already removed or never existed in memory; DB removal above is idempotent
+        }
         let has_wallet = !wallets.is_empty();
         drop(wallets);
 
         self.has_wallet.store(has_wallet, Ordering::Relaxed);
 
         Ok(())
     }

src/context/mod.rs (2)

106-112: Use tracing instead of println! for config load failure.

Line 109 uses println! while the rest of the codebase consistently uses tracing::error! / tracing::info!. Even if tracing isn't initialized at this point, tracing gracefully drops log records when no subscriber is set.

Proposed fix

-                println!("Failed to load config: {e}");
+                tracing::error!("Failed to load config: {e}");

---

534-543: default_platform_version wildcard arm panics at runtime.

While this is marked with a TODO to use sdk.version() instead, the _ => panic!("unsupported network") arm in a const fn will cause a runtime panic for any new Network variant. Since Network is an external enum that could gain variants, consider returning a default or using compile_error! if possible.

src/context/identity_db.rs (2)

116-137: load_wallet_for_identity never returns Err.

The function returns Result<()> but always returns Ok(()). If the intent is to log-and-continue on missing wallets (which it does), the return type could be (). However, if this is a pre-existing signature to allow future error propagation, it's fine to keep.

---

150-152: Change delete_scheduled_vote parameter from &String to &str to match the database layer.

The wrapper method currently accepts &String but passes it to the DB method that expects &str (line 117 of src/database/scheduled_votes.rs). This causes unnecessary type coercion. Remove the #[allow(clippy::ptr_arg)] suppress and align the wrapper signature with the DB layer.

For consistency, the insert_scheduled_votes method could also be improved to accept &[ScheduledDPNSVote] instead of &Vec<ScheduledDPNSVote>, but this would require a coordinated change in src/database/scheduled_votes.rs as well.

Proposed fix for delete_scheduled_vote

     /// Deletes a scheduled vote from the database
-    #[allow(clippy::ptr_arg)]
-    pub fn delete_scheduled_vote(&self, identity_id: &[u8], contested_name: &String) -> Result<()> {
+    pub fn delete_scheduled_vote(&self, identity_id: &[u8], contested_name: &str) -> Result<()> {
         self.db
             .delete_scheduled_vote(self, identity_id, contested_name)
     }

src/context/wallet_lifecycle.rs (3)

22-42: rusqlite::Result<(), String> is misleading.

rusqlite::Result<T, E> with an explicit String error type resolves to std::result::Result<(), String>, which has nothing to do with rusqlite errors. Using Result<(), String> directly would be clearer and consistent with other methods in the file (e.g., start_spv on line 44).

Proposed fix

-    pub fn clear_spv_data(&self) -> rusqlite::Result<(), String> {
+    pub fn clear_spv_data(&self) -> Result<(), String> {
         self.spv_manager.clear_data_dir()
     }

-    pub fn clear_network_database(&self) -> rusqlite::Result<(), String> {
+    pub fn clear_network_database(&self) -> Result<(), String> {

---

379-404: Debounce logic branches are nearly identical — can be simplified.

Both the "elapsed > 300ms" and the "sleep then reconcile" paths do the same reconcile + timestamp reset. A simpler approach: always sleep for the remainder of the debounce window, then reconcile.

Simplified debounce

             loop {
                 tokio::select! {
                     maybe = rx.recv() => {
                         if maybe.is_none() { break; }
-                        // simple debounce window
-                        if last.elapsed() > Duration::from_millis(300) {
-                            if let Err(e) = ctx.reconcile_spv_wallets().await { tracing::debug!("SPV reconcile error: {}", e); }
-                            last = Instant::now();
-                        } else {
-                            sleep(Duration::from_millis(300)).await;
-                            if let Err(e) = ctx.reconcile_spv_wallets().await { tracing::debug!("SPV reconcile error: {}", e); }
-                            last = Instant::now();
-                        }
+                        let remaining = Duration::from_millis(300).saturating_sub(last.elapsed());
+                        if !remaining.is_zero() {
+                            sleep(remaining).await;
+                        }
+                        if let Err(e) = ctx.reconcile_spv_wallets().await {
+                            tracing::debug!("SPV reconcile error: {}", e);
+                        }
+                        last = Instant::now();
                     }
                 }
             }

---

452-457: Add a delete_utxos_for_address method to the Database abstraction.

The raw SQL at lines 452-457 bypasses the database abstraction layer. The Database type provides encapsulated methods for other UTXO operations (insert_utxo, drop_utxo, get_utxos_by_address), but bulk-deletion by address is handled with direct SQL instead. Consolidate this into a pub(crate) fn delete_utxos_for_address(&self, address: &str, network: &str) -> rusqlite::Result<()> method in src/database/utxo.rs, keeping the implementation consistent with the existing UTXO API.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find Settings struct definition
rg -n "struct Settings" --type rust -A 20

Repository: dashpay/dash-evo-tool

Length of output: 1422

---

🏁 Script executed:

#!/bin/bash
# Find get_settings function
rg -n "fn get_settings" --type rust -A 5

Repository: dashpay/dash-evo-tool

Length of output: 818

---

🏁 Script executed:

#!/bin/bash
# Check context around line 216 in src/context/mod.rs
sed -n '210,225p' src/context/mod.rs

Repository: dashpay/dash-evo-tool

Length of output: 643

---

🏁 Script executed:

#!/bin/bash
# Search for other tuple index accesses on Settings to see if this pattern is used elsewhere
rg -n "\.([0-9])" src/context/mod.rs --type rust | grep -E "\.(0|[1-9])"

Repository: dashpay/dash-evo-tool

Length of output: 144

---

🏁 Script executed:

#!/bin/bash
# Get the full return type of database get_settings function
sed -n '534,560p' src/database/settings.rs

Repository: dashpay/dash-evo-tool

Length of output: 1097

---

Access core_backend_mode via tuple destructuring instead of fragile index s.7.

The database's get_settings() returns a 12-element tuple, and accessing the 8th element by index (s.7) is brittle—if a field is added, removed, or reordered in the tuple, this silently reads the wrong value. Destructure the tuple instead to make the intent explicit and maintain correctness if the tuple structure changes.

🤖 Prompt for AI Agents

In `@src/context/mod.rs` around lines 212 - 224, Replace the brittle tuple index
access in the saved_core_backend_mode computation by destructuring the tuple
returned from get_settings(), e.g. map(|(field1, field2, field3, field4, field5,
field6, field7, core_backend_mode, field9, field10, field11, field12)|
core_backend_mode) so you explicitly extract the core_backend_mode value; keep
the surrounding chain
(db.get_settings().ok().flatten().map(...).unwrap_or(CoreBackendMode::Rpc.as_u8()))
and the developer_mode_enabled conditional that forces RPC mode, and reference
the same symbols (get_settings, saved_core_backend_mode, CoreBackendMode,
developer_mode_enabled) when making the change.

[PastaPastaPasta]

this PR shouldn't change behavior; move only

[coderabbitai]

@PastaPastaPasta, you're absolutely right—this PR is a pure code movement refactor and shouldn't include behavioral or stylistic changes beyond the extraction itself. The tuple destructuring suggestion, while valid for improving maintainability, would go beyond the scope of a move-only refactor.

Would you like me to open a follow-up issue to track this improvement for a future PR? That way, the technical debt can be addressed separately without mixing concerns.

---

✏️ Learnings added

Learnt from: PastaPastaPasta
Repo: dashpay/dash-evo-tool PR: 543
File: src/context/mod.rs:212-224
Timestamp: 2026-02-10T04:19:14.918Z
Learning: PR `#543` is a move-only refactor that splits context.rs into submodules without changing logic or code style. Suggestions that improve code quality but alter the extracted code should be deferred to follow-up PRs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: lklimek
Repo: dashpay/dash-evo-tool PR: 264
File: src/database/wallet.rs:95-99
Timestamp: 2025-05-13T06:55:34.019Z
Learning: Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with `.expect()` and instead propagate errors properly using the `?` operator.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check how devnet_name is used in the codebase
rg -n "devnet_name" --type rust -C 3

Repository: dashpay/dash-evo-tool

Length of output: 4637

---

🏁 Script executed:

# Look at the AppContext struct definition
rg -n "struct AppContext" --type rust -A 15

Repository: dashpay/dash-evo-tool

Length of output: 1189

---

🏁 Script executed:

# Check core_cookie_path and how it uses devnet_name
rg -n "core_cookie_path" --type rust -B 5 -A 10

Repository: dashpay/dash-evo-tool

Length of output: 7207

---

🏁 Script executed:

# Check the context of the code around lines 235-238 to understand network_config
sed -n '220,260p' src/context/mod.rs

Repository: dashpay/dash-evo-tool

Length of output: 1950

---

🏁 Script executed:

# Check if network_config has devnet_name field
rg -n "network_config" src/context/mod.rs -B 2 -A 5

Repository: dashpay/dash-evo-tool

Length of output: 1931

---

Set devnet_name to network_config.devnet_name instead of hardcoding to None.

network_config.devnet_name is available at this location (loaded at line 114) and is already being used correctly elsewhere in the same function (line 151 with core_cookie_path). The hardcoded None creates an inconsistency and breaks devnet identification in the AppContext struct. Change line 238 from devnet_name: None, to devnet_name: network_config.devnet_name.clone(),.

🤖 Prompt for AI Agents

In `@src/context/mod.rs` around lines 235 - 238, AppContext is being initialized
with devnet_name set to None; instead set it from the loaded network_config by
assigning devnet_name: network_config.devnet_name.clone(). Update the AppContext
initializer (where AppContext { network, developer_mode:
AtomicBool::new(developer_mode_enabled), devnet_name: ... }) to use
network_config.devnet_name.clone() so the context matches other uses (e.g.,
core_cookie_path) and correctly carries the devnet identification.

[PastaPastaPasta]

this PR shouldn't change behavior; move only

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if the original context.rs file shows devnet_name being set to None
# Look at the deleted lines from src/context.rs in the PR
git diff origin/v1.0-dev...HEAD -- src/context.rs | grep -A 5 -B 5 "devnet_name"

Length of output: 1364

---

@PastaPastaPasta You're absolutely right—I've confirmed the original src/context.rs had devnet_name: None, at line 237, so the new code correctly preserves the existing behavior. Thank you for the clarification!

---

🧠 Learnings used

Learnt from: lklimek
Repo: dashpay/dash-evo-tool PR: 264
File: src/database/wallet.rs:469-493
Timestamp: 2025-05-12T08:38:06.423Z
Learning: In the Dash SDK, `require_network` accepts Testnet addresses for Devnet networks for legacy compatibility reasons. However, the network field on the address itself still needs to be corrected using `Address::new()` after validation to ensure the correct network is associated with the address.

---

Learnt from: lklimek
Repo: dashpay/dash-evo-tool PR: 264
File: src/database/wallet.rs:469-493
Timestamp: 2025-05-12T08:38:06.423Z
Learning: In the Dash SDK, `require_network` accepts Testnet addresses for Devnet networks for legacy compatibility reasons. However, the network field on the address itself still needs to be corrected using `Address::new()` after validation to ensure the correct network is associated with the address.

---

Learnt from: lklimek
Repo: dashpay/dash-evo-tool PR: 264
File: src/database/wallet.rs:469-493
Timestamp: 2025-05-12T08:38:06.423Z
Learning: For devnets, `require_network` in the Dash SDK accepts testnet addresses for legacy compatibility. However, it may still be necessary to update the network information on the address object itself with `Address::new()` to ensure the correct network is associated with the address.