Rate user feature #404
Rate user feature #404
Conversation
…ostro and sent as event too
…ity key - need to improve check on message arrival for latest trade index
WalkthroughThis pull request introduces changes to the Mostro P2P trading system, focusing on user management and database interactions. The Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant Database
User->>App: Request User Action
App->>Database: Check/Create/Update User
alt User Exists
Database-->>App: Return User Data
else User Not Found
App->>Database: Add New User
Database-->>App: Confirm User Creation
end
App->>User: Respond to Action
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)src/app/rate_user.rs (5)
The counterpart identification logic has been enhanced with:
The error handling now uses
The implementation correctly:
The implementation correctly uses the
The constants Also applies to: 19-20 ✅ Verification successfulRating constants are consistently used across the codebase The verification shows that
The public visibility of these constants is justified as they are used across modules, maintaining a single source of truth for rating boundaries. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for usage of rating constants
rg "MAX_RATING|MIN_RATING" --type rust
# Search for other rating range checks
ast-grep --pattern 'if !($_ ..= $_).contains(&$_)'
Length of output: 486 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: 5
🧹 Nitpick comments (4)
src/app/rate_user.rs (2)
94-94: Possible confusion in variable naming
You are assigning counterpart to 'master_seller_pubkey' if message_sender matches the buyer. This is correct from a logic perspective (buyer is rating the seller), but the variable names are somewhat confusing. Consider adding minimal clarifying comments or renaming variables to help future maintainers quickly grasp the scenario (e.g., “the buyer is rating the seller, so the counterpart must be the master_seller_pubkey”).
161-173: Error handling improvements
You correctly return an error on update failure. Consider logging the user’s pubkey in the error message for easier debugging. The format string could incorporate user_to_vote.pubkey to quickly identify which user triggered the error.Proposed snippet:
- return Err(Error::msg(format!("Error updating user rating : {}", e))); + return Err(Error::msg(format!("Error updating user rating for {}: {}", user_to_vote.pubkey, e)));migrations/20231005195154_users.sql (1)
2-2: Pubkey as primary key
Switching from an ID to a pubkey as the primary key is sensible for unique user identification. Ensure all queries or references that previously used the old ID field are updated or removed to avoid schema mismatches.src/app/admin_add_solver.rs (1)
45-45: Check for proper feedback mechanism
This match statement logs solver creation or errors—good start. You might also consider sending a success/error message back to the admin. This helps keep the user experience consistent when attempting to add a solver.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)migrations/20231005195154_users.sql(1 hunks)src/app.rs(3 hunks)src/app/admin_add_solver.rs(2 hunks)src/app/rate_user.rs(3 hunks)src/db.rs(1 hunks)
🔇 Additional comments (7)
src/app/rate_user.rs (4)
4-4: Leverage database-based rating logic thoroughly
Your import of the new database functions (is_user_present, update_user_rating) is good because it centralizes rating logic in the database layer. Ensure that all references to the old reputation checking methods (like get_user_reputation) are removed or refactored.
97-97: Check for potential “None” unwrap
The usage of order.master_buyer_pubkey.unwrap() assumes a guaranteed presence of a master buyer pubkey. This might lead to runtime panics if the unwrap fails. A quick guard check or thorough validation will help prevent unexpected crashes.
151-160: Ensure consistent display for user rating
This block generates a new Rating event from the user record. Confirm that converting numeric types (u64 vs. f64, etc.) is consistent across the entire application (like to_tags and from_tags) so that no data is lost or truncated.
182-182: Validate usage flags for rating
You set buyer_rating or seller_rating to true based on the message sender. This call to update_user_rating_event uses those flags to set or skip updating the DB with rate flags. Make sure your logic covers all possible edge cases (e.g., what if a user changes their rating or attempts a second rating?).
Cargo.toml (1)
41-42: Handling a Git branch dependency
You’re switching mostro-core to a Git-based dependency on a specific branch. While this can speed integration with new changes, confirm you have pinned a commit or have robust version control to avoid potential breakages if the branch is force-pushed.
src/app/admin_add_solver.rs (1)
1-1: Adding new user creation logic
Importing add_new_user from db centralizes user creation in one place, promoting consistency across the codebase. Ensure that other references to user creation (including older CRUD calls) are removed or updated for uniform usage.
src/app.rs (1)
83-86: LGTM! Good error handling.
The error is properly logged when updating the user's trade index fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/db.rs (2)
330-354: Enhance input validation for last_trade_index.While the public key validation is good, consider adding validation for
last_trade_indexto ensure it's non-negative, and handle unique constraint violations explicitly.pub async fn add_new_user( pool: &SqlitePool, public_key: String, last_trade_index: i64, ) -> anyhow::Result<User> { // 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 last_trade_index < 0 { + return Err(anyhow::anyhow!("Trade index cannot be negative")); + } let created_at: Timestamp = Timestamp::now(); - let user = sqlx::query_as::<_, User>( + let result = sqlx::query_as::<_, User>( r#" INSERT INTO users (pubkey, last_trade_index, created_at) VALUES (?1, ?2, ?3) RETURNING * "#, ) .bind(public_key) .bind(last_trade_index) .bind(created_at.to_string()) - .fetch_one(pool) - .await?; + .fetch_one(pool) + .await; + match result { + Ok(user) => Ok(user), + Err(sqlx::Error::Database(e)) if e.to_string().contains("UNIQUE constraint") => { + Err(anyhow::anyhow!("User with this public key already exists")) + } + Err(e) => Err(e.into()), + } - Ok(user) }
356-376: Maintain consistent validation across functions.For consistency with
add_new_user, consider adding the same validation forpublic_keyandtrade_index.pub async fn update_user_trade_index( pool: &SqlitePool, public_key: String, trade_index: i64, ) -> anyhow::Result<User> { + // 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!("Trade index cannot be negative")); + } if let Ok(user) = sqlx::query_as::<_, User>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app.rs(3 hunks)src/db.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.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>
total_reviews before calculation Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/db.rs (3)
330-354: Add validation for last_trade_index and handle duplicate users.While the public key validation is good, consider these additional improvements:
- Validate that
last_trade_indexis non-negative- Handle the case where a user with the same public key already exists
pub async fn add_new_user( pool: &SqlitePool, public_key: String, last_trade_index: i64, ) -> anyhow::Result<User> { // 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 last_trade_index + if last_trade_index < 0 { + return Err(anyhow::anyhow!("Invalid last_trade_index: must be non-negative")); + } + // Check if user already exists + if let Ok(_) = is_user_present(pool, public_key.clone()).await { + return Err(anyhow::anyhow!("User already exists")); + } let created_at: Timestamp = Timestamp::now();
378-426: Improve SQL formatting and error handling.The function has good validation but could benefit from these improvements:
- Format the SQL query for better readability
- Improve error message to include the public key
- Consider wrapping the update in a transaction
if let Ok(user) = sqlx::query_as::<_, User>( r#" - UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6 + UPDATE users + SET + last_rating = ?1, + min_rating = ?2, + max_rating = ?3, + total_reviews = ?4, + total_rating = ?5 + WHERE pubkey = ?6 RETURNING * "#, ) .bind(last_rating) .bind(min_rating) .bind(max_rating) .bind(total_reviews) .bind(total_rating) - .bind(public_key) + .bind(public_key.clone()) .fetch_one(pool) .await{ Ok(user) } else { - Err(anyhow::anyhow!("No user found")) + Err(anyhow::anyhow!("No user found with public key: {}", public_key)) }Consider wrapping the update in a transaction to ensure atomicity:
let mut tx = pool.begin().await?; // Perform the update within the transaction let result = sqlx::query_as::<_, User>(...) .fetch_one(&mut tx) .await; tx.commit().await?;
330-426: Consider extracting common validation logic and adding logging.To improve maintainability and debugging:
- Extract the public key validation into a shared function
- Add logging for database operations
// Add at the top of the file use log::{debug, error, info}; // Extract validation into a shared function fn validate_public_key(public_key: &str) -> anyhow::Result<()> { if !public_key.chars().all(|c| c.is_ascii_hexdigit()) || public_key.len() != 64 { return Err(anyhow::anyhow!("Invalid public key format")); } Ok(()) }Then update the functions to use the shared validation and add logging:
pub async fn add_new_user(...) -> anyhow::Result<User> { - if !public_key.chars().all(|c| c.is_ascii_hexdigit()) || public_key.len() != 64 { - return Err(anyhow::anyhow!("Invalid public key format")); - } + validate_public_key(&public_key)?; + debug!("Adding new user with public key: {}", public_key); // ... rest of the function }
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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/db.rs (1)
389-439: Enhance error handling and use constants.Two suggestions for improvement:
- Add more detail to the "No user found" error message
- Define rating constants at the module level
+const MIN_RATING: i64 = 0; +const MAX_RATING: i64 = 5; 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: i64, ) -> anyhow::Result<User> { // ... existing validation code ... - if !(0..=5).contains(&last_rating) { + if !(MIN_RATING..=MAX_RATING).contains(&last_rating) { return Err(anyhow::anyhow!("Invalid rating value")); } // ... rest of the function ... else { - Err(anyhow::anyhow!("No user found")) + Err(anyhow::anyhow!("No user found with public key: {}", public_key)) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/rate_user.rs(3 hunks)src/db.rs(1 hunks)
🔇 Additional comments (3)
src/app/rate_user.rs (1)
161-172: LGTM! Error handling is well implemented.
The error handling for the database update is comprehensive and provides clear error messages.
src/db.rs (2)
330-354: LGTM! Well-structured user creation with proper validation.
The function includes:
- Proper public key validation
- Correct SQL query with RETURNING clause
- Appropriate error handling
356-387: LGTM! Comprehensive validation and error handling.
The function includes all necessary validations and proper error messages.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/rate_user.rs(3 hunks)
🔇 Additional comments (4)
src/app/rate_user.rs (4)
4-4: LGTM: Database imports align with PR objectives
The new imports support the transition to database-based user management as described in the PR objectives.
94-101: LGTM: Proper error handling implemented
The unwrap() calls have been replaced with ok_or_else() and proper error messages, improving error handling and preventing potential panics.
126-137: LGTM: Robust rating validation
The implementation properly validates the rating value against defined constants and provides clear error messages.
185-185: LGTM: Event handling properly integrated
The reputation event is correctly passed to the update function.
…t rumor pubkey - changed to float total rating in user table
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/db.rs (1)
389-439: Consider adding transaction support for rating updates.While the validation is comprehensive, the update should be atomic to prevent race conditions when multiple rating updates occur simultaneously.
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<User> { + let mut tx = pool.begin().await?; // ... validation code ... if let Ok(user) = sqlx::query_as::<_, User>( r#" UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6 RETURNING * "#, ) .bind(last_rating) .bind(min_rating) .bind(max_rating) .bind(total_reviews) .bind(total_rating) .bind(public_key) - .fetch_one(pool) + .fetch_one(&mut *tx) .await { + tx.commit().await?; Ok(user) } else { + tx.rollback().await?; Err(anyhow::anyhow!("No user found")) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
migrations/20231005195154_users.sql(1 hunks)src/app.rs(3 hunks)src/app/rate_user.rs(3 hunks)src/db.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.rs
🔇 Additional comments (7)
migrations/20231005195154_users.sql (2)
2-2: LGTM! Using pubkey as primary key aligns with the PR objectives.
The change from id to pubkey as the primary key is a good decision as it:
- Directly maps to user identity
- Eliminates the need for an additional index on pubkey
- Simplifies queries and joins
9-9: LGTM! Float type is more appropriate for average ratings.
Using float instead of integer for total_rating is correct as it:
- Allows for more precise average ratings
- Better represents fractional rating values
src/app/rate_user.rs (3)
94-101: LGTM! Improved error handling for missing master pubkeys.
The error handling has been enhanced by:
- Using
ok_or_elseinstead ofunwrap - Providing clear error messages
139-151: LGTM! Rating calculation has been fixed.
The rating calculation now correctly:
- Increments total reviews first
- Uses floating-point arithmetic for precise average calculation
- Maintains the running average using the correct formula
164-174: Verify error handling for database update failure.
The error handling for database update is good, but consider adding retry logic for transient failures.
src/db.rs (2)
330-354: LGTM! Well-implemented user creation with validation.
The function correctly:
- Validates public key format
- Uses parameterized queries
- Returns the created user
- Includes RETURNING clause
356-387: LGTM! Comprehensive validation and error handling.
The function includes:
- Public key format validation
- Trade index validation
- Clear error messages
- RETURNING clause
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/rate_user.rs (1)
126-137: Consider extracting error message to a constantThe rating validation logic is solid, but the error message could be extracted to a constant for consistency and reusability.
+ const RATING_RANGE_ERROR: &str = "Rating must be between {} and {}"; if !(MIN_RATING..=MAX_RATING).contains(&v) { return Err(Error::msg(format!( - "Rating must be between {} and {}", + RATING_RANGE_ERROR, MIN_RATING, MAX_RATING ))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/app/rate_user.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (3)
src/app/rate_user.rs (3)
4-4: LGTM: Clean DB module imports
The new imports align with the DB-centric approach for user rating management.
94-101: Previous error handling improvements verified
The unwrap() calls have been properly replaced with ok_or_else() as suggested in previous reviews.
183-183: LGTM: Proper event handling
The rating event is correctly created and passed to the update function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/db.rs (2)
330-354: Return all columns in the RETURNING clause.For consistency with other functions and to avoid an additional query to fetch the complete user data, modify the RETURNING clause to return all columns.
- RETURNING pubkey + RETURNING *
356-384: Consider returning User instead of bool for consistency.For consistency with other functions like
add_new_userandupdate_user_rating, consider returning the updated User record instead of a boolean. This would also provide more information to the caller.-) -> anyhow::Result<bool> { +) -> anyhow::Result<User> { // ... existing validations ... - let mut conn = pool.acquire().await?; - let rows_affected = sqlx::query!( + if let Ok(user) = sqlx::query_as::<_, User>( r#" UPDATE users SET last_trade_index = ?1 WHERE pubkey = ?2 + RETURNING * "#, - trade_index, - public_key, ) - .execute(&mut conn) - .await? - .rows_affected(); + .bind(trade_index) + .bind(public_key.clone()) + .fetch_one(pool) + .await + { + Ok(user) + } else { + Err(anyhow::anyhow!("No user found with public key: {}", public_key)) + } - Ok(rows_affected > 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
migrations/20231005195154_users.sql(1 hunks)sqlx-data.json(1 hunks)src/app.rs(3 hunks)src/db.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.rs
🔇 Additional comments (3)
migrations/20231005195154_users.sql (1)
2-2: Schema changes look good but verify data migration plan.
The changes to make pubkey the primary key and update total_rating to real are appropriate. However, ensure there's a data migration plan for existing users if this is not a new deployment.
Also applies to: 9-9
sqlx-data.json (1)
73-81: LGTM! Query metadata is correctly structured.
The new query for updating user's last_trade_index is properly configured with the correct parameter count and types.
src/db.rs (1)
386-436: LGTM! Comprehensive validation and proper error handling.
The function includes thorough validation of all rating parameters and proper error handling with clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/take_sell.rs (1)
130-132: Ensure buyer identity and trade index updates align with new user-tracking logic
Similar to the seller fields in take_buy, the buyer fields (master_buyer_pubkey and trade_index_buyer) are now attached to the order. Confirm that the database schema and user-record updates (e.g., update_user_trade_index) properly reflect these fields, so the buyer data is stored consistently and can be queried as intended.One approach is to add a helper function that ensures both the user is in the DB and their trade index is updated. This helps you avoid repeated validation and ensures consistent user management.
+// Potential pseudo-code snippet in src/db.rs or a relevant module: +pub async fn ensure_buyer_in_db(pool: &Pool<Sqlite>, buyer_pubkey: &str, buyer_trade_index: Option<u64>) -> Result<()> { + // check if user exists, if not, insert using add_new_user + // then if buyer_trade_index is Some, update the trade index + Ok(()) +}src/app/rate_user.rs (1)
156-179: Improve code structure: Update last_rating after database operationThe current implementation updates
last_ratingbefore the database operation. If the database update fails, the in-memory state would be inconsistent.Apply this diff to improve the code structure:
- // 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))); } + + // Update last_rating after successful database operation + 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()?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/rate_user.rs(3 hunks)src/app/take_buy.rs(1 hunks)src/app/take_sell.rs(1 hunks)
🔇 Additional comments (3)
src/app/rate_user.rs (3)
4-4: LGTM: Database module imports
The new imports align with the PR's objective of moving user management to the database.
94-114: LGTM: Improved error handling
The changes properly handle potential errors by:
- Using
ok_or_else()instead ofunwrap() - Sending appropriate error messages to users
150-155:
Critical: Fix rating calculation formula
The current implementation has two issues:
- It incorrectly uses
last_ratinginstead of the newratingvalue - The formula doesn't properly calculate a rolling average
Apply this fix:
user_to_vote.total_reviews += 1;
- let old_rating = user_to_vote.total_rating as f64;
- // recompute new rating
- user_to_vote.total_rating = old_rating
- + ((user_to_vote.last_rating as f64) - old_rating) / (user_to_vote.total_reviews as f64);
+ // Calculate new rolling average
+ user_to_vote.total_rating = (
+ (user_to_vote.total_rating * (user_to_vote.total_reviews - 1) as f64)
+ + rating as f64
+ ) / user_to_vote.total_reviews as f64;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sqlx-data.json (1)
13-22: Add RETURNING clause to UPDATE queries for better error handling.Consider adding RETURNING clause to both UPDATE queries to return the updated user record, which would help in verifying the update and provide better error handling.
Apply these diffs:
- "query": "\n UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6\n " + "query": "\n UPDATE users SET last_rating = ?1, min_rating = ?2, max_rating = ?3, total_reviews = ?4, total_rating = ?5 WHERE pubkey = ?6 RETURNING *\n "- "query": "\n UPDATE users SET last_trade_index = ?1 WHERE pubkey = ?2\n " + "query": "\n UPDATE users SET last_trade_index = ?1 WHERE pubkey = ?2 RETURNING *\n "Also applies to: 82-91
src/db.rs (1)
357-385: Improve error message for non-existent user.The function should provide a more descriptive error message when the user is not found.
Apply this diff:
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) + if rows_affected > 0 { + Ok(true) + } else { + Err(anyhow::anyhow!("No user found with public key: {}", public_key)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sqlx-data.json(2 hunks)src/app/rate_user.rs(4 hunks)src/db.rs(2 hunks)
🔇 Additional comments (6)
src/app/rate_user.rs (4)
Line range hint 4-18: LGTM! Dependencies are properly organized.
The imports are well-organized and include all necessary dependencies.
19-20: LGTM! Constants are properly defined.
The rating range constants are well-defined and used consistently throughout the code.
94-114: LGTM! Error handling is improved.
The error handling for missing master pubkeys has been improved by using ok_or_else with descriptive error messages.
145-162:
Critical: Fix rating calculation formula.
The current implementation has two issues:
- It uses
last_ratinginstead of the newratingvalue in the calculation - The formula doesn't properly compute a rolling average
Apply this diff to fix the rating calculation:
- 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();
- } else {
- user_to_vote.total_rating = old_rating
- + ((user_to_vote.last_rating as f64) - old_rating)
- / (user_to_vote.total_reviews as f64);
- }
- // Store last rating
- user_to_vote.last_rating = rating.into();
+ // Store last rating
+ user_to_vote.last_rating = rating;
+ // Calculate new rolling average
+ user_to_vote.total_rating = if user_to_vote.total_reviews <= 1 {
+ rating as f64
+ } else {
+ ((user_to_vote.total_rating * (user_to_vote.total_reviews - 1) as f64)
+ + rating as f64) / user_to_vote.total_reviews as f64
+ };Likely invalid or redundant comment.
src/db.rs (2)
331-355: LGTM! User creation is well-implemented.
The function properly validates the public key format and includes all necessary fields for user creation.
387-434: LGTM! Rating update is well-implemented with comprehensive validation.
The function includes thorough validation for all rating parameters and proper error messages.
There was a problem hiding this comment.
Hi guys, great work!
In the 38383 events it always appears "rating", "none" , but in the database there are rates. I'm sure you've already seen that.
Minor things:
- If a user tries to rate himself with a pending order of his (I don't think many try it), this appears in the logs:
2024-12-24T01:11:16.230136Z WARN mostrod::app: Error in RateUser with context Missing buyer pubkey // if is a purchase order: Missing seller pubkey
Maybe it should return CantDo
I tried to break the rate and mostrod but I couldn't :) for now everything else I saw is from mostro-cli and I left it in the review of the rate-user PR in mostro-cli
Did not see! Thanks @Catrya will fix that! |
There was a problem hiding this comment.
Hi guys, I found some other things:
-
In the 38383 rate event when an order is completed, in the
dtag, the users' i0 pubkey is being published.
I think that if reputation is not going to be left as something global, but rather each user has a different reputation in each mostro, then there is no need to leave that event, even if it were published in thedtag of the trade key, what do you think? -
After restarting the mostro database, I get this in the logs
2024-12-24T20:08:05.254586Z ERROR mostrod::app: Error creating new user: no column found for name: is_admin
First of all... why working on Xmas eve? 🤣 |
I observed this too today. Entry in db seems ok but there is that error raising when a new user is saved the first time. Will delve in this after Xmas! |
haha look who's talking! 😂In Cuba we don't celebrate Christmas like in other countries. Anyway, I'm done for today💆♀️ Merry christmas for you :) |
please let's keep posting the rate on nostr for now, but we should use the |
It's ok to show messages like that in logs, CantDo is used on messages not in logs |
I meant to send the message CantDo to the user, and in the logs leave it as it is. But in any case a user who tries to rate a self order in status pending knows that he cannot do it, so it is not so important to mostrod respond with CantDo |
Hi @grunch ,
this is related to :
MostroP2P/mostro-core#73
i tested that now new users are added correctly when they send a new message to
mostrod, need to check more what happens with latest trade index. Now users haveidentity keyad primary key in db, i removed crud from users in mostro-core which was limiting the use ofstring.You will see
cargo.tomlpointing tomostro-corebranch to make this work.Summary by CodeRabbit
New Features
Bug Fixes
Documentation