refactor: split util.rs into modular structure #562
Conversation
- Created util/ directory with 6 specialized modules - Moved 30 functions to appropriate modules by domain - Moved 4 lightning functions to lightning/operations.rs - Maintained backward compatibility via re-exports - All tests passing (107/107)
|
Warning Rate limit exceeded@grunch has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 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. 📒 Files selected for processing (3)
WalkthroughAdds a new lightning operations submodule and re-exports its APIs; introduces multiple new util modules (helpers, nostr, orders, pricing, queues, reputation) with functions for order lifecycle, Nostr integration, pricing/fees, queuing, and reputation; adds bytes_to_string helper and a dev test dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as Mostro App
participant DB as Database
participant LND as LND
participant Nostr as Nostr
participant MQ as MessageQueue
Client->>App: request show_hold_invoice(params)
App->>LND: create hold invoice (get payment_request & hash)
App->>DB: persist order (preimage, hash, status, pubkeys)
App->>Nostr: publish order update event
App->>MQ: enqueue buyer & seller notifications
App->>App: spawn invoice_subscribe(hash)
Note over App,LND: background subscription monitors invoice states
LND-->>App: invoice state change (ACCEPTED / SETTLED / CANCELED)
alt ACCEPTED
App->>DB: update temporary status
App->>MQ: enqueue accepted notification
else SETTLED
Client->>App: call settle_seller_hold_invoice(preimage)
App->>DB: validate sender (seller or admin)
App->>LND: settle invoice with preimage
App->>DB: update final order status
App->>Nostr: publish settlement event
else CANCELED
App->>DB: mark canceled
App->>MQ: enqueue cancellation notifications
App->>Nostr: publish cancellation event
end
App-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (13)
src/lightning/operations.rs (4)
55-58: Consider usingget_db_pool()for consistency.This function creates a new DB connection via
db::connect()whileinvoice_subscribe(line 118) uses the shared pool viaget_db_pool(). Using the shared pool here would be more consistent and efficient.🔎 Proposed fix
// We need to publish a new event with the new status - let pool = db::connect() - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + let pool = get_db_pool();
97-97: Silently ignoringinvoice_subscriberesult may hide subscription failures.If the invoice subscription fails, the order will remain in
WaitingPaymentstatus without any payment state listener, potentially leaving the order in a stuck state. Consider logging a warning or propagating the error.🔎 Proposed fix
- let _ = invoice_subscribe(hash, request_id).await; + if let Err(e) = invoice_subscribe(hash, request_id).await { + tracing::warn!("Failed to subscribe to invoice for order {}: {}", order.id, e); + }
107-115: Subscription errors are silently discarded.The
subscribe_invoiceerror is mapped to a string and then discarded withlet _ =. If the subscription fails immediately, there's no indication of the failure. Consider logging subscription errors.🔎 Proposed fix
let invoice_task = { + let hash_hex = bytes_to_string(&hash); async move { - let _ = ln_client_invoices + if let Err(e) = ln_client_invoices .subscribe_invoice(hash, tx) .await - .map_err(|e| e.to_string()); + { + tracing::warn!("Invoice subscription failed for hash {}: {}", hash_hex, e); + } } };
165-165: Unnecessary#[allow(clippy::too_many_arguments)]attribute.The function only has 5 parameters, which is below Clippy's default threshold of 7. This attribute can be removed.
🔎 Proposed fix
/// Settle a seller hold invoice -#[allow(clippy::too_many_arguments)] pub async fn settle_seller_hold_invoice(src/util/mod.rs (2)
1-1: Use English for code comments.The comment "Submódulos" is in Spanish. For consistency and broader accessibility, consider using English: "Submodules".
9-9: Use English for code comments.The comment is in Spanish. Consider: "Re-exports for backward compatibility with existing imports".
src/util/nostr.rs (1)
46-51: Logging DM payload may expose sensitive data.The payload is logged at
infolevel, which may contain sensitive user data. Based on learnings, this is planned to be removed in a future version. Consider usingdebuglevel or removing the payload from the log.🔎 Proposed fix
info!( - "Sending DM, Event ID: {} to {} with payload: {:#?}", + "Sending DM, Event ID: {} to {}", event.id, receiver_pubkey.to_hex(), - payload );src/util/pricing.rs (4)
1-12: LGTM on imports and type definitions.The imports are well-organized and the
FiatNamestype alias provides clarity. Consider makingMAX_RETRYpublic if it needs to be accessible from other modules (per AI summary it should bepub const).If MAX_RETRY should be public per the API surface
-const MAX_RETRY: u16 = 4; +pub const MAX_RETRY: u16 = 4;
14-43: Misleading function name:retries_yadio_requestdoes not retry.The retry logic is implemented in the caller (
get_market_quote), not in this function. Consider renaming toyadio_requestorcheck_and_fetch_yadioto accurately reflect its behavior.
96-110: Redundant None check.The check at lines 96-100 is redundant since lines 102-110 handle the
Some/Nonecases again. Simplify by usingmatchdirectly.Proposed simplification
- if req.0.is_none() { - return Err(MostroError::MostroInternalErr( - ServiceError::MalformedAPIRes, - )); - } - - let quote = if let Some(q) = req.0 { - q.json::<Yadio>() + let quote = match req.0 { + Some(q) => q.json::<Yadio>() .await - .map_err(|_| MostroError::MostroInternalErr(ServiceError::MessageSerializationError))? - } else { - return Err(MostroError::MostroInternalErr( + .map_err(|_| MostroError::MostroInternalErr(ServiceError::MessageSerializationError))?, + None => return Err(MostroError::MostroInternalErr( ServiceError::MalformedAPIRes, - )); + )), };
177-193: Consider cachingTimestamp::now()to avoid potential inconsistency.
Timestamp::now()is called twice (lines 181 and 189), which could yield slightly different values. Caching it once at the start improves consistency and slightly reduces overhead.Proposed simplification
pub fn get_expiration_date(expire: Option<i64>) -> i64 { let mostro_settings = Settings::get_mostro(); - // We calculate order expiration - let expire_date: i64; - let expires_at_max: i64 = Timestamp::now().as_u64() as i64 + let now = Timestamp::now().as_u64() as i64; + let expires_at_max = now + Duration::days(mostro_settings.max_expiration_days.into()).num_seconds(); - if let Some(mut exp) = expire { - if exp > expires_at_max { - exp = expires_at_max; - }; - expire_date = exp; - } else { - expire_date = Timestamp::now().as_u64() as i64 - + Duration::hours(mostro_settings.expiration_hours as i64).num_seconds(); - } - expire_date + + match expire { + Some(exp) => exp.min(expires_at_max), + None => now + Duration::hours(mostro_settings.expiration_hours as i64).num_seconds(), + } }src/util/orders.rs (2)
26-42: Confusing use ofis_sell_order()Result pattern.Using
Ok(_)to mean "is sell" andErr(_)to mean "is buy" is counterintuitive. TheErrcase implies an error condition, not a valid "buy" order. Consider using a more explicit check or adding a clarifying comment.- let identity_pubkey = match order_updated.is_sell_order() { - Ok(_) => order_updated + // is_sell_order() returns Ok(()) for sell orders, Err for buy orders + let is_sell = order_updated.is_sell_order().is_ok(); + let identity_pubkey = if is_sell { + order_updated .get_master_seller_pubkey(MOSTRO_DB_PASSWORD.get()) - .map_err(MostroInternalErr)?, - Err(_) => order_updated + .map_err(MostroInternalErr)? + } else { + order_updated .get_master_buyer_pubkey(MOSTRO_DB_PASSWORD.get()) - .map_err(MostroInternalErr)?, + .map_err(MostroInternalErr)? };
434-448: Consider simplifying the if-else control flow.The nested if-else can be flattened using early return.
Proposed simplification
- // if invoice is valid - if is_valid_invoice( + is_valid_invoice( pr.clone(), Some(order.amount as u64), Some(total_buyer_fees as u64), ) .await - .is_err() - { - return Err(MostroCantDo(CantDoReason::InvalidInvoice)); - } - // if invoice is valid return it - else { - payment_request = Some(pr); - } + .map_err(|_| MostroCantDo(CantDoReason::InvalidInvoice))?; + + payment_request = Some(pr); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/lightning/mod.rssrc/lightning/operations.rssrc/util.rs.backupsrc/util/helpers.rssrc/util/mod.rssrc/util/nostr.rssrc/util/orders.rssrc/util/pricing.rssrc/util/queues.rssrc/util/reputation.rs
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/util/helpers.rssrc/util/queues.rssrc/lightning/mod.rssrc/util/pricing.rssrc/lightning/operations.rssrc/util/mod.rssrc/util/reputation.rssrc/util/nostr.rssrc/util/orders.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/util/helpers.rssrc/util/queues.rssrc/lightning/mod.rssrc/util/pricing.rssrc/lightning/operations.rssrc/util/mod.rssrc/util/reputation.rssrc/util/nostr.rssrc/util/orders.rs
🧠 Learnings (10)
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util/queues.rssrc/util/pricing.rssrc/util/nostr.rssrc/util/orders.rs
📚 Learning: 2025-02-07T21:35:08.100Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 431
File: src/scheduler.rs:33-65
Timestamp: 2025-02-07T21:35:08.100Z
Learning: The message queue implementation in mostro includes a custom retry mechanism for handling failed messages, with metrics planned for a future implementation.
Applied to files:
src/util/queues.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util/pricing.rssrc/util/orders.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/util/pricing.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/lightning/operations.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/lightning/operations.rs
📚 Learning: 2025-12-23T17:33:03.588Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T17:33:03.588Z
Learning: Applies to src/**/*.rs : Runtime code lives in `src/` directory with subdirectories: `src/app/` for order flows and business logic, `src/lightning/` for LND bindings and Lightning helpers, `src/rpc/` for gRPC service and types, and `src/config/` for settings and loaders
Applied to files:
src/util/mod.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util/reputation.rs
📚 Learning: 2025-09-04T18:50:47.723Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/util.rs:462-466
Timestamp: 2025-09-04T18:50:47.723Z
Learning: The logging of DM payload at info level in src/util.rs send_dm function is planned to be removed in a future version by the maintainers, as it can contain sensitive data.
Applied to files:
src/util/nostr.rs
📚 Learning: 2025-11-06T23:01:06.496Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app/last_trade_index.rs:67-70
Timestamp: 2025-11-06T23:01:06.496Z
Learning: In the Mostro codebase, users are only created through the check_trade_index() function when they create or take their first order. Since check_trade_index() validates and rejects trade_index = 0 (using pattern matching at line 134 in src/app.rs with `index @ 1..`), no user can ever be created with last_trade_index = 0, even though the database migration shows DEFAULT 0. Therefore, the guard in last_trade_index.rs that returns InvalidTradeIndex when last_trade_index == 0 is safe and will only catch invalid database states.
Applied to files:
src/util/orders.rs
🧬 Code graph analysis (6)
src/lightning/mod.rs (1)
src/lightning/operations.rs (4)
get_market_amount_and_fee(152-162)invoice_subscribe(103-150)settle_seller_hold_invoice(166-193)show_hold_invoice(18-100)
src/util/pricing.rs (3)
src/config/settings.rs (1)
get_mostro(45-50)src/db.rs (1)
None(650-650)src/bitcoin_price.rs (1)
get_price(46-54)
src/lightning/operations.rs (8)
src/config/settings.rs (1)
get_db_pool(31-33)src/util/helpers.rs (1)
bytes_to_string(12-17)src/util/orders.rs (1)
update_order_event(248-285)src/util/pricing.rs (2)
get_fee(122-127)get_market_quote(50-120)src/util/queues.rs (1)
enqueue_order_msg(31-46)src/lightning/mod.rs (1)
new(44-58)src/messages.rs (1)
hold_invoice_description(3-11)src/flow.rs (3)
hold_invoice_paid(8-128)hold_invoice_settlement(130-137)hold_invoice_canceled(139-146)
src/util/mod.rs (6)
src/util/helpers.rs (1)
bytes_to_string(12-17)src/util/pricing.rs (7)
calculate_dev_fee(140-143)get_bitcoin_price(45-47)get_dev_fee(149-152)get_expiration_date(177-193)get_fee(122-127)get_market_quote(50-120)retries_yadio_request(14-43)src/util/nostr.rs (7)
connect_nostr(169-194)get_keys(127-137)get_nostr_client(197-205)get_nostr_relays(208-214)publish_dev_fee_audit_event(64-125)send_dm(16-61)update_user_rating_event(140-167)src/util/queues.rs (3)
enqueue_cant_do_msg(6-19)enqueue_order_msg(31-46)enqueue_restore_session_msg(21-29)src/util/reputation.rs (3)
get_dispute(123-136)notify_taker_reputation(41-121)rate_counterpart(10-39)src/lightning/operations.rs (4)
get_market_amount_and_fee(152-162)invoice_subscribe(103-150)settle_seller_hold_invoice(166-193)show_hold_invoice(18-100)
src/util/reputation.rs (2)
src/db.rs (1)
is_user_present(958-973)src/util/queues.rs (1)
enqueue_order_msg(31-46)
src/util/orders.rs (5)
src/config/settings.rs (1)
get_db_pool(31-33)src/db.rs (2)
is_user_present(958-973)None(650-650)src/nip33.rs (2)
new_event(24-38)order_to_tags(187-282)src/util/queues.rs (1)
enqueue_order_msg(31-46)src/util/nostr.rs (1)
get_nostr_client(197-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (27)
src/util/queues.rs (1)
1-46: LGTM!The queue utility functions are well-structured with consistent patterns across all three helpers. The fire-and-forget approach aligns with the existing message queue retry mechanism mentioned in the learnings.
src/lightning/mod.rs (1)
2-7: LGTM!The module declaration and re-exports cleanly expose the new Lightning operations while maintaining a logical public API surface. This approach preserves backward compatibility per the PR objectives.
src/util/helpers.rs (2)
19-36: LGTM!Tests are well-structured, co-located in the
mod testsblock, and descriptively named per the coding guidelines.
3-11: Doctest example needs a module path orusestatement.The doctest example references
bytes_to_stringwithout the full path or import, which will cause the doctest to fail when runningcargo test --doc.🔎 Proposed fix
/// # Examples /// /// ``` -/// let bytes = vec![0xde, 0xad, 0xbe, 0xef]; -/// let result = bytes_to_string(&bytes); +/// use mostro::util::bytes_to_string; +/// +/// let bytes = [0xde, 0xad, 0xbe, 0xef]; +/// let result = bytes_to_string(&bytes); /// assert_eq!(result, "deadbeef"); /// ```⛔ Skipped due to learnings
Learnt from: arkanoider Repo: MostroP2P/mostro PR: 0 File: :0-0 Timestamp: 2025-02-28T15:32:19.572Z Learning: Rust doctests (code examples in documentation that are automatically executed as tests) are only available for library code, not for binary applications like Mostro. While docstrings are still valuable for documentation in binary applications, the examples within them won't be automatically tested.src/lightning/operations.rs (1)
152-162: LGTM!Clean composition of the pricing utilities with appropriate error propagation.
src/util/mod.rs (1)
37-40: Verify this cross-module re-export is intentional.Re-exporting
crate::lightning::operationsitems fromutilcreates a cross-module dependency whereutilexposeslightningfunctionality. While this maintains backward compatibility, it may cause confusion about the canonical import path. Consider whether consumers should import directly fromcrate::lightninginstead.src/util/reputation.rs (3)
9-39: LGTM!The function correctly enqueues rating requests to both counterparts with appropriate documentation.
123-136: LGTM!The function correctly retrieves disputes with appropriate error handling for missing IDs and DB access failures.
46-46: No issue found. The code at line 46 is correct.is_buy_order()returns aResultwhereOk(_)indicates the order is a buy order andErr(_)indicates it is not. Using.is_ok()properly converts this to a boolean value, which is the idiomatic Rust pattern for this validation. This approach is used consistently throughout the codebase (e.g.,nip33.rs:97,release.rs:388).Likely an incorrect or invalid review comment.
src/util/nostr.rs (5)
63-125: LGTM!The dev fee audit event is well-structured with appropriate tags for queryability. The function follows the coding guidelines with a triple-slash documentation comment.
127-137: LGTM!Proper error handling with tracing for logging parse failures.
139-167: LGTM!The function correctly updates user rating events and order vote status. The
#[allow(clippy::too_many_arguments)]is appropriate for 7 parameters.
169-194: LGTM!The Nostr client initialization with custom relay limits is correctly implemented with clear documentation of the size adjustments.
196-214: LGTM!Both getter functions are correctly implemented. The different return types (
ResultvsOption) likely reflect different usage patterns where relay access can gracefully degrade.src/util/pricing.rs (5)
45-47: LGTM!Clean wrapper function that delegates to
BitcoinPriceManager::get_price.
122-127: LGTM!The split fee calculation is clear and the rounding behavior is appropriate.
129-143: LGTM!Well-documented pure function that's easy to test with different percentage values.
145-152: LGTM!Clean wrapper that retrieves the percentage from settings and delegates to the pure
calculate_dev_feefunction.
195-241: LGTM on tests!Good edge case coverage for
calculate_dev_fee(basic, rounding, zero, tiny amounts). The URL construction test validates the expected format.src/util/orders.rs (8)
1-18: LGTM!Well-organized imports and the
OrderKindtype alias improves readability throughout the file.
63-87: LGTM!Clean implementation that handles both existing users and new users (when identity matches trade pubkey).
248-285: LGTM!Good use of
get_nostr_client()for proper error handling. The warning log for expired orders is appropriate since the order update can still proceed.
287-300: LGTM!Clean order retrieval with proper error handling for missing order ID and database errors.
302-346: LGTM!Excellent use of
QueryBuilderwithpush_bindfor safe parameterized queries. TheORDER BY CASEpattern cleverly preserves the caller's requested order sequence.
348-403: LGTM!Good use of
saturating_subto prevent underflow when calculating buyer's final amount. The function properly notifies both buyer and seller about the order status.
405-422: LGTM!Clear implementation with proper boundary validation for range orders.
452-603: LGTM on tests!Excellent test setup with in-memory SQLite and good coverage of
get_user_orders_by_idincluding filtering by user and order preservation. The helper functions keep tests clean and readable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/util/nostr.rs:
- Line 242: Clippy flags a redundant closure in the initialization of the
NOSTR_CLIENT static: replace the closure `|| Client::default()` with the
function pointer `Client::default` when calling `NOSTR_CLIENT.get_or_init`;
update the call site that references `NOSTR_CLIENT.get_or_init(||
Client::default())` to use `NOSTR_CLIENT.get_or_init(Client::default)` so the
default constructor is passed directly.
🧹 Nitpick comments (3)
src/util/nostr.rs (3)
46-51: Reminder: DM payload logging contains sensitive data.The payload logged here can contain sensitive information. Based on learnings, this is planned to be removed in a future version.
175-176: Consider making relay limits configurable.The message and event size limits are currently hardcoded. Based on learnings, these should be made configurable via settings.toml in a future update.
222-236: Test has conditional logic that reduces its effectiveness.The test conditionally checks the error case only if the client is uninitialized. Once
NOSTR_CLIENTis initialized by any test (even with#[serial_test::serial]), subsequent runs skip the actual assertion, making this test less reliable.Consider restructuring to avoid relying on global mutable state, or ensure the test always validates expected behavior regardless of initialization state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomlsrc/util/nostr.rs
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/util/nostr.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/util/nostr.rs
🧠 Learnings (4)
📚 Learning: 2024-12-20T12:17:56.987Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 398
File: src/util.rs:386-387
Timestamp: 2024-12-20T12:17:56.987Z
Learning: In future updates, the message and event size limits for Nostr relays will be made configurable via settings.toml rather than hardcoded.
Applied to files:
src/util/nostr.rs
📚 Learning: 2025-09-04T18:50:47.723Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/util.rs:462-466
Timestamp: 2025-09-04T18:50:47.723Z
Learning: The logging of DM payload at info level in src/util.rs send_dm function is planned to be removed in a future version by the maintainers, as it can contain sensitive data.
Applied to files:
src/util/nostr.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util/nostr.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/util/nostr.rs
🧬 Code graph analysis (1)
src/util/nostr.rs (3)
src/nip33.rs (1)
new_event(24-38)src/util/orders.rs (1)
get_order(287-300)src/config/settings.rs (1)
get_nostr(61-63)
🪛 GitHub Actions: CI
src/util/nostr.rs
[error] 242-242: Clippy: redundant closure. Replace '|| Client::default()' with 'Client::default'.
🔇 Additional comments (6)
Cargo.toml (1)
93-93: The serial_test dependency is correctly added and appropriately used.The version "3.2" is valid and resolves to 3.3.1 in the lock file. The crate is being actively used in test code:
src/util/nostr.rshas test functions decorated with#[serial_test::serial]that serialize test execution for tests sharing mutable global state (NOSTR_CLIENT). No security advisories identified.src/util/nostr.rs (5)
63-125: LGTM!The dev fee audit event creation and publishing logic is well-structured with appropriate error handling and logging.
127-137: LGTM!Key parsing with proper error handling and logging.
139-167: LGTM!Rating event update logic correctly fetches the order, creates a replaceable event, updates rating flags, and queues the event.
196-205: LGTM!Clean getter implementation with appropriate error handling for uninitialized state.
207-214: LGTM!Straightforward relay getter with appropriate Option handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @src/util/orders.rs:
- Line 8: The import NOSTR_CLIENT in orders.rs is unused after refactoring to
use the get_nostr_client() helper; remove the `use crate::NOSTR_CLIENT;`
statement from the top of src/util/orders.rs so the file only references
get_nostr_client() where needed (e.g., the call sites around the existing
get_nostr_client() usages) to eliminate the unused import and fix the CI
failure.
- Around line 300-344: The query in get_user_orders_by_id incorrectly compares
encrypted master_buyer_pubkey/master_seller_pubkey columns to plaintext
user_pubkey; instead, remove the "(master_buyer_pubkey = ... OR
master_seller_pubkey = ...)" predicates so you only fetch orders by id, then
after fetching, decrypt each order's master_buyer_pubkey and
master_seller_pubkey using CryptoUtils::decrypt_stored with
crate::config::MOSTRO_DB_PASSWORD and compare the decrypted strings to
user_pubkey to filter allowed orders, preserving the original input order (use
the existing ORDER BY CASE id logic or re-order the filtered Vec according to
the input ids) and return the filtered results; reference get_user_orders_by_id,
CryptoUtils::decrypt_stored, MOSTRO_DB_PASSWORD, and the Order fields
master_buyer_pubkey/master_seller_pubkey when making changes.
- Around line 450-601: Tests insert plaintext
master_buyer_pubkey/master_seller_pubkey which masks the production behavior
where prepare_new_order encrypts those fields via CryptoUtils::store_encrypted,
so update the tests to insert encrypted values (call
CryptoUtils::store_encrypted or the test-side helper that mirrors it) into
insert_order before binding master_buyer_pubkey/master_seller_pubkey, or
alternatively mock CryptoUtils::store_encrypted in the test to return the
plaintext sentinel and use that sentinel in insert_order; ensure references to
prepare_new_order, CryptoUtils::store_encrypted, get_user_orders_by_id, and the
test helper insert_order are used so the test exercises the same
encryption/decryption logic as production (or add an integration test that
writes real encrypted rows).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util/orders.rs
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory with subdirectories:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/util/orders.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent,snake_casefor functions,PascalCasefor types, andSCREAMING_SNAKE_CASEfor constants in Rust code (Rust 2021 edition)
Document non-obvious public APIs with triple-slash///documentation comments
Prefertracingspans over ad-hoc logging in Rust code
Co-locate tests in their modules undermod testsblocks and name tests descriptively
Files:
src/util/orders.rs
🧠 Learnings (7)
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-10-14T13:26:33.845Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 534
File: src/util.rs:1023-1066
Timestamp: 2025-10-14T13:26:33.845Z
Learning: In the Mostro database, master keys (master_buyer_pubkey and master_seller_pubkey) are encrypted using CryptoUtils::store_encrypted with unique salt and nonce values for each encryption operation. This means the same plaintext key will produce different encrypted values each time it's stored. Therefore, comparing encrypted keys directly will not work; the correct approach is to decrypt the database values first and then compare them against the plaintext key.
Applied to files:
src/util/orders.rs
📚 Learning: 2025-11-06T23:01:06.496Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app/last_trade_index.rs:67-70
Timestamp: 2025-11-06T23:01:06.496Z
Learning: In the Mostro codebase, users are only created through the check_trade_index() function when they create or take their first order. Since check_trade_index() validates and rejects trade_index = 0 (using pattern matching at line 134 in src/app.rs with `index @ 1..`), no user can ever be created with last_trade_index = 0, even though the database migration shows DEFAULT 0. Therefore, the guard in last_trade_index.rs that returns InvalidTradeIndex when last_trade_index == 0 is safe and will only catch invalid database states.
Applied to files:
src/util/orders.rs
🧬 Code graph analysis (1)
src/util/orders.rs (7)
src/config/settings.rs (1)
get_db_pool(31-33)src/db.rs (2)
is_user_present(958-973)None(650-650)src/lightning/invoice.rs (2)
is_valid_invoice(181-195)payment_request(131-132)src/nip33.rs (2)
new_event(24-38)order_to_tags(187-282)src/util/pricing.rs (2)
get_expiration_date(177-193)get_fee(122-127)src/util/queues.rs (1)
enqueue_order_msg(31-46)src/util/nostr.rs (1)
get_nostr_client(197-205)
🪛 GitHub Actions: CI
src/util/orders.rs
[error] 8-8: unused import: crate::NOSTR_CLIENT. To fix, remove the unused import or use it if intended.
🔇 Additional comments (2)
src/util/orders.rs (2)
160-165: LGTM! Nostr client access properly fixed.The code now correctly uses
get_nostr_client()with proper error propagation instead of the unsafeunwrap()pattern.
213-233: LGTM! Encryption error handling is now consistent.Both Buy and Sell order paths correctly use
ServiceError::EncryptionErrorfor encryption failures.
| /// Efficiently retrieves multiple orders by their IDs for a specific user | ||
| pub async fn get_user_orders_by_id( | ||
| pool: &Pool<Sqlite>, | ||
| orders: &[Uuid], | ||
| user_pubkey: &str, | ||
| ) -> Result<Vec<Order>, MostroError> { | ||
| // Return empty vector if no orders requested | ||
| if orders.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| let mut query_builder = QueryBuilder::new("SELECT * FROM orders WHERE id IN ("); | ||
|
|
||
| { | ||
| let mut separated = query_builder.separated(", "); | ||
| for order_id in orders { | ||
| separated.push_bind(order_id); | ||
| } | ||
| } | ||
|
|
||
| query_builder.push(") AND ("); | ||
| query_builder.push("master_buyer_pubkey = "); | ||
| query_builder.push_bind(user_pubkey); | ||
| query_builder.push(" OR master_seller_pubkey = "); | ||
| query_builder.push_bind(user_pubkey); | ||
| query_builder.push(")"); | ||
|
|
||
| // Preserve the caller requested order sequence so that response payload matches | ||
| query_builder.push(" ORDER BY CASE id"); | ||
| for (index, order_id) in orders.iter().enumerate() { | ||
| query_builder.push(" WHEN "); | ||
| query_builder.push_bind(order_id); | ||
| query_builder.push(" THEN "); | ||
| query_builder.push_bind(index as i64); | ||
| } | ||
| query_builder.push(" END"); | ||
|
|
||
| let found_orders = query_builder | ||
| .build_query_as::<Order>() | ||
| .fetch_all(pool) | ||
| .await | ||
| .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; | ||
|
|
||
| Ok(found_orders) | ||
| } |
There was a problem hiding this comment.
Critical bug: Cannot compare encrypted master keys directly with plaintext.
Lines 321-325 compare master_buyer_pubkey and master_seller_pubkey database columns directly with the plaintext user_pubkey parameter. However, these database columns store encrypted values created with CryptoUtils::store_encrypted, which uses unique salt and nonce for each encryption. This means the comparison will never match in production.
Based on learnings from PR #534, the correct approach is to decrypt the database values first, then compare them against the plaintext key.
🔎 Recommended fix approach
Refactor to fetch orders first, then filter by decrypting and comparing master keys:
pub async fn get_user_orders_by_id(
pool: &Pool<Sqlite>,
orders: &[Uuid],
user_pubkey: &str,
) -> Result<Vec<Order>, MostroError> {
if orders.is_empty() {
return Ok(Vec::new());
}
let mut query_builder = QueryBuilder::new("SELECT * FROM orders WHERE id IN (");
{
let mut separated = query_builder.separated(", ");
for order_id in orders {
separated.push_bind(order_id);
}
}
- query_builder.push(") AND (");
- query_builder.push("master_buyer_pubkey = ");
- query_builder.push_bind(user_pubkey);
- query_builder.push(" OR master_seller_pubkey = ");
- query_builder.push_bind(user_pubkey);
- query_builder.push(")");
+ query_builder.push(")");
// Preserve the caller requested order sequence so that response payload matches
query_builder.push(" ORDER BY CASE id");
for (index, order_id) in orders.iter().enumerate() {
query_builder.push(" WHEN ");
query_builder.push_bind(order_id);
query_builder.push(" THEN ");
query_builder.push_bind(index as i64);
}
query_builder.push(" END");
- let found_orders = query_builder
+ let all_orders = query_builder
.build_query_as::<Order>()
.fetch_all(pool)
.await
.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
+ // Filter by decrypting and comparing master keys
+ let mut found_orders = Vec::new();
+ for order in all_orders {
+ let is_buyer = if let Some(ref encrypted_buyer) = order.master_buyer_pubkey {
+ match order.get_master_buyer_pubkey(MOSTRO_DB_PASSWORD.get()) {
+ Ok(decrypted) => decrypted == user_pubkey,
+ Err(_) => false,
+ }
+ } else {
+ false
+ };
+
+ let is_seller = if let Some(ref encrypted_seller) = order.master_seller_pubkey {
+ match order.get_master_seller_pubkey(MOSTRO_DB_PASSWORD.get()) {
+ Ok(decrypted) => decrypted == user_pubkey,
+ Err(_) => false,
+ }
+ } else {
+ false
+ };
+
+ if is_buyer || is_seller {
+ found_orders.push(order);
+ }
+ }
+
Ok(found_orders)
}Note: Ensure MOSTRO_DB_PASSWORD is imported from crate::config::MOSTRO_DB_PASSWORD if not already available in scope.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/util/orders.rs around lines 300 - 344, The query in
get_user_orders_by_id incorrectly compares encrypted
master_buyer_pubkey/master_seller_pubkey columns to plaintext user_pubkey;
instead, remove the "(master_buyer_pubkey = ... OR master_seller_pubkey = ...)"
predicates so you only fetch orders by id, then after fetching, decrypt each
order's master_buyer_pubkey and master_seller_pubkey using
CryptoUtils::decrypt_stored with crate::config::MOSTRO_DB_PASSWORD and compare
the decrypted strings to user_pubkey to filter allowed orders, preserving the
original input order (use the existing ORDER BY CASE id logic or re-order the
filtered Vec according to the input ids) and return the filtered results;
reference get_user_orders_by_id, CryptoUtils::decrypt_stored,
MOSTRO_DB_PASSWORD, and the Order fields
master_buyer_pubkey/master_seller_pubkey when making changes.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use mostro_core::message::{Message, MessageKind}; | ||
| use mostro_core::order::Order as SmallOrderTest; | ||
| use sqlx::sqlite::SqlitePoolOptions; | ||
| use sqlx::SqlitePool; | ||
| use uuid::{uuid, Uuid}; | ||
|
|
||
| async fn setup_orders_pool() -> SqlitePool { | ||
| let pool = SqlitePoolOptions::new() | ||
| .max_connections(1) | ||
| .connect(":memory:") | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| sqlx::query(include_str!("../../migrations/20221222153301_orders.sql")) | ||
| .execute(&pool) | ||
| .await | ||
| .unwrap(); | ||
| sqlx::query(include_str!("../../migrations/20251126120000_dev_fee.sql")) | ||
| .execute(&pool) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| pool | ||
| } | ||
|
|
||
| async fn insert_order( | ||
| pool: &SqlitePool, | ||
| id: Uuid, | ||
| identity_buyer_pubkey: Option<&str>, | ||
| identity_seller_pubkey: Option<&str>, | ||
| creator_pubkey: &str, | ||
| ) { | ||
| sqlx::query( | ||
| r#" | ||
| INSERT INTO orders ( | ||
| id, | ||
| kind, | ||
| event_id, | ||
| creator_pubkey, | ||
| status, | ||
| premium, | ||
| payment_method, | ||
| amount, | ||
| fiat_code, | ||
| fiat_amount, | ||
| created_at, | ||
| expires_at, | ||
| master_buyer_pubkey, | ||
| master_seller_pubkey | ||
| ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| "#, | ||
| ) | ||
| .bind(id) | ||
| .bind("buy") | ||
| .bind(id.simple().to_string()) | ||
| .bind(creator_pubkey) | ||
| .bind("active") | ||
| .bind(0_i64) | ||
| .bind("ln") | ||
| .bind(1_000_i64) | ||
| .bind("USD") | ||
| .bind(1_000_i64) | ||
| .bind(1_000_i64) | ||
| .bind(2_000_i64) | ||
| .bind(identity_buyer_pubkey) | ||
| .bind(identity_seller_pubkey) | ||
| .execute(pool) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_get_fiat_amount_requested() { | ||
| let uuid = uuid!("308e1272-d5f4-47e6-bd97-3504baea9c23"); | ||
| let order = SmallOrderTest { | ||
| amount: 1000, | ||
| min_amount: Some(500), | ||
| max_amount: Some(2000), | ||
| ..Default::default() | ||
| }; | ||
| let message = Message::Order(MessageKind::new( | ||
| Some(uuid), | ||
| Some(1), | ||
| Some(1), | ||
| Action::TakeSell, | ||
| Some(Payload::Amount(order.amount)), | ||
| )); | ||
| let amount = get_fiat_amount_requested(&order, &message); | ||
| assert_eq!(amount, Some(1000)); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_get_user_orders_by_id_filters_and_preserves_order() { | ||
| let pool = setup_orders_pool().await; | ||
| let user_pubkey = "a".repeat(64); | ||
| let other_pubkey = "b".repeat(64); | ||
|
|
||
| let first_id = Uuid::new_v4(); | ||
| let second_id = Uuid::new_v4(); | ||
| let third_id = Uuid::new_v4(); | ||
|
|
||
| insert_order( | ||
| &pool, | ||
| first_id, | ||
| Some(&user_pubkey), | ||
| Some(&other_pubkey), | ||
| &user_pubkey, | ||
| ) | ||
| .await; | ||
| insert_order( | ||
| &pool, | ||
| second_id, | ||
| Some(&other_pubkey), | ||
| Some(&user_pubkey), | ||
| &user_pubkey, | ||
| ) | ||
| .await; | ||
| insert_order( | ||
| &pool, | ||
| third_id, | ||
| Some(&other_pubkey), | ||
| Some(&other_pubkey), | ||
| &other_pubkey, | ||
| ) | ||
| .await; | ||
|
|
||
| let requested = vec![second_id, first_id, third_id]; | ||
|
|
||
| let orders = get_user_orders_by_id(&pool, &requested, &user_pubkey) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(orders.len(), 2); | ||
| assert_eq!(orders[0].id, second_id); | ||
| assert_eq!(orders[1].id, first_id); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_get_user_orders_by_id_empty_input() { | ||
| let pool = setup_orders_pool().await; | ||
| let user_pubkey = "a".repeat(64); | ||
|
|
||
| let orders = get_user_orders_by_id(&pool, &[], &user_pubkey) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(orders.is_empty()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Tests mask production bug: master keys should be encrypted.
The test functions insert plaintext values for master_buyer_pubkey and master_seller_pubkey (lines 517-518), while production code encrypts these fields using CryptoUtils::store_encrypted (lines 214-219, 227-232 in prepare_new_order). This causes the tests to pass despite the critical bug in get_user_orders_by_id where encrypted database values are compared directly with plaintext.
To properly test production behavior, the test should encrypt the master keys before insertion, or use a different approach that doesn't rely on direct comparison.
🔎 Suggested test fix
Either update the test to use encrypted values:
async fn insert_order(
pool: &SqlitePool,
id: Uuid,
identity_buyer_pubkey: Option<&str>,
identity_seller_pubkey: Option<&str>,
creator_pubkey: &str,
) {
+ // Encrypt the master keys to match production behavior
+ let encrypted_buyer = identity_buyer_pubkey.map(|pk| {
+ CryptoUtils::store_encrypted(pk, MOSTRO_DB_PASSWORD.get(), None).unwrap()
+ });
+ let encrypted_seller = identity_seller_pubkey.map(|pk| {
+ CryptoUtils::store_encrypted(pk, MOSTRO_DB_PASSWORD.get(), None).unwrap()
+ });
+
sqlx::query(/* ... */)
.bind(id)
// ... other binds ...
- .bind(identity_buyer_pubkey)
- .bind(identity_seller_pubkey)
+ .bind(encrypted_buyer)
+ .bind(encrypted_seller)
.execute(pool)
.await
.unwrap();
}Or add a comment explaining why the test uses plaintext (if there's a valid reason), and add integration tests that use real encrypted values.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/util/orders.rs around lines 450 - 601, Tests insert plaintext
master_buyer_pubkey/master_seller_pubkey which masks the production behavior
where prepare_new_order encrypts those fields via CryptoUtils::store_encrypted,
so update the tests to insert encrypted values (call
CryptoUtils::store_encrypted or the test-side helper that mirrors it) into
insert_order before binding master_buyer_pubkey/master_seller_pubkey, or
alternatively mock CryptoUtils::store_encrypted in the test to return the
plaintext sentinel and use that sentinel in insert_order; ensure references to
prepare_new_order, CryptoUtils::store_encrypted, get_user_orders_by_id, and the
test helper insert_order are used so the test exercises the same
encryption/decryption logic as production (or add an integration test that
writes real encrypted rows).
Using std::thread::sleep in an async function blocks the entire runtime thread, preventing other tasks from making progress. Use tokio::time::sleep instead.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Utilities
Chores
✏️ Tip: You can customize this high-level summary in your review settings.