From 51caba5c3b944607e3c99734ab49e9bf5ca57f12 Mon Sep 17 00:00:00 2001 From: arkanoider <113362043+arkanoider@users.noreply.github.com> Date: Thu, 26 Dec 2024 19:56:31 +0100 Subject: [PATCH] Rate user feature (#404) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Rate-user: first draft modification to test - user rate is saved to mostro and sent as event too * rate user improve: users now are saved correctly based on their identity key - need to improve check on message arrival for latest trade index * some rabbit things... * Update src/db.rs Add comprehensive validation for rating parameters. Consider adding validation for all rating parameters to ensure data consistency: Validate min_rating and max_rating range Ensure min_rating <= last_rating <= max_rating Validate total_rating against total_reviews Add public_key format validation Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/app/rate_user.rs total_reviews before calculation Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/db.rs Add consistent validation and improve error messages. For consistency with other functions and better error handling: Add public key validation (same as in add_new_user and update_user_rating) Add trade index validation Improve error message to include the public key Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix: calculation of user rating and last rating save on db * Fix: improved checks in rate order * Fix: math corrected with all rabbit suggestion * Fix: wrong check on incoming messages, need to check signature against rumor pubkey - changed to float total rating in user table * Fix total_rating calculation * Fix: last trade index check working * Comment on test_get_nostr_client_failure test - fails on gh actions locally working weird * Fix test: modified not passing test * Bug fixes found on this PR review * Fix: first review special case - fix on user rate update for sqlx issue documented * Fix: rate user now updates correctly max and min rate of a user in db * Shows trade public key on rate event --------- Co-authored-by: Francisco Calderón Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- Cargo.lock | 4 +- Cargo.toml | 2 +- migrations/20231005195154_users.sql | 5 +- sqlx-data.json | 20 +++++ src/app.rs | 30 +++++-- src/app/admin_add_solver.rs | 4 +- src/app/rate_user.rs | 130 ++++++++++++++++++---------- src/app/take_buy.rs | 4 +- src/app/take_sell.rs | 3 + src/db.rs | 106 +++++++++++++++++++++++ src/util.rs | 6 +- 11 files changed, 251 insertions(+), 63 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2471597a..16ce5157 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1991,9 +1991,9 @@ dependencies = [ [[package]] name = "mostro-core" -version = "0.6.18" +version = "0.6.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "365f4109979a283c0f198befaa9a518edb346873ead95fca742fb7cc5eb65c81" +checksum = "598112169173ad9c97abab86cb9b1aed551e78201bb938df5cd845f35edc0c08" dependencies = [ "anyhow", "bitcoin", diff --git a/Cargo.toml b/Cargo.toml index 1279c2e0..2e819894 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,7 @@ uuid = { version = "1.8.0", features = [ "serde", ] } reqwest = { version = "0.12.1", features = ["json"] } -mostro-core = { version = "0.6.18", features = ["sqlx"] } +mostro-core = { version = "0.6.19", features = ["sqlx"] } tracing = "0.1.40" tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } config = "0.14.0" diff --git a/migrations/20231005195154_users.sql b/migrations/20231005195154_users.sql index f3941c40..6c825469 100644 --- a/migrations/20231005195154_users.sql +++ b/migrations/20231005195154_users.sql @@ -1,13 +1,12 @@ CREATE TABLE IF NOT EXISTS users ( - id char(36) primary key not null, - pubkey char(64) unique not null, + pubkey char(64) primary key not null, is_admin integer not null default 0, is_solver integer not null default 0, is_banned integer not null default 0, category integer not null default 0, last_trade_index integer not null default 0, total_reviews integer not null default 0, - total_rating integer not null default 0, + total_rating real not null default 0.0, last_rating integer not null default 0, max_rating integer not null default 0, min_rating integer not null default 0, diff --git a/sqlx-data.json b/sqlx-data.json index ab16f536..48a96de2 100644 --- a/sqlx-data.json +++ b/sqlx-data.json @@ -10,6 +10,16 @@ }, "query": "\n UPDATE orders\n SET\n master_seller_pubkey = ?1\n WHERE id = ?2\n " }, + "071917afd7e1d00e2ae860779a1471d281f7aa45da61f7a7bdbd4625a3966a5d": { + "describe": { + "columns": [], + "nullable": [], + "parameters": { + "Right": 6 + } + }, + "query": "\n UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6\n " + }, "77ea98f6af16fa6e5a7d604965593700c563f88d88cb10b348bdc4200c87ad1d": { "describe": { "columns": [], @@ -69,5 +79,15 @@ } }, "query": "\n UPDATE orders\n SET\n seller_pubkey = ?1\n WHERE id = ?2\n " + }, + "fb8c9a8f42f6fc3c70e734db9db4af88c9602143afdbfcf57562f3fecb80b3a7": { + "describe": { + "columns": [], + "nullable": [], + "parameters": { + "Right": 2 + } + }, + "query": "\n UPDATE users SET last_trade_index = ?1 WHERE pubkey = ?2\n " } } \ No newline at end of file diff --git a/src/app.rs b/src/app.rs index 2f8e1b86..94ff6e49 100644 --- a/src/app.rs +++ b/src/app.rs @@ -31,7 +31,9 @@ use crate::app::release::release_action; use crate::app::take_buy::take_buy_action; use crate::app::take_sell::take_sell_action; +use crate::db::update_user_trade_index; // Core functionality imports +use crate::db::add_new_user; use crate::lightning::LndConnector; use crate::nip59::unwrap_gift_wrap; use crate::util::send_cant_do_msg; @@ -44,7 +46,6 @@ use mostro_core::message::{Action, CantDoReason, Message}; use mostro_core::user::User; use nostr_sdk::prelude::*; use sqlx::{Pool, Sqlite}; -use sqlx_crud::Crud; use std::sync::Arc; use tokio::sync::Mutex; /// Helper function to log warning messages for action errors @@ -76,11 +77,17 @@ async fn check_trade_index(pool: &Pool, event: &UnwrappedGift, msg: &Mes if index > user.last_trade_index && msg .get_inner_message_kind() - .verify_signature(event.sender, sig) + .verify_signature(event.rumor.pubkey, sig) { user.last_trade_index = index; - if user.update(pool).await.is_ok() { - tracing::info!("Update user trade index"); + if let Err(e) = update_user_trade_index( + pool, + event.sender.to_string(), + user.last_trade_index, + ) + .await + { + tracing::error!("Error updating user trade index: {}", e); } } else { tracing::info!("Invalid signature or trade index"); @@ -96,13 +103,22 @@ async fn check_trade_index(pool: &Pool, event: &UnwrappedGift, msg: &Mes } Err(_) => { if let (true, last_trade_index) = message_kind.has_trade_index() { - let new_user = User { + let new_user: User = User { pubkey: event.sender.to_string(), last_trade_index, ..Default::default() }; - if new_user.create(pool).await.is_ok() { - tracing::info!("Added new user for rate"); + if let Err(e) = + add_new_user(pool, new_user.pubkey, new_user.last_trade_index).await + { + tracing::error!("Error creating new user: {}", e); + send_cant_do_msg( + None, + msg.get_inner_message_kind().id, + Some(CantDoReason::InvalidTextMessage), + &event.rumor.pubkey, + ) + .await; } } } diff --git a/src/app/admin_add_solver.rs b/src/app/admin_add_solver.rs index 22e7d1e6..3bfb9d71 100644 --- a/src/app/admin_add_solver.rs +++ b/src/app/admin_add_solver.rs @@ -1,3 +1,4 @@ +use crate::db::add_new_user; use crate::util::{send_cant_do_msg, send_dm}; use anyhow::Result; @@ -6,7 +7,6 @@ use mostro_core::user::User; use nostr::nips::nip59::UnwrappedGift; use nostr_sdk::prelude::*; use sqlx::{Pool, Sqlite}; -use sqlx_crud::Crud; use tracing::{error, info}; pub async fn admin_add_solver_action( @@ -42,7 +42,7 @@ pub async fn admin_add_solver_action( let public_key = PublicKey::from_bech32(npubkey)?.to_hex(); let user = User::new(public_key, 0, 1, 0, 0, trade_index); // Use CRUD to create user - match user.create(pool).await { + match add_new_user(pool, user.pubkey, user.last_trade_index).await { Ok(r) => info!("Solver added: {:#?}", r), Err(ee) => error!("Error creating solver: {:#?}", ee), } diff --git a/src/app/rate_user.rs b/src/app/rate_user.rs index 52e63bfe..bb2e3438 100644 --- a/src/app/rate_user.rs +++ b/src/app/rate_user.rs @@ -1,8 +1,9 @@ use crate::util::{send_cant_do_msg, send_new_order_msg, update_user_rating_event}; use crate::NOSTR_CLIENT; +use crate::db::{is_user_present, update_user_rating}; use anyhow::{Error, Result}; -use mostro_core::message::{Action, Message, Payload}; +use mostro_core::message::{Action, CantDoReason, Message, Payload}; use mostro_core::order::{Order, Status}; use mostro_core::rating::Rating; use mostro_core::NOSTR_REPLACEABLE_EVENT_KIND; @@ -15,8 +16,8 @@ use std::time::Duration; use tokio::sync::Mutex; use tracing::error; -const MAX_RATING: u8 = 5; -const MIN_RATING: u8 = 1; +pub const MAX_RATING: u8 = 5; +pub const MIN_RATING: u8 = 1; pub async fn get_user_reputation(user: &str, my_keys: &Keys) -> Result> { // Request NIP33 of the counterparts @@ -85,22 +86,39 @@ pub async fn update_user_reputation_action( } // Get counterpart pubkey let mut counterpart: String = String::new(); + let mut counterpart_trade_pubkey: String = String::new(); let mut buyer_rating: bool = false; let mut seller_rating: bool = false; // Find the counterpart public key if message_sender == buyer { - counterpart = seller; + counterpart = order + .master_seller_pubkey + .ok_or_else(|| Error::msg("Missing master seller pubkey"))?; buyer_rating = true; + counterpart_trade_pubkey = order + .buyer_pubkey + .ok_or_else(|| Error::msg("Missing buyer pubkey"))?; } else if message_sender == seller { - counterpart = buyer; + counterpart = order + .master_buyer_pubkey + .ok_or_else(|| Error::msg("Missing master buyer pubkey"))?; seller_rating = true; + counterpart_trade_pubkey = order + .seller_pubkey + .ok_or_else(|| Error::msg("Missing seller pubkey"))?; }; // Add a check in case of no counterpart found if counterpart.is_empty() { // We create a Message - send_cant_do_msg(request_id, Some(order.id), None, &event.rumor.pubkey).await; + send_cant_do_msg( + request_id, + Some(order.id), + Some(CantDoReason::InvalidPeer), + &event.rumor.pubkey, + ) + .await; return Ok(()); }; @@ -118,53 +136,77 @@ pub async fn update_user_reputation_action( }; // Check if content of Peer is the same of counterpart - let rating; - - if let Some(Payload::RatingUser(v)) = msg.get_inner_message_kind().payload.to_owned() { - if !(MIN_RATING..=MAX_RATING).contains(&v) { - return Err(Error::msg(format!( - "Rating must be between {} and {}", - MIN_RATING, MAX_RATING - ))); - } - rating = v; + let rating = + if let Some(Payload::RatingUser(v)) = msg.get_inner_message_kind().payload.to_owned() { + if !(MIN_RATING..=MAX_RATING).contains(&v) { + return Err(Error::msg(format!( + "Rating must be between {} and {}", + MIN_RATING, MAX_RATING + ))); + } + v + } else { + return Err(Error::msg("No rating present")); + }; + + // Get counter to vote from db + let mut user_to_vote = is_user_present(pool, counterpart.clone()).await?; + + // Update user reputation + // Going on with calculation + // increment first + user_to_vote.total_reviews += 1; + let old_rating = user_to_vote.total_rating as f64; + // recompute new rating + if user_to_vote.total_reviews <= 1 { + user_to_vote.total_rating = rating.into(); + user_to_vote.max_rating = rating.into(); + user_to_vote.min_rating = rating.into(); } else { - return Err(Error::msg("No rating present")); + user_to_vote.total_rating = old_rating + + ((user_to_vote.last_rating as f64) - old_rating) + / (user_to_vote.total_reviews as f64); + if user_to_vote.max_rating < rating.into() { + user_to_vote.max_rating = rating.into(); + } + if user_to_vote.min_rating > rating.into() { + user_to_vote.min_rating = rating.into(); + } } - - // Ask counterpart reputation - let rep = get_user_reputation(&counterpart, my_keys).await?; - // Here we have to update values of the review of the counterpart - let mut reputation; - - if let Some(r) = rep { - // Update user reputation - // Going on with calculation - reputation = r; - let old_rating = reputation.total_rating; - let last_rating = reputation.last_rating; - let new_rating = - old_rating + (last_rating as f64 - old_rating) / (reputation.total_reviews as f64); - - reputation.last_rating = rating; - reputation.total_reviews += 1; - // Format with two decimals - let new_rating = format!("{:.2}", new_rating).parse::()?; - - // Assing new total rating to review - reputation.total_rating = new_rating; - } else { - reputation = Rating::new(1, rating as f64, rating, MIN_RATING, MAX_RATING); + // Store last rating + user_to_vote.last_rating = rating.into(); + // Create new rating event + let reputation_event = Rating::new( + user_to_vote.total_reviews as u64, + user_to_vote.total_rating as f64, + user_to_vote.last_rating as u8, + user_to_vote.min_rating as u8, + user_to_vote.max_rating as u8, + ) + .to_tags()?; + + // Save new rating to db + if let Err(e) = update_user_rating( + pool, + user_to_vote.pubkey, + user_to_vote.last_rating, + user_to_vote.min_rating, + user_to_vote.max_rating, + user_to_vote.total_reviews, + user_to_vote.total_rating, + ) + .await + { + return Err(Error::msg(format!("Error updating user rating : {}", e))); } - let reputation = reputation.to_tags()?; if buyer_rating || seller_rating { // Update db with rate flags update_user_rating_event( - &counterpart, + &counterpart_trade_pubkey, update_buyer_rate, update_seller_rate, - reputation, + reputation_event, order.id, my_keys, pool, diff --git a/src/app/take_buy.rs b/src/app/take_buy.rs index 05938210..05830afc 100644 --- a/src/app/take_buy.rs +++ b/src/app/take_buy.rs @@ -101,7 +101,9 @@ pub async fn take_buy_action( order.fee = fee; } - // Update trade index for seller + // Add seller identity pubkey to order + order.master_seller_pubkey = Some(event.sender.to_string()); + // Add seller trade index to order order.trade_index_seller = msg.get_inner_message_kind().trade_index; // Timestamp order take time order.taken_at = Timestamp::now().as_u64() as i64; diff --git a/src/app/take_sell.rs b/src/app/take_sell.rs index 9dc5fbc9..95b0b5d6 100644 --- a/src/app/take_sell.rs +++ b/src/app/take_sell.rs @@ -127,6 +127,9 @@ pub async fn take_sell_action( // Add buyer pubkey to order order.buyer_pubkey = Some(buyer_trade_pubkey.to_string()); + // Add buyer identity pubkey to order + order.master_buyer_pubkey = Some(event.sender.to_string()); + // Add buyer trade index to order order.trade_index_buyer = msg.get_inner_message_kind().trade_index; // Timestamp take order time order.taken_at = Timestamp::now().as_u64() as i64; diff --git a/src/db.rs b/src/db.rs index 1485b396..31132892 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,3 +1,4 @@ +use crate::app::rate_user::{MAX_RATING, MIN_RATING}; use mostro_core::dispute::Dispute; use mostro_core::order::Order; use mostro_core::order::Status; @@ -327,6 +328,111 @@ pub async fn is_user_present(pool: &SqlitePool, public_key: String) -> anyhow::R Ok(user) } +pub async fn add_new_user( + pool: &SqlitePool, + public_key: String, + last_trade_index: i64, +) -> anyhow::Result { + // Validate public key format (32-bytes hex) + if !public_key.chars().all(|c| c.is_ascii_hexdigit()) || public_key.len() != 64 { + return Err(anyhow::anyhow!("Invalid public key format")); + } + let created_at: Timestamp = Timestamp::now(); + let user = sqlx::query_as::<_, User>( + r#" + INSERT INTO users (pubkey, last_trade_index, created_at) + VALUES (?1, ?2, ?3) + RETURNING pubkey + "#, + ) + .bind(public_key) + .bind(last_trade_index) + .bind(created_at.to_string()) + .fetch_one(pool) + .await?; + + Ok(user) +} + +pub async fn update_user_trade_index( + pool: &SqlitePool, + public_key: String, + trade_index: i64, +) -> anyhow::Result { + // Validate public key format (32-bytes hex) + if !public_key.chars().all(|c| c.is_ascii_hexdigit()) || public_key.len() != 64 { + return Err(anyhow::anyhow!("Invalid public key format")); + } + // Validate trade_index + if trade_index < 0 { + return Err(anyhow::anyhow!("Invalid trade_index: must be non-negative")); + } + + let mut conn = pool.acquire().await?; + + let rows_affected = sqlx::query!( + r#" + UPDATE users SET last_trade_index = ?1 WHERE pubkey = ?2 + "#, + trade_index, + public_key, + ) + .execute(&mut conn) + .await? + .rows_affected(); + + Ok(rows_affected > 0) +} + +pub async fn update_user_rating( + pool: &SqlitePool, + public_key: String, + last_rating: i64, + min_rating: i64, + max_rating: i64, + total_reviews: i64, + total_rating: f64, +) -> anyhow::Result { + // Validate public key format (32-bytes hex) + if !public_key.chars().all(|c| c.is_ascii_hexdigit()) || public_key.len() != 64 { + return Err(anyhow::anyhow!("Invalid public key format")); + } + // Validate rating values + if !(0..=5).contains(&last_rating) { + return Err(anyhow::anyhow!("Invalid rating value")); + } + if !(0..=5).contains(&min_rating) || !(0..=5).contains(&max_rating) { + return Err(anyhow::anyhow!("Invalid min/max rating values")); + } + if MIN_RATING as i64 > last_rating || last_rating > MAX_RATING as i64 { + return Err(anyhow::anyhow!( + "Rating values must satisfy: min_rating <= last_rating <= max_rating" + )); + } + if total_reviews < 0 { + return Err(anyhow::anyhow!("Invalid total reviews")); + } + if total_rating < 0.0 || total_rating > (total_reviews * 5) as f64 { + return Err(anyhow::anyhow!("Invalid total rating")); + } + let rows_affected = sqlx::query!( + r#" + UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6 + "#, + last_rating, + min_rating, + max_rating, + total_reviews, + total_rating, + public_key, + ) + .execute(pool) + .await? + .rows_affected(); + + Ok(rows_affected > 0) +} + pub async fn is_assigned_solver( pool: &SqlitePool, solver_pubkey: &str, diff --git a/src/util.rs b/src/util.rs index 081af31a..f059428a 100644 --- a/src/util.rs +++ b/src/util.rs @@ -778,9 +778,9 @@ mod tests { #[tokio::test] async fn test_get_nostr_client_failure() { initialize(); - // Assuming NOSTR_CLIENT is not initialized - let client = get_nostr_client(); - assert!(client.is_err()); + // Ensure NOSTR_CLIENT is not initialized for the test + let client = NOSTR_CLIENT.get(); + assert!(client.is_none()); } #[tokio::test]