feat: database version handling

[pauldelucia]

Summary by CodeRabbit

• New Features

  • Enhanced database initialization process with improved handling for first-time setups and versioning.
  • Added functionality to update app settings when switching networks.

• Bug Fixes

  • Improved error handling and consistency for database operations.

• Documentation

  • Updated constants for minimum and default database versions to reflect the latest changes.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request involve significant modifications to the AppState struct and the Database implementation. The AppState struct's new method now initializes the Database with a reference to the database file path. Additionally, the update method has been updated to enhance the handling of network switching actions. In the Database implementation, the initialize method has been altered to manage database file paths, including checks for first-time setups and versioning. New helper methods and constants have been introduced to improve robustness and performance.

Changes

File	Change Summary
src/app.rs	- Updated new method in AppState to pass a reference for db_file_path. - Modified update method in AppState to include a call to update_settings for AppAction::SwitchNetwork.
src/database/initialization.rs	- Changed initialize method to accept db_file_path parameter. - Added methods: is_first_time_setup, is_outdated, backup_and_recreate_db, set_default_version. - Updated constants: MIN_SUPPORTED_DB_VERSION (to 1), DEFAULT_DB_VERSION (to 1), and added DEFAULT_NETWORK ("dash"). - Reformatted SQL commands and added new indexes for performance.

Sequence Diagram(s)

sequenceDiagram
    participant AppState
    participant Database
    participant Context

    AppState->>Database: new(db_file_path)
    Database->>Database: initialize(db_file_path)
    alt First time setup
        Database-->>Database: Create tables, set default version
    else Outdated database
        Database-->>Database: Backup and recreate database
    end
    AppState->>Context: update(AppAction::SwitchNetwork)
    Context->>AppState: update_settings(RootScreenType::RootScreenNetworkChooser)

Loading

🐰 "In the code, we hop and play,
With databases bright and gay.
New paths we set, and actions flow,
To make our app a joy to show!
So let’s rejoice, with a cheer and a dance,
For every change, we take a chance!" 🐇✨

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[pauldelucia]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (5)

src/database/initialization.rs (5)

31-50: Optimize is_first_time_setup method logic

The method checks for the existence of the settings table and whether it has entries. However, you can simplify the logic by attempting to select from the settings table and handling exceptions if the table doesn't exist.

Apply this refactor to simplify the method:

-fn is_first_time_setup(&self) -> rusqlite::Result<bool> {
-    let conn = self.conn.lock().unwrap();
-
-    let table_exists: bool = conn.query_row(
-        "SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type='table' AND name='settings')",
-        [],
-        |row| row.get(0),
-    )?;
-
-    if !table_exists {
-        Ok(true)
-    } else {
-        let count: i64 =
-            conn.query_row("SELECT COUNT(*) FROM settings", [], |row| row.get(0))?;
-        Ok(count == 0)
-    }
-}
+fn is_first_time_setup(&self) -> rusqlite::Result<bool> {
+    let conn = self.conn.lock().unwrap();
+    let result = conn.query_row(
+        "SELECT COUNT(*) FROM settings",
+        [],
+        |row| row.get::<_, i64>(0),
+    );
+    match result {
+        Ok(count) => Ok(count == 0),
+        Err(rusqlite::Error::SqliteFailure(_, _)) => Ok(true), // Table doesn't exist
+        Err(e) => Err(e),
+    }
+}

---

53-63: Handle default version more gracefully in is_outdated

In is_outdated, if there's no version set, it defaults to 0. This could lead to unnecessary database backups if the version is not found. Consider explicitly checking for the absence of a version and handling it appropriately.

---

118-130: Consolidate table creation statements

You're calling self.execute multiple times for creating individual tables. Consider consolidating the creation of all tables within a single transaction to improve performance and ensure atomicity.

Apply this change to wrap table creation in a transaction:

+let conn = self.conn.lock().unwrap();
+let tx = conn.transaction()?;
 // Create the settings table
 self.execute(
     "CREATE TABLE IF NOT EXISTS settings (
         id INTEGER PRIMARY KEY CHECK (id = 1),
         network TEXT NOT NULL,
         start_root_screen INTEGER NOT NULL,
         database_version INTEGER NOT NULL
     )",
     [],
 )?;
 
 // ... other table creation statements ...

+tx.commit()?;

---

7-9: Use consistent naming for constants

The constant MIN_SUPPORTED_DB_VERSION is set to 1, which is the same as DEFAULT_DB_VERSION. If both are intended to always be the same, consider adding a comment to clarify or reuse one constant to avoid confusion.

---

85-87: Handle potential errors when renaming the database file

When renaming data.db to the backup file, any failure will currently return a generic ToSqlConversionFailure error. Consider providing a more descriptive error or handling specific cases like permission issues.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9b5685e and 019e6ef.

📒 Files selected for processing (2)

• src/app.rs (2 hunks)
• src/database/initialization.rs (3 hunks)

🔇 Additional comments (3)

src/app.rs (1)

117-118: ⚠️ Potential issue

Improve database initialization error handling and clarity.

The current implementation has several concerns:

1. Using unwrap() for database operations could lead to runtime panics
2. Database is initialized twice with the same path

Consider applying this improvement:

-        let db = Arc::new(Database::new(&db_file_path).unwrap());
-        db.initialize(&db_file_path).unwrap();
+        let db = Arc::new(Database::new(&db_file_path)
+            .map_err(|e| format!("Failed to create database: {}", e))?);
+        db.initialize(&db_file_path)
+            .map_err(|e| format!("Failed to initialize database: {}", e))?;

Let's verify if these are indeed separate operations:

src/database/initialization.rs (2)

104-110: Encrypt sensitive data when inserting default settings

When inserting default settings, ensure that any sensitive information is properly encrypted or secured, especially if network or other fields could contain confidential data.

[security]

---

Line range hint 66-116: Ensure thread safety when recreating database connection in backup_and_recreate_db

When you replace the self.conn with a new connection after recreating the database, ensure that this operation is thread-safe. Locking the mutex and replacing the connection is correct, but consider the implications if other threads are accessing the database simultaneously.

To verify thread safety, check for other usages of self.conn that might be affected during the replacement:

✅ Verification successful

Based on the search results, I can now generate the final response:

Thread safety is properly maintained in database operations

All usages of self.conn across the codebase consistently follow the same pattern of acquiring the mutex lock before performing database operations. The backup_and_recreate_db method follows this pattern correctly:

• It creates a new connection outside the lock
• Only acquires the lock when replacing the connection
• Releases the lock immediately after the replacement

This implementation ensures thread safety as:

• Other threads will block when trying to acquire the lock during connection replacement
• The lock is held for the minimal duration needed
• All other database operations in the codebase properly acquire the lock before accessing the connection

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for all usages of self.conn in the codebase to identify potential threading issues.

# Expected: Review all points where self.conn is used to ensure they handle the mutex correctly.

rg -A 2 "self\.conn" src/

Length of output: 7833

[coderabbitai]

💡 Codebase verification

🛠️ Refactor suggestion

Error handling improvement is valid but needs adjustment

The review suggestion is partially correct. While error handling can be improved, the implementation should be adjusted based on the codebase context:

            AppAction::SwitchNetwork(network) => {
                self.change_network(network);
-                self.current_app_context()
-                    .update_settings(RootScreenType::RootScreenNetworkChooser)
-                    .ok();
+                if let Err(e) = self.current_app_context()
+                    .update_settings(RootScreenType::RootScreenNetworkChooser) {
+                    self.visible_screen_mut().display_message(
+                        "Failed to save network settings",
+                        MessageType::Error
+                    );
+                }
            }

• The error message should be simplified as the detailed error might expose internal implementation details
• The display_message method exists through the ScreenLike trait
• The suggested error handling pattern aligns with the existing MessageType enum that includes Error variant

🔗 Analysis chain

Enhance network switching error handling and user feedback.

The current implementation silently ignores potential errors during settings update with .ok(). This could lead to a confusing user experience if the settings update fails.

Consider this improvement:

             AppAction::SwitchNetwork(network) => {
                 self.change_network(network);
-                self.current_app_context()
-                    .update_settings(RootScreenType::RootScreenNetworkChooser)
-                    .ok();
+                if let Err(e) = self.current_app_context()
+                    .update_settings(RootScreenType::RootScreenNetworkChooser) {
+                    self.visible_screen_mut()
+                        .display_message(&format!("Failed to update network settings: {}", e), MessageType::Error);
+                }
             }

Let's verify the network switching implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if change_network handles all network types
ast-grep --pattern 'fn change_network($params) {
  $$$
}'

# Verify if update_settings is properly implemented
ast-grep --pattern 'fn update_settings($params) {
  $$$
}'

Length of output: 272

---

Script:

#!/bin/bash
# Search for change_network method implementation
rg "fn change_network" -A 10

# Search for update_settings method implementation
rg "fn update_settings" -A 10

# Search for any error handling patterns in network switching
rg "SwitchNetwork" -A 10

Length of output: 3666

---

Script:

#!/bin/bash
# Check if there's any error handling or user feedback mechanism in the visible_screen_mut method
rg "fn visible_screen_mut" -A 10

# Check if there's any error type definition for update_settings
rg "insert_or_update_settings" -A 10

# Look for MessageType enum definition
rg "enum MessageType" -A 10

Length of output: 2193

[coderabbitai]

🛠️ Refactor suggestion

Ensure proper error handling in initialize method

The initialize method handles different scenarios for database setup and versioning. However, consider handling potential errors more robustly, especially when calling create_tables() and set_default_version() methods. If these methods fail, it might leave the database in an inconsistent state. You could implement a transaction to ensure atomicity of database initialization operations.

[coderabbitai]

🛠️ Refactor suggestion

Define foreign key constraints explicitly

In the identity table, you're referencing the wallet table via wallet(seed_hash). Ensure that the foreign key constraints are correctly enforced and consider adding ON UPDATE CASCADE or ON DELETE CASCADE as appropriate.

[coderabbitai]

⚠️ Potential issue

Prevent potential conflicts in set_default_version

The INSERT ... ON CONFLICT statement updates the database_version, but it also attempts to insert default values for other columns, which might not be desired if those columns have been modified. Consider updating only the database_version to avoid overwriting other settings.

Apply this fix to update only the database_version:

 self.execute(
     "INSERT INTO settings (id, database_version)
      VALUES (1, ?)
      ON CONFLICT(id) DO UPDATE SET database_version = excluded.database_version",
     params![DEFAULT_DB_VERSION],
 )?;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	fn set_default_version(&self) -> rusqlite::Result<()> {
	self.execute(
	"INSERT INTO settings (id, network, start_root_screen, database_version)
	VALUES (1, ?, 0, ?)
	ON CONFLICT(id) DO UPDATE SET database_version = excluded.database_version",
	params![DEFAULT_NETWORK, DEFAULT_DB_VERSION],
	)?;
	Ok(())
	}
	fn set_default_version(&self) -> rusqlite::Result<()> {
	self.execute(
	"INSERT INTO settings (id, database_version)
	VALUES (1, ?)
	ON CONFLICT(id) DO UPDATE SET database_version = excluded.database_version",
	params![DEFAULT_DB_VERSION],
	)?;
	Ok(())
	}