Code refactoring related with message payload#91
Conversation
* bumps mostro-cli to 0.10.4 * bumps mostro-core to last 0.6.14 * Refactoring to use a new payload structure instead of content * Update users table structure
WalkthroughThis pull request includes updates to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Outside diff range and nitpick comments (4)
src/cli/send_msg.rs (1)
41-43: Consider makingpayloadimmutableSince
payloadis only set once and not modified afterward, consider usingletinstead oflet mut:- let mut payload = None; - if let Some(t) = text { - payload = Some(Payload::TextMessage(t.to_string())); - } + let payload = text.map(|t| Payload::TextMessage(t.to_string()));Also applies to: 47-47
src/cli/take_sell.rs (1)
43-49: Consider simplifying the payload transformation logicThe nested match structure for handling amounts could be simplified for better readability:
- payload = match payload { - Some(Payload::PaymentRequest(a, b, _)) => { - Some(Payload::PaymentRequest(a, b, Some(amount.unwrap() as i64))) - } - None => Some(Payload::Amount(amount.unwrap().into())), - _ => None, - }; + payload = amount.map(|amt| match payload { + Some(Payload::PaymentRequest(a, b, _)) => + Payload::PaymentRequest(a, b, Some(amt as i64)), + _ => Payload::Amount(amt.into()), + });src/cli/get_dm.rs (1)
Line range hint
48-53: Enhance error handling informationThe error case could provide more context about why the message parsing failed:
- Err(_) => { + Err(e) => { println!("You got this message:"); println!(); println!("{}", el.0); + println!("Failed to parse: {}", e); println!(); }src/util.rs (1)
Line range hint
1-450: Consider documenting the payload structure changesThe refactoring from
ContenttoPayloadrepresents a significant architectural change. Consider:
- Updating documentation to reflect the new payload structure
- Adding comments explaining the payload format for future maintainers
- Creating migration guides if this is a breaking change for external consumers
This will help maintain the codebase's long-term maintainability and ease future onboarding.
📜 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 (13)
Cargo.toml(2 hunks)src/cli/add_invoice.rs(2 hunks)src/cli/get_dm.rs(2 hunks)src/cli/new_order.rs(4 hunks)src/cli/rate_user.rs(2 hunks)src/cli/send_msg.rs(2 hunks)src/cli/take_buy.rs(2 hunks)src/cli/take_dispute.rs(0 hunks)src/cli/take_sell.rs(2 hunks)src/db.rs(1 hunks)src/nip59.rs(5 hunks)src/pretty_table.rs(1 hunks)src/util.rs(4 hunks)
💤 Files with no reviewable changes (1)
- src/cli/take_dispute.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (16)
src/cli/send_msg.rs (1)
6-6: LGTM! Clean transition to the new Payload type
The import changes and payload initialization align well with the refactoring objectives.
Also applies to: 41-43
src/cli/take_sell.rs (1)
55-55: Verify the trade_index conversion
The .into() conversion of trade_index to Option<i64> should be verified to ensure it handles the full range of possible values correctly.
✅ Verification successful
Let me check the type of trade_index and its usage to ensure the conversion is safe.
From the results, I can see that trade_index is consistently defined as u32 across different files, and it's being converted to Option<i64> using .into(). Let me verify the payload structure to ensure this conversion is expected.
Let me try one final verification to check the database schema or type definitions that might explain this conversion.
The trade_index conversion is correctly implemented
The conversion from u32 to Option<i64> is safe and matches the database schema:
trade_indexis defined asu32in the CLI structs- The database schema uses
INTEGER(mapped toi64in Rust) - The conversion using
.into()is safe asu32values can always fit withini64
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of trade_index conversion
rg -A 2 "trade_index.*into\(\)"
Length of output: 467
Script:
#!/bin/bash
# Find the type definition and usage of trade_index
ast-grep --pattern 'trade_index'
# Also check the struct/type definitions
rg -B 2 -A 2 "struct.*Trade"
Length of output: 782
Script:
#!/bin/bash
# Check the payload structure and type definitions
rg -B 3 -A 3 "struct.*Payload"
# Also check where trade_index is used in database operations
ast-grep --pattern 'fn get_next_trade_index'
Length of output: 79
Script:
#!/bin/bash
# Look for database schema or type definitions
rg -B 2 -A 2 "type.*Trade"
# Check for SQL queries or database operations related to trade_index
rg -B 2 -A 2 "trade_index.*i64|trade_index.*INTEGER"
Length of output: 676
src/util.rs (3)
11-11: LGTM: Import change aligns with payload structure refactoring
The import change from Content to Payload is consistent with the PR's objective of refactoring the message payload structure.
27-35: LGTM: Consistent parameter renaming and encryption handling
The renaming from content to payload is applied consistently, and the encryption logic remains secure with the new payload structure.
108-109: Verify consistent usage of new Payload pattern
The change from Content::PaymentRequest to Payload::PaymentRequest looks good and simplifies the access pattern.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
All instances of PaymentRequest have been updated to use the new pattern
The verification shows that:
- No instances of the old
Content::PaymentRequestpattern remain - All 8 occurrences of
Payload::PaymentRequestfollow the new pattern consistently - No
.content.0access patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of Content::PaymentRequest
rg "Content::PaymentRequest"
# Search for all uses of the new Payload::PaymentRequest pattern
rg "Payload::PaymentRequest"
# Look for any remaining .content.0 access patterns that might need updating
rg "\.content\.0"
Length of output: 871
src/cli/new_order.rs (3)
71-71: Validate parameters passed to SmallOrder::new
Ensure that all parameters passed to SmallOrder::new are correct and in the expected order, especially after transitioning from Content to Payload. Incorrect parameter alignment might lead to unexpected behaviors or runtime errors.
117-117: Check the use of trade_index.into()
The conversion of trade_index using .into() suggests a type change. Verify that trade_index implements the necessary Into trait for the expected type, and that this conversion is appropriate in this context to prevent potential issues.
2-2: Ensure all references to Content are updated to Payload
The import of Payload signifies a refactoring from Content to Payload. Please verify that all instances of Content in this file have been updated to Payload to prevent any type mismatches or compilation errors.
Run the following script to check for any remaining references to Content:
✅ Verification successful
No remaining references to Content found in the file
The search results show that there are no occurrences of Content in src/cli/new_order.rs, indicating that all references have been properly updated to Payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find occurrences of `Content` in Rust files.
rg 'Content' --type rs src/cli/new_order.rs
Length of output: 70
Script:
#!/bin/bash
# Let's try without the file type flag since it's not recognized
rg 'Content' src/cli/new_order.rs
Length of output: 33
src/cli/take_buy.rs (3)
22-22: Ensure safe casting from u32 to i64
The amount amt is being cast from u32 to i64. Confirm that this casting won't lead to unintended consequences, such as negative values or overflow issues, especially if amt could exceed the range of i64.
27-27: Verify the conversion of trade_index using .into()
Ensure that trade_index correctly implements the Into trait for the expected type, and that using .into() here performs the intended conversion without data loss or errors.
2-2: Confirm replacement of Content with Payload
With Payload now imported, please ensure that all previous references to Content have been replaced with Payload throughout this file to maintain consistency and prevent compilation errors.
Use this script to find any lingering references:
✅ Verification successful
All Content references have been properly replaced with Payload
After analyzing the complete file content, there are no remaining references to Content. The file correctly uses Payload in both the import statement and its usage in the code where it's used to create a payload with Payload::Amount(amt as i64). The replacement has been done consistently throughout the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find occurrences of `Content` in Rust files.
rg 'Content' --type rs src/cli/take_buy.rs
Length of output: 69
Script:
#!/bin/bash
# Search for 'Content' without file type restriction since rg doesn't recognize rs
rg 'Content' src/cli/take_buy.rs
Length of output: 32
Script:
#!/bin/bash
# Also search for current usage of 'Payload' to verify the replacement
rg 'Payload' src/cli/take_buy.rs -A 2
Length of output: 284
Script:
#!/bin/bash
# Get the full content of the file to analyze
cat src/cli/take_buy.rs
Length of output: 1016
src/cli/rate_user.rs (2)
21-21: Confirm correct assignment to Payload::RatingUser
Assigning rating_content = Payload::RatingUser(*rating) appears appropriate. Ensure that *rating correctly provides the intended value and that Payload::RatingUser is the correct variant for this payload.
2-2: Ensure consistent use of Payload over Content
With Payload being imported, verify that all instances of Content in this file have been updated to Payload to prevent any inconsistencies or type errors.
Check for any remaining references using:
✅ Verification successful
No instances of Content found in the file
The search results confirm that there are no occurrences of Content in src/cli/rate_user.rs. Since Payload is properly imported and there are no remaining instances of Content, the code is consistent in its usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find occurrences of `Content` in Rust files.
rg 'Content' --type rs src/cli/rate_user.rs
Length of output: 70
Script:
#!/bin/bash
# Let's try without the file type flag since it's not recognized
rg 'Content' src/cli/rate_user.rs
Length of output: 33
src/cli/add_invoice.rs (3)
26-29: Validate creation of Payload::PaymentRequest
When assigning to payload, ensure that the parameters passed to Payload::PaymentRequest(None, invoice.to_string(), None) are correct and align with the expected structure of PaymentRequest. Verify that passing None for the first and third parameters is appropriate in this context.
34-37: Confirm correct usage of Message::new_order with payload
The construction of add_invoice_message using Message::new_order and the new payload appears correct. Ensure that all parameters are provided as expected and that the payload correctly encapsulates the required data for the AddInvoice action.
5-5: Verify all instances of Content are replaced with Payload
After importing Payload, please ensure that all references to Content have been updated in this file to maintain consistency and prevent compilation issues.
Use the following command to identify any occurrences:
✅ Verification successful
No instances of Content found, all usages are already using Payload
After reviewing the entire file content, I can confirm that there are no instances of Content in the file. The code is already consistently using Payload in all relevant places, including the import statement and the payload construction in two locations. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find occurrences of `Content` in Rust files.
rg 'Content' --type rs src/cli/add_invoice.rs
Length of output: 72
Script:
#!/bin/bash
# Search for 'Content' without file type restriction since rg doesn't recognize rs
rg 'Content' src/cli/add_invoice.rs
Length of output: 35
Script:
#!/bin/bash
# Also search for 'Payload' to verify the replacement
rg 'Payload' src/cli/add_invoice.rs -A 1
Length of output: 352
Script:
#!/bin/bash
# Let's see the entire file content to better understand the context
cat src/cli/add_invoice.rs
Length of output: 1406
| println!(); | ||
| println!("Action: {}", m.get_inner_message_kind().action); | ||
| println!("Content: {:#?}", m.get_inner_message_kind().content); | ||
| println!("Payload: {:#?}", m.get_inner_message_kind().payload); |
There was a problem hiding this comment.
Review sensitive data exposure in debug output
The debug printing of the entire payload might expose sensitive information. Consider implementing a filtered or redacted version for logging.
- println!("Payload: {:#?}", m.get_inner_message_kind().payload);
+ println!("Payload type: {}", m.get_inner_message_kind().payload.type_name());Committable suggestion skipped: line range outside the PR's diff.
| // We convert back the string to a message | ||
| let message = Message::from_json(&payload).unwrap(); |
There was a problem hiding this comment.
Replace unwrap() with proper error handling
The unwrap() call on the JSON parsing could panic if the payload string is not valid JSON. Consider using proper error handling to gracefully handle parsing failures.
- let message = Message::from_json(&payload).unwrap();
+ let message = Message::from_json(&payload)
+ .map_err(|e| BuilderError::Custom(format!("Failed to parse payload: {}", e)))?;📝 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.
| // We convert back the string to a message | |
| let message = Message::from_json(&payload).unwrap(); | |
| // We convert back the string to a message | |
| let message = Message::from_json(&payload) | |
| .map_err(|e| BuilderError::Custom(format!("Failed to parse payload: {}", e)))?; |
| ); | ||
| CREATE TABLE IF NOT EXISTS users ( | ||
| i0_pubkey TEXT PRIMARY KEY, | ||
| i0_pubkey char(64) PRIMARY KEY, |
There was a problem hiding this comment.
💡 Codebase verification
Breaking schema change requires migration strategy
The codebase shows a schema change in src/db.rs where i0_pubkey column type is changed to char(64), but there's no evidence of:
- Database migration code
- Schema version tracking
- Data validation or conversion logic
This change could break existing databases. Please:
- Add a migration strategy to handle the schema change
- Include data validation to ensure existing records meet the new 64-character requirement
- Consider adding schema versioning to track database changes
🔗 Analysis chain
Consider adding a database migration strategy
Changing the column type from TEXT to char(64) is a breaking schema change that could affect existing data. While the fixed-length constraint is good for data integrity, consider:
- Adding a migration strategy for existing databases
- Handling cases where existing data might not conform to the 64-character length
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing public keys that don't match the 64-character requirement
fd -e db . | xargs -I {} sqlite3 {} "SELECT i0_pubkey, length(i0_pubkey) FROM users WHERE length(i0_pubkey) != 64;"
Length of output: 121
Script:
#!/bin/bash
# Let's check for any database migration files or schema changes
fd -e sql -e rs . | xargs rg -l "migration|schema|ALTER TABLE|CREATE TABLE"
# Also check for any existing database schema definitions
fd -e rs . | xargs rg -A 5 "CREATE TABLE.*users"
Length of output: 404
Script:
#!/bin/bash
# Let's check the git history for the previous schema definition
git log -p src/db.rs | rg -B 2 -A 2 "i0_pubkey.*PRIMARY KEY"
# Also check for any existing database initialization or upgrade code
fd -e rs . | xargs rg -l "initialize.*db|upgrade.*db|migrate.*db"
Length of output: 67
| pub fn print_order_preview(ord: Payload) -> Result<String, String> { | ||
| let single_order = match ord { | ||
| Content::Order(o) => o, | ||
| Payload::Order(o) => o, | ||
| _ => return Err("Error".to_string()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with descriptive messages
The current error handling returns a generic "Error" string, which doesn't provide useful information about what went wrong. Consider providing more descriptive error messages.
- _ => return Err("Error".to_string()),
+ _ => return Err("Expected Payload::Order variant but received a different payload type".to_string()),📝 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 print_order_preview(ord: Payload) -> Result<String, String> { | |
| let single_order = match ord { | |
| Content::Order(o) => o, | |
| Payload::Order(o) => o, | |
| _ => return Err("Error".to_string()), | |
| pub fn print_order_preview(ord: Payload) -> Result<String, String> { | |
| let single_order = match ord { | |
| Payload::Order(o) => o, | |
| _ => return Err("Expected Payload::Order variant but received a different payload type".to_string()), |
1 participant
Summary by CodeRabbit
New Features
Payloadtype.Bug Fixes
userstable to enforce fixed-length public key format.Documentation
ContenttoPayload.Refactor
sign_contentfunction to simplify message processing.Chores