sqlite support and key management implementation #89
Conversation
Table creation on ~/.mcli/mcli.db
|
Warning Rate limit exceeded@grunch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 58 seconds before requesting another review. ⌛ 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. WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
src/db.rs (1)
169-169: Ensure consistency in comment languageThe comment
// Setters encadenablesis in Spanish, whereas other comments are in English. For consistency and clarity, consider translating it to English.Apply this diff:
- // Setters encadenables + // Chainable setterssrc/cli.rs (1)
301-304: Remove commented-out code to enhance code clarityCommented-out code can clutter the codebase and make it harder to read. If this code is no longer needed, consider removing it.
Apply this diff:
- // let user = User::get(pool).await.unwrap(); - // println!("User: {:#?}", user); - // let mcli_path = PathBuf::from(mcli_path); - // println!("mcli path: {:?}", mcli_path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(2 hunks)src/cli.rs(2 hunks)src/db.rs(1 hunks)src/lib.rs(1 hunks)src/util.rs(2 hunks)
🔇 Additional comments (2)
src/lib.rs (1)
2-2: Module declaration for db added correctly
The addition of pub mod db; appropriately includes the new db module in the library.
Cargo.toml (1)
25-26: Dependency updates are appropriate
The updates to the dependencies, including nostr-sdk, serde, mostro-core, and addition of sqlx and bip39, are correctly specified and necessary for the new functionalities.
Also applies to: 42-48
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/util.rs (1)
527-542: Add documentation for the add_trailing_slash functionConsider adding documentation to explain the function's purpose and behavior.
Add this documentation:
+/// Adds a trailing slash to the given PathBuf if it doesn't already have one. +/// +/// # Arguments +/// +/// * `p` - A mutable reference to a PathBuf that will be modified in place +/// +/// # Examples +/// +/// ``` +/// let mut path = PathBuf::from("/tmp/test"); +/// add_trailing_slash(&mut path); +/// assert_eq!(path.to_str().unwrap(), "/tmp/test/"); +/// ``` fn add_trailing_slash(p: &mut PathBuf) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(2 hunks)src/db.rs(1 hunks)src/util.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (1)
src/util.rs (1)
522-525: LGTM!
The implementation correctly handles path separators in a cross-platform manner.
| #[derive(Debug, Default, Clone)] | ||
| pub struct Order { | ||
| pub id: Option<String>, | ||
| pub kind: Option<String>, | ||
| pub status: Option<String>, | ||
| pub amount: i64, | ||
| pub fiat_code: String, | ||
| pub min_amount: Option<i64>, | ||
| pub max_amount: Option<i64>, | ||
| pub fiat_amount: i64, | ||
| pub payment_method: String, | ||
| pub premium: i64, | ||
| pub buyer_trade_pubkey: Option<String>, | ||
| pub seller_trade_pubkey: Option<String>, | ||
| pub buyer_invoice: Option<String>, | ||
| pub created_at: Option<i64>, | ||
| pub expires_at: Option<i64>, | ||
| pub buyer_token: Option<u16>, | ||
| pub seller_token: Option<u16>, | ||
| } | ||
|
|
||
| impl Order { | ||
| // Setters encadenables | ||
| pub fn set_kind(&mut self, kind: String) -> &mut Self { | ||
| self.kind = Some(kind); | ||
| self | ||
| } | ||
|
|
||
| pub fn set_status(&mut self, status: String) -> &mut Self { | ||
| self.status = Some(status); | ||
| self | ||
| } | ||
|
|
||
| pub fn set_amount(&mut self, amount: i64) -> &mut Self { | ||
| self.amount = amount; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_fiat_code(&mut self, fiat_code: String) -> &mut Self { | ||
| self.fiat_code = fiat_code; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_min_amount(&mut self, min_amount: i64) -> &mut Self { | ||
| self.min_amount = Some(min_amount); | ||
| self | ||
| } | ||
|
|
||
| pub fn set_max_amount(&mut self, max_amount: i64) -> &mut Self { | ||
| self.max_amount = Some(max_amount); | ||
| self | ||
| } | ||
|
|
||
| pub fn set_fiat_amount(&mut self, fiat_amount: i64) -> &mut Self { | ||
| self.fiat_amount = fiat_amount; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_payment_method(&mut self, payment_method: String) -> &mut Self { | ||
| self.payment_method = payment_method; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_premium(&mut self, premium: i64) -> &mut Self { | ||
| self.premium = premium; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_buyer_trade_key(&mut self, buyer_trade_pubkey: String) -> &mut Self { | ||
| self.buyer_trade_pubkey = Some(buyer_trade_pubkey); | ||
| self | ||
| } | ||
|
|
||
| pub fn set_seller_trade_key(&mut self, seller_trade_pubkey: String) -> &mut Self { | ||
| self.seller_trade_pubkey = Some(seller_trade_pubkey); | ||
| self | ||
| } | ||
|
|
||
| // Applying changes to the database | ||
| pub async fn save(&self, pool: &SqlitePool) -> Result<(), Box<dyn std::error::Error>> { | ||
| // Validation if an identity document is present | ||
| if let Some(ref id) = self.id { | ||
| sqlx::query( | ||
| r#" | ||
| UPDATE orders | ||
| SET kind = ?, status = ?, amount = ?, fiat_code = ?, min_amount = ?, max_amount = ?, | ||
| fiat_amount = ?, payment_method = ?, premium = ?, buyer_trade_pubkey = ?, | ||
| seller_trade_pubkey = ?, buyer_invoice = ?, created_at = ?, expires_at = ?, | ||
| buyer_token = ?, seller_token = ? | ||
| WHERE id = ? | ||
| "#, | ||
| ) | ||
| .bind(&self.kind) | ||
| .bind(&self.status) | ||
| .bind(self.amount) | ||
| .bind(&self.fiat_code) | ||
| .bind(self.min_amount) | ||
| .bind(self.max_amount) | ||
| .bind(self.fiat_amount) | ||
| .bind(&self.payment_method) | ||
| .bind(self.premium) | ||
| .bind(&self.buyer_trade_pubkey) | ||
| .bind(&self.seller_trade_pubkey) | ||
| .bind(&self.buyer_invoice) | ||
| .bind(self.created_at) | ||
| .bind(self.expires_at) | ||
| .bind(self.buyer_token) | ||
| .bind(self.seller_token) | ||
| .bind(id) | ||
| .execute(pool) | ||
| .await?; | ||
|
|
||
| println!("Order with id {} updated in the database.", id); | ||
| } else { | ||
| return Err("Order must have an ID to be updated.".into()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing CRUD operations for Order struct
The Order implementation needs additional functionality:
- Missing method to create new orders
- Missing method to fetch orders
- Inconsistent error handling between setters and save
Here's a suggested implementation for the missing functionality:
+impl Order {
+ pub async fn new(
+ kind: String,
+ amount: i64,
+ fiat_code: String,
+ fiat_amount: i64,
+ payment_method: String,
+ premium: i64,
+ pool: &SqlitePool,
+ ) -> Result<Self, Box<dyn std::error::Error>> {
+ let order = Order {
+ id: Some(Uuid::new_v4().to_string()),
+ kind: Some(kind),
+ status: Some("PENDING".to_string()),
+ amount,
+ fiat_code,
+ min_amount: None,
+ max_amount: None,
+ fiat_amount,
+ payment_method,
+ premium,
+ buyer_trade_pubkey: None,
+ seller_trade_pubkey: None,
+ buyer_invoice: None,
+ created_at: Some(chrono::Utc::now().timestamp()),
+ expires_at: None,
+ buyer_token: None,
+ seller_token: None,
+ };
+
+ sqlx::query(
+ r#"
+ INSERT INTO orders (
+ id, kind, status, amount, fiat_code, fiat_amount,
+ payment_method, premium, created_at
+ ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
+ "#,
+ )
+ .bind(&order.id)
+ .bind(&order.kind)
+ .bind(&order.status)
+ .bind(order.amount)
+ .bind(&order.fiat_code)
+ .bind(order.fiat_amount)
+ .bind(&order.payment_method)
+ .bind(order.premium)
+ .bind(order.created_at)
+ .execute(pool)
+ .await?;
+
+ Ok(order)
+ }
+
+ pub async fn get_by_id(id: &str, pool: &SqlitePool) -> Result<Self, Box<dyn std::error::Error>> {
+ let order = sqlx::query_as::<_, Order>(
+ r#"
+ SELECT *
+ FROM orders
+ WHERE id = ?
+ "#,
+ )
+ .bind(id)
+ .fetch_one(pool)
+ .await?;
+
+ Ok(order)
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| pub async fn connect() -> Result<Pool<Sqlite>, sqlx::Error> { | ||
| let mcli_dir = get_mcli_path(); | ||
| let mcli_db_path = format!("{}/mcli.db", mcli_dir); | ||
| println!("mcli_db_path: {}", mcli_db_path); | ||
| let db_url = format!("sqlite://{}", mcli_db_path); | ||
| let pool: Pool<Sqlite>; | ||
| if !Path::exists(Path::new(&mcli_db_path)) { | ||
| if let Err(res) = File::create(&mcli_db_path) { | ||
| println!("Error in creating db file: {}", res); | ||
| return Err(sqlx::Error::Io(res)); | ||
| } | ||
| pool = SqlitePool::connect(&db_url).await?; | ||
| println!("Creating database file with orders table..."); | ||
| sqlx::query( | ||
| r#" | ||
| CREATE TABLE IF NOT EXISTS orders ( | ||
| id TEXT PRIMARY KEY, | ||
| kind TEXT, | ||
| status TEXT, | ||
| amount INTEGER NOT NULL, | ||
| fiat_code TEXT NOT NULL, | ||
| min_amount INTEGER, | ||
| max_amount INTEGER, | ||
| fiat_amount INTEGER NOT NULL, | ||
| payment_method TEXT NOT NULL, | ||
| premium INTEGER NOT NULL, | ||
| master_buyer_pubkey TEXT, | ||
| master_seller_pubkey TEXT, | ||
| buyer_invoice TEXT, | ||
| created_at INTEGER, | ||
| expires_at INTEGER, | ||
| buyer_token INTEGER, | ||
| seller_token INTEGER | ||
| ); | ||
| CREATE TABLE IF NOT EXISTS users ( | ||
| i0_pubkey TEXT PRIMARY KEY, | ||
| mnemonic TEXT, | ||
| last_trade_index INTEGER, | ||
| created_at INTEGER | ||
| ); | ||
| "#, | ||
| ) | ||
| .execute(&pool) | ||
| .await?; | ||
|
|
||
| let mnemonic = match Mnemonic::generate(12) { | ||
| Ok(m) => m.to_string(), | ||
| Err(e) => { | ||
| println!("Error generating mnemonic: {}", e); | ||
| return Err(sqlx::Error::Decode(Box::new(e))); | ||
| } | ||
| }; | ||
| let user = User::new(mnemonic, &pool).await.unwrap(); | ||
| println!("User created with pubkey: {}", user.i0_pubkey); | ||
| } else { | ||
| pool = SqlitePool::connect(&db_url).await?; | ||
| } | ||
|
|
||
| Ok(pool) | ||
| } |
There was a problem hiding this comment.
Improve error handling and database initialization
Several improvements are needed in the database connection logic:
- The database path construction should handle errors gracefully
- Table creation queries should be parameterized
- User creation should handle errors instead of using unwrap()
- Consider adding database migrations for schema changes
Apply this diff to improve error handling:
pub async fn connect() -> Result<Pool<Sqlite>, sqlx::Error> {
let mcli_dir = get_mcli_path();
let mcli_db_path = format!("{}/mcli.db", mcli_dir);
- println!("mcli_db_path: {}", mcli_db_path);
+ log::info!("Using database at: {}", mcli_db_path);
let db_url = format!("sqlite://{}", mcli_db_path);
let pool: Pool<Sqlite>;
if !Path::exists(Path::new(&mcli_db_path)) {
if let Err(res) = File::create(&mcli_db_path) {
- println!("Error in creating db file: {}", res);
- return Err(sqlx::Error::Io(res));
+ log::error!("Failed to create database file: {}", res);
+ return Err(sqlx::Error::Io(res));
}
pool = SqlitePool::connect(&db_url).await?;
- println!("Creating database file with orders table...");
+ log::info!("Initializing database schema...");
sqlx::query(
r#"
CREATE TABLE IF NOT EXISTS orders (
id TEXT PRIMARY KEY,
kind TEXT,
status TEXT,
amount INTEGER NOT NULL,
fiat_code TEXT NOT NULL,
min_amount INTEGER,
max_amount INTEGER,
fiat_amount INTEGER NOT NULL,
payment_method TEXT NOT NULL,
premium INTEGER NOT NULL,
master_buyer_pubkey TEXT,
master_seller_pubkey TEXT,
buyer_invoice TEXT,
created_at INTEGER,
expires_at INTEGER,
buyer_token INTEGER,
seller_token INTEGER
);
CREATE TABLE IF NOT EXISTS users (
i0_pubkey TEXT PRIMARY KEY,
mnemonic TEXT,
last_trade_index INTEGER,
created_at INTEGER
);
"#,
)
.execute(&pool)
.await?;
let mnemonic = match Mnemonic::generate(12) {
Ok(m) => m.to_string(),
Err(e) => {
println!("Error generating mnemonic: {}", e);
return Err(sqlx::Error::Decode(Box::new(e)));
}
};
- let user = User::new(mnemonic, &pool).await.unwrap();
- println!("User created with pubkey: {}", user.i0_pubkey);
+ let user = User::new(mnemonic, &pool).await
+ .map_err(|e| {
+ log::error!("Failed to create user: {}", e);
+ sqlx::Error::Protocol(format!("Failed to create user: {}", e))
+ })?;
+ log::info!("Created new user with pubkey: {}", user.i0_pubkey);
} else {
pool = SqlitePool::connect(&db_url).await?;
}
Ok(pool)
}📝 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.
| pub async fn connect() -> Result<Pool<Sqlite>, sqlx::Error> { | |
| let mcli_dir = get_mcli_path(); | |
| let mcli_db_path = format!("{}/mcli.db", mcli_dir); | |
| println!("mcli_db_path: {}", mcli_db_path); | |
| let db_url = format!("sqlite://{}", mcli_db_path); | |
| let pool: Pool<Sqlite>; | |
| if !Path::exists(Path::new(&mcli_db_path)) { | |
| if let Err(res) = File::create(&mcli_db_path) { | |
| println!("Error in creating db file: {}", res); | |
| return Err(sqlx::Error::Io(res)); | |
| } | |
| pool = SqlitePool::connect(&db_url).await?; | |
| println!("Creating database file with orders table..."); | |
| sqlx::query( | |
| r#" | |
| CREATE TABLE IF NOT EXISTS orders ( | |
| id TEXT PRIMARY KEY, | |
| kind TEXT, | |
| status TEXT, | |
| amount INTEGER NOT NULL, | |
| fiat_code TEXT NOT NULL, | |
| min_amount INTEGER, | |
| max_amount INTEGER, | |
| fiat_amount INTEGER NOT NULL, | |
| payment_method TEXT NOT NULL, | |
| premium INTEGER NOT NULL, | |
| master_buyer_pubkey TEXT, | |
| master_seller_pubkey TEXT, | |
| buyer_invoice TEXT, | |
| created_at INTEGER, | |
| expires_at INTEGER, | |
| buyer_token INTEGER, | |
| seller_token INTEGER | |
| ); | |
| CREATE TABLE IF NOT EXISTS users ( | |
| i0_pubkey TEXT PRIMARY KEY, | |
| mnemonic TEXT, | |
| last_trade_index INTEGER, | |
| created_at INTEGER | |
| ); | |
| "#, | |
| ) | |
| .execute(&pool) | |
| .await?; | |
| let mnemonic = match Mnemonic::generate(12) { | |
| Ok(m) => m.to_string(), | |
| Err(e) => { | |
| println!("Error generating mnemonic: {}", e); | |
| return Err(sqlx::Error::Decode(Box::new(e))); | |
| } | |
| }; | |
| let user = User::new(mnemonic, &pool).await.unwrap(); | |
| println!("User created with pubkey: {}", user.i0_pubkey); | |
| } else { | |
| pool = SqlitePool::connect(&db_url).await?; | |
| } | |
| Ok(pool) | |
| } | |
| pub async fn connect() -> Result<Pool<Sqlite>, sqlx::Error> { | |
| let mcli_dir = get_mcli_path(); | |
| let mcli_db_path = format!("{}/mcli.db", mcli_dir); | |
| log::info!("Using database at: {}", mcli_db_path); | |
| let db_url = format!("sqlite://{}", mcli_db_path); | |
| let pool: Pool<Sqlite>; | |
| if !Path::exists(Path::new(&mcli_db_path)) { | |
| if let Err(res) = File::create(&mcli_db_path) { | |
| log::error!("Failed to create database file: {}", res); | |
| return Err(sqlx::Error::Io(res)); | |
| } | |
| pool = SqlitePool::connect(&db_url).await?; | |
| log::info!("Initializing database schema..."); | |
| sqlx::query( | |
| r#" | |
| CREATE TABLE IF NOT EXISTS orders ( | |
| id TEXT PRIMARY KEY, | |
| kind TEXT, | |
| status TEXT, | |
| amount INTEGER NOT NULL, | |
| fiat_code TEXT NOT NULL, | |
| min_amount INTEGER, | |
| max_amount INTEGER, | |
| fiat_amount INTEGER NOT NULL, | |
| payment_method TEXT NOT NULL, | |
| premium INTEGER NOT NULL, | |
| master_buyer_pubkey TEXT, | |
| master_seller_pubkey TEXT, | |
| buyer_invoice TEXT, | |
| created_at INTEGER, | |
| expires_at INTEGER, | |
| buyer_token INTEGER, | |
| seller_token INTEGER | |
| ); | |
| CREATE TABLE IF NOT EXISTS users ( | |
| i0_pubkey TEXT PRIMARY KEY, | |
| mnemonic TEXT, | |
| last_trade_index INTEGER, | |
| created_at INTEGER | |
| ); | |
| "#, | |
| ) | |
| .execute(&pool) | |
| .await?; | |
| let mnemonic = match Mnemonic::generate(12) { | |
| Ok(m) => m.to_string(), | |
| Err(e) => { | |
| println!("Error generating mnemonic: {}", e); | |
| return Err(sqlx::Error::Decode(Box::new(e))); | |
| } | |
| }; | |
| let user = User::new(mnemonic, &pool).await | |
| .map_err(|e| { | |
| log::error!("Failed to create user: {}", e); | |
| sqlx::Error::Protocol(format!("Failed to create user: {}", e)) | |
| })?; | |
| log::info!("Created new user with pubkey: {}", user.i0_pubkey); | |
| } else { | |
| pool = SqlitePool::connect(&db_url).await?; | |
| } | |
| Ok(pool) | |
| } |
| #[derive(Debug, Default, Clone, sqlx::FromRow)] | ||
| pub struct User { | ||
| /// The user's ID is the identity pubkey | ||
| pub i0_pubkey: String, | ||
| pub mnemonic: String, | ||
| pub last_trade_index: Option<i64>, | ||
| pub created_at: i64, | ||
| } | ||
|
|
||
| impl User { | ||
| pub async fn new( | ||
| mnemonic: String, | ||
| pool: &SqlitePool, | ||
| ) -> Result<Self, Box<dyn std::error::Error>> { | ||
| let mut user = User::default(); | ||
| let account = NOSTR_REPLACEABLE_EVENT_KIND as u32; | ||
| let i0_keys = | ||
| Keys::from_mnemonic_advanced(&mnemonic, None, Some(account), Some(0), Some(0))?; | ||
| user.i0_pubkey = i0_keys.public_key().to_string(); | ||
| user.created_at = chrono::Utc::now().timestamp(); | ||
| user.mnemonic = mnemonic; | ||
| sqlx::query( | ||
| r#" | ||
| INSERT INTO users (i0_pubkey, mnemonic, created_at) | ||
| VALUES (?, ?, ?) | ||
| "#, | ||
| ) | ||
| .bind(&user.i0_pubkey) | ||
| .bind(&user.mnemonic) | ||
| .bind(user.created_at) | ||
| .execute(pool) | ||
| .await?; | ||
|
|
||
| Ok(user) | ||
| } | ||
| // Chainable setters | ||
| pub fn set_mnemonic(&mut self, mnemonic: String) -> &mut Self { | ||
| self.mnemonic = mnemonic; | ||
| self | ||
| } | ||
|
|
||
| pub fn set_last_trade_index(&mut self, last_trade_index: i64) -> &mut Self { | ||
| self.last_trade_index = Some(last_trade_index); | ||
| self | ||
| } | ||
|
|
||
| // Applying changes to the database | ||
| pub async fn save(&self, pool: &SqlitePool) -> Result<(), Box<dyn std::error::Error>> { | ||
| sqlx::query( | ||
| r#" | ||
| UPDATE users | ||
| SET mnemonic = ? | ||
| WHERE i0_pubkey = ? | ||
| "#, | ||
| ) | ||
| .bind(&self.mnemonic) | ||
| .bind(&self.i0_pubkey) | ||
| .execute(pool) | ||
| .await?; | ||
|
|
||
| println!( | ||
| "User with i0 pubkey {} updated in the database.", | ||
| self.i0_pubkey | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn get(pool: SqlitePool) -> Result<User, Box<dyn std::error::Error>> { | ||
| let user = sqlx::query_as::<_, User>( | ||
| r#" | ||
| SELECT i0_pubkey, mnemonic, last_trade_index, created_at | ||
| FROM users | ||
| "#, | ||
| ) | ||
| .fetch_one(&pool) | ||
| .await?; | ||
|
|
||
| Ok(user) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance User struct implementation
The User implementation has several areas for improvement:
- The
get()method fetches all users without a limit - The
save()method only updates the mnemonic field - Missing validation for mnemonic format
Apply this diff to improve the implementation:
pub async fn get(pool: SqlitePool) -> Result<User, Box<dyn std::error::Error>> {
- let user = sqlx::query_as::<_, User>(
+ let user = sqlx::query_as::<_, User>(
r#"
SELECT i0_pubkey, mnemonic, last_trade_index, created_at
FROM users
+ LIMIT 1
"#,
)
.fetch_one(&pool)
.await?;
Ok(user)
}
pub async fn save(&self, pool: &SqlitePool) -> Result<(), Box<dyn std::error::Error>> {
sqlx::query(
r#"
UPDATE users
- SET mnemonic = ?
+ SET mnemonic = ?,
+ last_trade_index = ?
WHERE i0_pubkey = ?
"#,
)
.bind(&self.mnemonic)
+ .bind(self.last_trade_index)
.bind(&self.i0_pubkey)
.execute(pool)
.await?;
Ok(())
}
+fn validate_mnemonic(mnemonic: &str) -> bool {
+ mnemonic.split_whitespace().count() == 12
+}Committable suggestion skipped: line range outside the PR's diff.
| pub fn get_mcli_path() -> String { | ||
| let home_dir = dirs::home_dir().expect("Couldn't get home directory"); | ||
| let mcli_path = format!("{}/.mcli", home_dir.display()); | ||
| if !Path::new(&mcli_path).exists() { | ||
| fs::create_dir(&mcli_path).expect("Couldn't create mostro-cli directory in HOME"); | ||
| println!("Directory {} created.", mcli_path); | ||
| } | ||
|
|
||
| mcli_path | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve directory creation error handling
The directory creation logic needs better error handling and permissions verification.
Apply this diff:
pub fn get_mcli_path() -> String {
let home_dir = dirs::home_dir().expect("Couldn't get home directory");
let mcli_path = format!("{}/.mcli", home_dir.display());
if !Path::new(&mcli_path).exists() {
- fs::create_dir(&mcli_path).expect("Couldn't create mostro-cli directory in HOME");
- println!("Directory {} created.", mcli_path);
+ fs::create_dir_all(&mcli_path)
+ .map_err(|e| {
+ log::error!("Failed to create directory {}: {}", mcli_path, e);
+ e
+ })
+ .expect("Failed to create mostro-cli directory");
+
+ #[cfg(unix)]
+ {
+ use std::os::unix::fs::PermissionsExt;
+ fs::set_permissions(&mcli_path, fs::Permissions::from_mode(0o700))
+ .expect("Failed to set directory permissions");
+ }
+
+ log::info!("Created directory: {}", mcli_path);
}
mcli_path
}📝 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.
| pub fn get_mcli_path() -> String { | |
| let home_dir = dirs::home_dir().expect("Couldn't get home directory"); | |
| let mcli_path = format!("{}/.mcli", home_dir.display()); | |
| if !Path::new(&mcli_path).exists() { | |
| fs::create_dir(&mcli_path).expect("Couldn't create mostro-cli directory in HOME"); | |
| println!("Directory {} created.", mcli_path); | |
| } | |
| mcli_path | |
| } | |
| pub fn get_mcli_path() -> String { | |
| let home_dir = dirs::home_dir().expect("Couldn't get home directory"); | |
| let mcli_path = format!("{}/.mcli", home_dir.display()); | |
| if !Path::new(&mcli_path).exists() { | |
| fs::create_dir_all(&mcli_path) | |
| .map_err(|e| { | |
| log::error!("Failed to create directory {}: {}", mcli_path, e); | |
| e | |
| }) | |
| .expect("Failed to create mostro-cli directory"); | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::PermissionsExt; | |
| fs::set_permissions(&mcli_path, fs::Permissions::from_mode(0o700)) | |
| .expect("Failed to set directory permissions"); | |
| } | |
| log::info!("Created directory: {}", mcli_path); | |
| } | |
| mcli_path | |
| } |
* Upgrade nostr sdk version * Fixes on db
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
src/nip59.rs (2)
37-38: Inconsistent Parameter Naming: Consider Renamingsender_keysFor consistency with the rest of the codebase, consider renaming the
sender_keysparameter toidentity_keysin thesealfunction. This will align the function signature with the updated key management terminology.
93-93: Clarify Key Selection LogicIn the
unwrap_gift_wrapfunction, the selection betweenkeysandgw_ckcould be simplified or better documented to enhance readability and maintainability.src/cli/rate_user.rs (1)
Line range hint
19-25: Consider enhancing rating validationWhile the current rating validation is functional, consider extracting it into a dedicated function for better reusability and maintainability.
+fn validate_rating(rating: u8) -> Result<Content> { + match rating { + 1..=5 => Ok(Content::RatingUser(rating)), + _ => Err(anyhow::anyhow!("Rating must be in the range 1 - 5")) + } +}src/cli/add_invoice.rs (2)
Line range hint
9-16: Add documentation for the new key parameters.The split between
identity_keysandtrade_keysneeds documentation to explain their distinct purposes and when each should be used.Add documentation above the function:
+/// Execute the add invoice command +/// @param identity_keys Keys used for identifying the user +/// @param trade_keys Keys used for signing trade-related messages pub async fn execute_add_invoice( order_id: &Uuid, invoice: &str, identity_keys: &Keys, trade_keys: &Keys, mostro_key: PublicKey, client: &Client, ) -> Result<()> {
46-54: Consider consolidating boolean flags in send_order_id_cmd.The
send_order_id_cmdcall has two boolean parameters (true, false) whose purpose isn't immediately clear.Consider using a more explicit parameter structure:
-send_order_id_cmd( - client, - identity_keys, - trade_keys, - mostro_key, - add_invoice_message, - true, - false, -).await?; +send_order_id_cmd( + SendOrderOptions { + client, + identity_keys, + trade_keys, + mostro_key, + message: add_invoice_message, + require_confirmation: true, + skip_validation: false, + } +).await?;src/cli/get_dm.rs (1)
13-13: Consider adding error context for get_direct_messages.The error from
get_direct_messagesis not being contextualized, which might make debugging harder.- let dm = get_direct_messages(client, trade_keys, *since, from_user).await; + let dm = get_direct_messages(client, trade_keys, *since, from_user) + .await + .context("Failed to retrieve direct messages")?;src/cli.rs (1)
324-334: Consider implementing key rotation for enhanced security.The trade keys are generated sequentially which might be predictable. Consider implementing key rotation mechanisms and secure key derivation.
Some suggestions:
- Implement key rotation based on time or usage
- Add entropy to key derivation
- Consider storing key usage metrics
Also applies to: 337-346, 349-357
src/util.rs (5)
26-61: Enhance error handling in send_dm functionThe function should provide more detailed error context when operations fail.
Apply this diff:
pub async fn send_dm( client: &Client, identity_keys: &Keys, trade_keys: &Keys, receiver_pubkey: &PublicKey, content: String, to_user: bool, ) -> Result<()> { - let pow: u8 = var("POW").unwrap_or('0'.to_string()).parse().unwrap(); + let pow: u8 = var("POW") + .unwrap_or('0'.to_string()) + .parse() + .map_err(|e| anyhow::anyhow!("Failed to parse POW value: {}", e))?; let event = if to_user { // Derive conversation key let ck = ConversationKey::derive(trade_keys.secret_key(), receiver_pubkey); // Encrypt content - let encrypted_content = encrypt_to_bytes(&ck, content)?; + let encrypted_content = encrypt_to_bytes(&ck, content) + .map_err(|e| anyhow::anyhow!("Failed to encrypt content: {}", e))?; // Encode with base64 let b64decoded_content = general_purpose::STANDARD.encode(encrypted_content); // Compose builder EventBuilder::new(Kind::PrivateDirectMessage, b64decoded_content) .pow(pow) .tag(Tag::public_key(*receiver_pubkey)) - .sign_with_keys(trade_keys)? + .sign_with_keys(trade_keys) + .map_err(|e| anyhow::anyhow!("Failed to sign event: {}", e))? } else { gift_wrap( identity_keys, trade_keys, *receiver_pubkey, content, None, pow, - )? + ).map_err(|e| anyhow::anyhow!("Failed to wrap gift: {}", e))? }; info!("Sending event: {event:#?}"); - client.send_event(event).await?; + client.send_event(event) + .await + .map_err(|e| anyhow::anyhow!("Failed to send event: {}", e))?; Ok(()) }
Line range hint
64-79: Improve relay connection error handlingThe function should handle relay connection failures gracefully and provide feedback.
Apply this diff:
pub async fn connect_nostr() -> Result<Client> { let my_keys = Keys::generate(); - let relays = var("RELAYS").expect("RELAYS is not set"); + let relays = var("RELAYS") + .map_err(|_| anyhow::anyhow!("RELAYS environment variable is not set"))?; let relays = relays.split(',').collect::<Vec<&str>>(); // Create new client let client = Client::new(my_keys); + let mut connected_relays = 0; // Add relays for r in relays.into_iter() { - client.add_relay(r).await?; + match client.add_relay(r).await { + Ok(_) => { + connected_relays += 1; + info!("Successfully connected to relay: {}", r); + } + Err(e) => { + error!("Failed to connect to relay {}: {}", r, e); + } + } + } + + if connected_relays == 0 { + return Err(anyhow::anyhow!("Failed to connect to any relays")); } // Connect to relays and keep connection alive client.connect().await; Ok(client) }
Line range hint
82-150: Refactor for better readability and maintainabilityThe function is complex with deep nesting. Consider splitting it into smaller, more focused functions.
Consider extracting the message handling logic into a separate function:
+async fn handle_dm_message(dm: &(String, String, u64)) -> Result<()> { + match Message::from_json(&dm.0) { + Ok(m) => { + if let Some(Content::PaymentRequest(ord, inv, _)) = &m.get_inner_message_kind().content.0 { + println!("NEW MESSAGE:"); + println!( + "Mostro sent you this hold invoice for order id: {}", + ord.as_ref().unwrap().id.unwrap() + ); + println!(); + println!("Pay this invoice to continue --> {}", inv); + println!(); + } + } + Err(_) => { + println!("NEW MESSAGE:"); + println!(); + println!("You got this message --> {}", dm.0); + println!(); + } + } + Ok(()) +}Then simplify the main function:
pub async fn send_order_id_cmd( client: &Client, identity_keys: &Keys, trade_keys: &Keys, receiver_pubkey: PublicKey, message: String, wait_for_dm_ans: bool, to_user: bool, ) -> Result<()> { - println!("to_user: {to_user}"); + info!("Sending message with to_user: {}", to_user); send_dm( client, identity_keys, trade_keys, &receiver_pubkey, message, to_user, ) .await?; let mut notifications = client.notifications(); while let Ok(notification) = notifications.recv().await { if wait_for_dm_ans { let dms = get_direct_messages(client, trade_keys, 1, to_user).await; - for dm in dms.iter() { - match Message::from_json(&dm.0) { - // ... existing match logic ... - } - } + for dm in dms { + handle_dm_message(&dm).await?; + } break; } else if let RelayPoolNotification::Message { message: RelayMessage::Ok { .. }, .. } = notification { println!( "Message correctly sent to Mostro! Check messages with getdm or listorders command" ); break; } } Ok(()) }
Line range hint
152-177: Make timeout duration configurableThe 3-second timeout is hardcoded. Consider making it configurable through environment variables or function parameters.
Apply this diff:
+const DEFAULT_RELAY_TIMEOUT_SECS: u64 = 3; + pub async fn requests_relay( client: Client, relay: (RelayUrl, Relay), filters: Filter, + timeout_secs: Option<u64>, ) -> Vec<Event> { let relrequest = get_events_of_mostro(&relay.1, vec![filters.clone()], client); // Buffer vector let mut res: Vec<Event> = Vec::new(); // Using a timeout to avoid unresponsive relays to block the loop forever. - if let Ok(rx) = timeout(Duration::from_secs(3), relrequest).await { + let timeout_duration = Duration::from_secs(timeout_secs.unwrap_or(DEFAULT_RELAY_TIMEOUT_SECS)); + if let Ok(rx) = timeout(timeout_duration, relrequest).await { match rx { Ok(m) => { if m.is_empty() { info!("No requested events found on relay {}", relay.0.to_string()); } res = m } - Err(_e) => println!("Error"), + Err(e) => error!("Error fetching events from relay {}: {}", relay.0, e), } } res }
Line range hint
298-332: Improve error handling and configuration in message decryptionThe function uses hardcoded values and could benefit from better error handling.
Apply this diff:
+const DEFAULT_FAKE_MINUTES: i64 = 2880; + if from_user { let ck = ConversationKey::derive(my_key.secret_key(), &dm.pubkey); let b64decoded_content = match general_purpose::STANDARD.decode(dm.content.as_bytes()) { Ok(b64decoded_content) => b64decoded_content, Err(e) => { + error!("Failed to decode base64 content: {}", e); continue; } }; // Decrypt - let unencrypted_content = decrypt_to_bytes(&ck, b64decoded_content).unwrap(); - content = String::from_utf8(unencrypted_content).expect("Found invalid UTF-8"); + let unencrypted_content = match decrypt_to_bytes(&ck, b64decoded_content) { + Ok(content) => content, + Err(e) => { + error!("Failed to decrypt content: {}", e); + continue; + } + }; + content = match String::from_utf8(unencrypted_content) { + Ok(content) => content, + Err(e) => { + error!("Invalid UTF-8 in decrypted content: {}", e); + continue; + } + }; created_at = dm.created_at; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.gitignore(1 hunks)Cargo.toml(2 hunks)src/cli.rs(6 hunks)src/cli/add_invoice.rs(3 hunks)src/cli/conversation_key.rs(1 hunks)src/cli/get_dm.rs(2 hunks)src/cli/new_order.rs(3 hunks)src/cli/rate_user.rs(2 hunks)src/cli/send_dm.rs(1 hunks)src/cli/send_msg.rs(2 hunks)src/cli/take_buy.rs(2 hunks)src/cli/take_dispute.rs(2 hunks)src/cli/take_sell.rs(3 hunks)src/db.rs(1 hunks)src/nip33.rs(2 hunks)src/nip59.rs(8 hunks)src/util.rs(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (14)
src/cli/new_order.rs (2)
25-27: Function Signature Updated Appropriately
The function signature has been updated to include identity_keys, trade_keys, and trade_index, which aligns with the updated key management strategy.
113-113: Correct Use of trade_keys for Signing Order Content
Using trade_keys to sign the order_content ensures that trade-specific keys are utilized in the signing process, which enhances security and key separation.
src/cli/conversation_key.rs (1)
5-7: Usage of trade_keys for Deriving Conversation Key
The function now uses trade_keys to derive the conversation key with the receiver, which is appropriate for maintaining key separation between identity and trading activities.
src/cli/take_dispute.rs (2)
Line range hint 33-39: Verify key usage security
The separation of keys into identity_keys and trade_keys is a significant security-related change. Ensure that:
- Keys are properly stored in the SQLite database
- Access to these keys is appropriately controlled
✅ Verification successful
Let me gather more information about the key storage in SQLite.
Let me check one more aspect about the key derivation and storage.
Keys are properly managed with secure derivation and storage
The verification shows that the key management system is well-implemented with proper security measures:
- Keys are derived from a securely generated mnemonic using BIP-39 standards
- Identity and trade keys are separately derived using different derivation paths
- The mnemonic is stored in an SQLite database with proper table structure
- Trade keys use an incrementing index for key rotation
- Keys are derived on-demand and not stored directly in the database
The separation of identity_keys and trade_keys is properly implemented and follows security best practices for key management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for key usage patterns across the codebase
rg -A 3 "identity_keys|trade_keys"
Length of output: 10470
Script:
#!/bin/bash
# Search for key storage patterns in the database implementation
ast-grep --pattern 'pub async fn $_(pool: $_) -> Result<Keys, $_> {
$$$
}'
# Also check the database schema
fd -e sql | xargs cat
Length of output: 565
Script:
#!/bin/bash
# Check the User struct implementation and mnemonic handling
ast-grep --pattern 'struct User {
$$$
}'
# Look for mnemonic-related database operations
rg -A 5 "mnemonic"
Length of output: 3409
21-30: Verify message construction parameters
The Message::new_dispute construction includes multiple None parameters which could be better documented for clarity. Consider adding comments explaining the purpose of each parameter or using named parameters if the API supports it.
src/cli/rate_user.rs (1)
29-36: Verify message construction consistency
The Message::new_order construction includes multiple None parameters. Ensure this aligns with the message protocol specifications.
✅ Verification successful
Let me analyze the message construction patterns to verify the consistency. I'll check the Message struct definition to understand the expected parameters.
Let me try a different approach to find the Message implementation.
Let me try one last search with ripgrep to find the Message implementation.
Message construction pattern is consistent across the codebase
Based on the search results of Message::new_order usage patterns across different files, the construction in rate_user.rs follows the same pattern as other implementations:
- All implementations use a combination of
SomeandNonevalues - The parameter order and structure is consistent across all files
- Similar actions like
AddInvoiceandRateUserfollow identical patterns withSome(order_id), twoNones, and optional content/signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Message::new_order usage patterns
ast-grep --pattern 'Message::new_order($$$)'
Length of output: 2006
Script:
#!/bin/bash
# Search for Message struct definition
ast-grep --pattern 'struct Message {
$$$
}'
Length of output: 47
Script:
#!/bin/bash
# Search for new_order implementation
ast-grep --pattern 'impl Message {
$$$
fn new_order($$$) {
$$$
}
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Search for Message::new_order definition
rg "impl.*Message" -A 10
Length of output: 26
src/cli/take_buy.rs (1)
27-31: Verify signature implementation security
The new signature implementation needs careful review:
- Ensure the signature covers all necessary content
- Verify the signature validation process
src/cli/add_invoice.rs (1)
33-44: Verify signature creation and message construction.
The signature is created and included in the message, but we should verify:
- That content is never None when signing
- The message construction follows the expected format
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The signature creation and message construction patterns are consistent and safe.
- All
sign_contentcalls consistently use.unwrap()on content only after ensuring it'sSome - The
Message::new_orderconstruction follows a consistent pattern across all usages, with the correct parameter ordering:order_id,None,trade_index,action,content,signature - The signature is properly created from the content before being included in the message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other sign_content usages to verify consistent pattern
rg "sign_content\(" --type rust -B 2 -A 2
# Search for Message::new_order usage patterns
ast-grep --pattern 'Message::new_order($$$)'
Length of output: 3131
src/cli/get_dm.rs (1)
29-31: Review content access pattern safety.
The new .0 index access on content suggests a tuple structure. This needs:
- Documentation explaining the tuple structure
- Null safety validation
Also applies to: 35-37
✅ Verification successful
The previous search might have failed due to specific syntax. Let's try broader patterns to understand the Content type definition and its usage patterns in the codebase.
Content access pattern is safe and consistent across codebase
The .0 tuple access pattern for content is used consistently throughout the codebase, as evidenced by multiple similar usages:
- In
src/util.rs:&m.get_inner_message_kind().content.0 - In
src/cli/get_dm.rs: Multiple occurrences with same pattern - The
Contentenum is imported frommostro_core::messageand used consistently across multiple files
The pattern appears to be part of the core message handling structure, with proper null safety through Option and pattern matching:
if let Some(Content::PaymentRequest(_, inv, _)) = &m.get_inner_message_kind().content.0🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar content access patterns
ast-grep --pattern 'content.0'
# Search for Content enum definition
rg "enum Content" --type rust -B 2 -A 10
Length of output: 73
Script:
#!/bin/bash
# Search for Content type definition and usage with broader patterns
rg "Content::" --type rust -B 2 -A 2
# Search for struct or enum definitions containing "Content"
rg "^(struct|enum)\s+Content" --type rust -B 2 -A 10
# Look for files importing Content
rg "use.*Content" --type rust
# Search for message content access patterns
ast-grep --pattern 'get_inner_message_kind()'
Length of output: 4700
src/nip33.rs (1)
8-11: LGTM! Parameter type update looks good.
The change from Vec<Tag> to Tags and the corresponding update to use to_vec() instead of as_vec() is appropriate and maintains the existing functionality.
src/db.rs (3)
11-69:
Improve database initialization and error handling.
Several improvements needed in the database connection logic:
- The database path construction should handle errors gracefully
- File creation error handling could be improved
- User creation uses unwrap() which could panic
Apply this diff to improve error handling:
pub async fn connect() -> Result<Pool<Sqlite>, sqlx::Error> {
let mcli_dir = get_mcli_path();
let mcli_db_path = format!("{}/mcli.db", mcli_dir);
+ log::info!("Using database at: {}", mcli_db_path);
let db_url = format!("sqlite://{}", mcli_db_path);
let pool: Pool<Sqlite>;
if !Path::exists(Path::new(&mcli_db_path)) {
if let Err(res) = File::create(&mcli_db_path) {
- println!("Error in creating db file: {}", res);
+ log::error!("Failed to create database file: {}", res);
return Err(sqlx::Error::Io(res));
}
pool = SqlitePool::connect(&db_url).await?;
- println!("Creating database file with orders table...");
+ log::info!("Initializing database schema...");
sqlx::query(
// ... SQL query remains unchanged ...
)
.execute(&pool)
.await?;
let mnemonic = match Mnemonic::generate(12) {
Ok(m) => m.to_string(),
Err(e) => {
- println!("Error generating mnemonic: {}", e);
+ log::error!("Failed to generate mnemonic: {}", e);
return Err(sqlx::Error::Decode(Box::new(e)));
}
};
- let user = User::new(mnemonic, &pool).await.unwrap();
- println!("User created with pubkey: {}", user.i0_pubkey);
+ let user = User::new(mnemonic, &pool).await
+ .map_err(|e| {
+ log::error!("Failed to create user: {}", e);
+ sqlx::Error::Protocol(format!("Failed to create user: {}", e))
+ })?;
+ log::info!("Created new user with pubkey: {}", user.i0_pubkey);
} else {
pool = SqlitePool::connect(&db_url).await?;
}
Ok(pool)
}Likely invalid or redundant comment.
71-186: 🛠️ Refactor suggestion
Enhance User implementation with better error handling and validation.
The User implementation has several areas for improvement:
- The
save()method only updates the mnemonic field - Missing validation for mnemonic format
- Potential for race conditions in
get_next_trade_index
Consider these improvements:
pub async fn save(&self, pool: &SqlitePool) -> Result<(), Box<dyn std::error::Error>> {
sqlx::query(
r#"
UPDATE users
- SET mnemonic = ?
+ SET mnemonic = ?,
+ last_trade_index = ?
WHERE i0_pubkey = ?
"#,
)
.bind(&self.mnemonic)
+ .bind(self.last_trade_index)
.bind(&self.i0_pubkey)
.execute(pool)
.await?;
- println!(
- "User with i0 pubkey {} updated in the database.",
- self.i0_pubkey
- );
+ log::info!("Updated user with i0 pubkey: {}", self.i0_pubkey);
Ok(())
}
+fn validate_mnemonic(mnemonic: &str) -> bool {
+ mnemonic.split_whitespace().count() == 12
+}Likely invalid or redundant comment.
188-307: 🛠️ Refactor suggestion
Improve Order struct implementation and add missing CRUD operations.
The Order implementation needs several improvements:
- Missing method to create new orders
- Missing method to fetch orders
- No validation for required fields before save
Add these essential methods:
+impl Order {
+ pub async fn new(
+ kind: String,
+ amount: i64,
+ fiat_code: String,
+ fiat_amount: i64,
+ payment_method: String,
+ premium: i64,
+ pool: &SqlitePool,
+ ) -> Result<Self, Box<dyn std::error::Error>> {
+ let order = Order {
+ id: Some(Uuid::new_v4().to_string()),
+ kind: Some(kind),
+ status: Some("PENDING".to_string()),
+ amount,
+ fiat_code,
+ min_amount: None,
+ max_amount: None,
+ fiat_amount,
+ payment_method,
+ premium,
+ buyer_trade_pubkey: None,
+ seller_trade_pubkey: None,
+ buyer_invoice: None,
+ created_at: Some(chrono::Utc::now().timestamp()),
+ expires_at: None,
+ buyer_token: None,
+ seller_token: None,
+ };
+
+ sqlx::query(
+ r#"
+ INSERT INTO orders (
+ id, kind, status, amount, fiat_code, fiat_amount,
+ payment_method, premium, created_at
+ ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
+ "#,
+ )
+ .bind(&order.id)
+ .bind(&order.kind)
+ .bind(&order.status)
+ .bind(order.amount)
+ .bind(&order.fiat_code)
+ .bind(order.fiat_amount)
+ .bind(&order.payment_method)
+ .bind(order.premium)
+ .bind(order.created_at)
+ .execute(pool)
+ .await?;
+
+ Ok(order)
+ }
+
+ pub async fn get_by_id(id: &str, pool: &SqlitePool) -> Result<Self, Box<dyn std::error::Error>> {
+ let order = sqlx::query_as::<_, Order>(
+ r#"
+ SELECT *
+ FROM orders
+ WHERE id = ?
+ "#,
+ )
+ .bind(id)
+ .fetch_one(pool)
+ .await?;
+
+ Ok(order)
+ }
+}Likely invalid or redundant comment.
src/util.rs (1)
511-520:
Ensure cross-platform compatibility and improve error handling
The current implementation needs better error handling and cross-platform support.
The previous review comment about using the dirs crate and improving error handling is still valid. Additionally, consider setting appropriate permissions on Unix systems.
| trade_keys, | ||
| trade_keys, |
There was a problem hiding this comment.
Possible Incorrect Key Usage in Function Call
In the call to send_order_id_cmd, trade_keys is passed as both identity_keys and trade_keys. If send_order_id_cmd expects separate identity_keys and trade_keys, using the same key for both might be incorrect. Please verify if identity_keys should be used as the second argument instead.
| my_key: &Keys, | ||
| identity_keys: &Keys, | ||
| trade_keys: &Keys, | ||
| trade_index: u32, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Add validation for trade_index using get_next_trade_index
The trade_index parameter should be validated against the next available trade index from the database. The codebase shows a get_next_trade_index function in src/db.rs that manages trade indices, but it's not being used for validation in execute_take_buy.
- Add validation in
execute_take_buyby comparing againstget_next_trade_indexbefore using the index - Consider adding error handling for cases where the index is invalid
🔗 Analysis chain
Validate trade_index parameter
The new trade_index parameter should be validated before use to ensure it references a valid trade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for trade_index validation patterns
rg -A 3 "trade_index.*valid|check.*trade"
Length of output: 43
Script:
#!/bin/bash
# Let's first check the usage context of trade_index in take_buy.rs
rg -A 5 "trade_index" src/cli/take_buy.rs
# Also search for any trade validation patterns in the codebase
rg -A 5 "trade.*valid|valid.*trade"
# Let's also look at the struct/function definition containing trade_index
ast-grep --pattern 'struct $_ {
$$$
trade_index: $_,
$$$
}'
Length of output: 422
Script:
#!/bin/bash
# Let's look at the full content of take_buy.rs to understand the context better
cat src/cli/take_buy.rs
# Also check for any trade-related error handling or validation in the codebase
rg -A 5 "Error.*trade|trade.*Error"
Length of output: 1473
Script:
#!/bin/bash
# Let's check the Message struct definition to understand how trade_index is used
ast-grep --pattern 'struct Message {
$$$
}'
# Also check the implementation of new_order
ast-grep --pattern 'impl Message {
$$$
fn new_order($$$) {
$$$
}
$$$
}'
Length of output: 133
| send_order_id_cmd( | ||
| client, trade_keys, trade_keys, mostro_key, message, false, false, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Passing same trade_keys twice is incorrect - one should be identity_keys
Based on the function definition in src/util.rs and usage patterns across the codebase, the function expects distinct keys:
- First parameter should be
identity_keys - Second parameter should be
trade_keys
All other calls in the codebase (take_buy.rs, take_sell.rs, add_invoice.rs, etc.) correctly pass identity_keys as the first key parameter. Only this instance in send_msg.rs incorrectly passes trade_keys twice.
🔗 Analysis chain
Review duplicate trade_keys usage in send_order_id_cmd.
The same trade_keys is passed twice to send_order_id_cmd. This might indicate:
- A potential bug where identity_keys should be used instead
- An API design that needs refinement
Let me check the function definition to understand the expected parameters.
Let me try a different approach to get the function definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other send_order_id_cmd calls to verify correct key usage
rg "send_order_id_cmd\(" --type rust -B 2 -A 2
Length of output: 1792
Script:
#!/bin/bash
# Get the function definition of send_order_id_cmd
ast-grep --pattern 'pub async fn send_order_id_cmd($$$)'
Length of output: 58
Script:
#!/bin/bash
# Get the function definition using ripgrep with context
rg "pub async fn send_order_id_cmd" -A 10 --type rust
Length of output: 476
grunch
changed the title
Table creation on ~/.mcli/mcli.db
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores