Feat: issue #437 implementation#445
Conversation
WalkthroughThis update removes the user reputation fetching functionality from the rating module by eliminating the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/util.rs (1)
46-47: Consider preserving original error details.Here, errors are mapped without capturing the underlying error message. Preserving the original error details often helps with debugging and troubleshooting.
Below is a sample diff demonstrating how to retain the original error message:
- .map_err(|_| MostroInternalErr(ServiceError::NoAPIResponse))? + .map_err(|err| MostroInternalErr(ServiceError::NoAPIResponse(err.to_string())))? - .map_err(|_| MostroInternalErr(ServiceError::MalformedAPIRes))? + .map_err(|err| MostroInternalErr(ServiceError::MalformedAPIRes(err.to_string())))? - .map_err(|_| MostroInternalErr(ServiceError::NoAPIResponse))?; + .map_err(|err| MostroInternalErr(ServiceError::NoAPIResponse(err.to_string())))?;Also applies to: 49-50, 60-60
src/nip33.rs (1)
39-41: Floating-point rating format could be refined.Switching from
Option<Rating>toOption<f64>simplifies the flow but consider limiting precision (e.g., rounding) to avoid overly long decimal representations in the tag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/rate_user.rs(0 hunks)src/nip33.rs(2 hunks)src/util.rs(6 hunks)
💤 Files with no reviewable changes (1)
- src/app/rate_user.rs
🔇 Additional comments (5)
src/util.rs (4)
4-4: Praise for newly introduced imports.These additional imports for database checks, invoice states, and
HashMapusage look appropriate and align well with the rest of the code changes.Also applies to: 16-16, 28-28
42-42: Enhanced error-handling signature.Changing the function to return
Result<_, MostroError>is a solid improvement, ensuring consistent and descriptive error handling throughout the codebase.
203-205: User presence check aligned with new approach.Loading user data from the database harmonizes with the removal of the outdated reputation-fetching method and ensures accurate rating retrieval.
208-208: Passing user rating for event tags looks correct.Supplying
user.total_ratingtoorder_to_tagsnaturally integrates the user's database-sourced reputation into the event.src/nip33.rs (1)
68-68: Function signature update streamlines rating usage.Using
Option<f64>instead of a customRatingstruct is consistent with the updated user reputation approach across the codebase.
| .await | ||
| .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; | ||
| // We transform the order fields to tags to use in the event | ||
| let tags = order_to_tags(&new_order_db, reputation); |
There was a problem hiding this comment.
We need to show to the counterpart 3 fields, total_rating, total_reviews and days_operating, we need to refactor to that
There was a problem hiding this comment.
[
"rating",
"4.5"
],
[
"reviews",
"54"
],
[
"days_operating",
"456"
],Is it good like this @grunch ?
Days operating filed I suppose it's calculated from user creation time in mostrod users table in database right?
Catrya
left a comment
There was a problem hiding this comment.
Hi @arkanoider when i try to create a new order this appears in mostrod logs
2025-02-19T15:53:44.827163Z WARN mostrod::app: Error in NewOrder with context Order id not present in database
83e689f to
849e233
Compare
I did a big shit! :) Will check asap...in office I was not able to test. |
|
@Catrya give another try to this please. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/nip33.rs (2)
39-50: Add validation and improve robustness of operating days calculation.The current implementation has potential edge cases:
- No validation for future timestamps
- Possible i32 overflow for very old accounts
- Potential loss of precision due to integer division
Consider this more robust implementation:
-fn calculate_operating_days(created_at: i64) -> i32 { +fn calculate_operating_days(created_at: i64) -> i64 { let now = Timestamp::now(); - let days = (now.as_u64() - created_at as u64) / 86400; - days as i32 + if created_at > now.as_i64() { + return 0; + } + (now.as_i64() - created_at) / 86400 }
73-73: Improve maintainability of reputation data handling.The implementation correctly handles the reputation data tuple, but could benefit from named constants and type definitions for better maintainability.
Consider adding these improvements:
type ReputationData = (f64, i64, i64); // (rating, review_count, created_at) const DEFAULT_RATING: &str = "0"; const DEFAULT_REVIEWS: &str = "0"; pub fn order_to_tags(order: &Order, reputation_data: Option<ReputationData>) -> Tags { // ... existing code ... }This makes the code more self-documenting and easier to maintain.
Also applies to: 105-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/nip33.rs(3 hunks)src/util.rs(6 hunks)
🔇 Additional comments (3)
src/util.rs (3)
39-63: LGTM! Error handling improvements.The changes enhance error handling by making errors explicit in the return type and using
map_errfor cleaner error conversion.
203-210: LGTM! User reputation handling improvements.The changes correctly implement the new user rating system by:
- Using
is_user_presentto fetch user data- Passing complete user data (rating, reviews, creation date) to
order_to_tags
4-4: LGTM! Import adjustments.The import changes correctly support the new functionality:
is_user_presentfor the new user rating systemInvoiceStatefor invoice state handlingHashMapfor collectionsAlso applies to: 16-16, 28-28
|
Modified [
"rating",
"{\"total_reviews\":1,\"total_rating\":3.0,\"days\":33}"
], |
Catrya
left a comment
There was a problem hiding this comment.
when a order is in pending status is ok
"[\"rating\",{\"total_reviews\":1,\"total_rating\":5.0,\"days\":0}]"
But when changes the status the Days are wrong:
"rating",
"[\"rating\",{\"total_reviews\":0,\"total_rating\":0.0,\"days\":20138}]"
|
I was thinking that it would be good to add to the message that the Maker receives to pay the hold invoice if he is the seller, or to put the invoice if he is the buyer, the reputation of the Taker, the number of ratings and the days he has been using a Mostro. Do you think it would be better if I open an issue for that or that it is addressed within this same PR? |
Ok got it, just need to manage the Maybe we can always add rating data to all messages related to orders? What do you think? |
Mmmmhhh...i will try to answer to myself, maybe it was correct to have rating tagvonly in a new order event the reputation of the maker, which makes sense because the taker can see it and use it to take a decision. After taking an order all the event update of the could be without rating tag ( or set to null or zero ), maybe just when we have a child new order from a range order we can publish it back for the new child order. The "bad" thing in this case is that rating is the same as before because users won't have sent the ratings to each other. |
…te it. During trade is not showed ( i think it's no more meaningful during status update ) - bonus removed another ref to anyhow, target is to remove dep completely
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/nip33.rs (1)
131-140: Consider moving the rating tag insertion position.The rating tag is currently inserted at a hardcoded position (index 7). This could break if the tags vector structure changes.
Consider using a more flexible approach:
- tags.insert(7, Tag::custom( + tags.push(Tag::custom( TagKind::Custom(Cow::Borrowed("rating")), vec![create_rating_tag(reputation_data)], ));src/app/release.rs (1)
382-413: Consider extracting user rating logic to a separate function.The user rating extraction logic in
create_order_eventis complex and could benefit from being moved to a dedicated function.Consider refactoring like this:
+async fn get_user_rating_data(order: &Order, pool: &Pool<Sqlite>) -> Result<(f64, i64, i64), MostroError> { + let identity_pubkey = match order.is_sell_order() { + Ok(_) => order.get_master_seller_pubkey()?, + Err(_) => order.get_master_buyer_pubkey()?, + }; + + let user = crate::db::is_user_present(pool, identity_pubkey.to_string()).await?; + Ok((user.total_rating, user.total_reviews, user.created_at)) +} + async fn create_order_event(new_order: &mut Order, my_keys: &Keys) -> Result<Event, MostroError> { let pool = match crate::db::connect().await { Ok(p) => p, Err(e) => return Err(MostroInternalErr(ServiceError::DbAccessError(e.to_string()))), }; - let identity_pubkey = match new_order.is_sell_order() { - Ok(_) => new_order.get_master_seller_pubkey().map_err(MostroInternalErr)?, - Err(_) => new_order.get_master_buyer_pubkey().map_err(MostroInternalErr)?, - }; - - let user = crate::db::is_user_present(&pool, identity_pubkey.to_string()) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + let rating_data = get_user_rating_data(new_order, &pool) + .await + .map(Some) + .unwrap_or(None); - let tags = crate::nip33::order_to_tags( - new_order, - Some((user.total_rating, user.total_reviews, user.created_at)), - ); + let tags = crate::nip33::order_to_tags(new_order, rating_data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/release.rs(5 hunks)src/lnurl.rs(3 hunks)src/nip33.rs(4 hunks)
🔇 Additional comments (3)
src/lnurl.rs (1)
35-35: LGTM! Error handling improvements look good.The changes standardize error handling by:
- Using explicit
MostroErrorreturn type- Consistently mapping errors to specific
ServiceErrorvariants- Using
map_errfor error transformationAlso applies to: 45-45, 51-51, 67-67, 70-75
src/nip33.rs (1)
47-56: LGTM! Well-structured rating tag implementation.The new
create_rating_tagfunction:
- Uses JSON for structured data representation
- Includes comprehensive rating metrics (total_reviews, total_rating, days)
- Handles optional reputation data gracefully
src/app/release.rs (1)
204-208: LGTM! Improved error handling for payment request validation.The change properly handles missing payment requests by returning a specific
InvoiceInvalidError.
|
@Catrya if you can do a round of test with latest here let me know.
Let me know your thoughts and results. Removed other use of |
Hi @arkanoider I like rating appears only in But, I have not been able to test it when a child order is published because there is a bug that does not allow a child order to be published when it is by range. |
|
Well, I tested it again, for me it's ok The bug that I thought wouldn't let me try it out isn't actually that bad, the publication of child orders does work, but if the amount left is less than the initial range limit, it sends a command that it shouldn't send, but that has nothing to do with this PR |
yes please create a new issue for that, let's not include it here and keep this one small |
I think the rating data is only needed on pending orders |
yep! it should be like that now! |
I agree, in the future mostro can update that order event with the new rating data, but let's not get complicated right now with that |
|
tACK @arkanoider @grunch |
Hi @Catrya ,
i did the fix for rating of user not present in the nostr event when a new order is created. Please take a look and test when you're available.
Summary by CodeRabbit
Refactor
Chore