fix(#490): child order of buy order was not sent to maker, but to taker

[arkanoider]

@Catrya

this is the fix for #490 .
Tested with a sell order and a buy order - now child order seems correctly sent to makers.

Summary by CodeRabbit

• Bug Fixes
  • Improved order handling logic for better accuracy in updating trade details.
  • Enhanced permission checks to ensure only authorized users can release funds or send fiat.
  • Fixed update flow for order status changes to improve clarity and reliability.

[coderabbitai]

Walkthrough

An explicit seller permission check was added in release_action to ensure only the seller can release funds. The handle_child_order function was refactored to use explicit conditional branches based on order kind and creator pubkey, updating child order fields accordingly and adding encryption with optional salt. In fiat_sent_action, order update and database write were reorganized to defer the update until after conditional logic for range orders.

Changes

File(s)	Change Summary
src/app/release.rs	Added explicit seller permission check in release_action; refactored handle_child_order to use explicit if-else conditions for buyer/seller pubkey checks and child order field updates; added helper functions for buy/sell child orders with encryption and optional salt; updated function signature to return Result<(), MostroError>.
src/app/fiat_sent.rs	Reorganized fiat_sent_action to defer database update until after conditional logic for range orders; added comments clarifying buyer-only permission for sending fiat and notification purpose.

Possibly related PRs

• Get the right next trade fields on release #422: Modifies release_action in src/app/release.rs to adjust handling of next_trade and permission checks, closely related to seller pubkey validation.
• Code optimization on release #424: Updates handle_child_order function signature and internal logic in src/app/release.rs, related to child order processing changes.
• Handle next trade on fiatsent #421: Adds and handles next_trade_pubkey and next_trade_index fields during fiat sent and release actions, including database schema changes, closely related to child order handling and next trade logic.

Suggested reviewers

• Catrya
• grunch

Poem

🐇 A seller’s key now guards the gate,
Child orders shaped by fate.
Buyer sends the fiat true,
Updates wait, then flow anew.
In code’s soft burrow, clear and bright,
Rabbits hop through day and night! 🌸✨

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75fe9b4 and 443f684.

📒 Files selected for processing (1)

• src/app/release.rs (4 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: tests

🔇 Additional comments (9)

src/app/release.rs (9)

12-12: LGTM: Imports support new cryptographic functionality.

The new imports for argon2::password_hash::SaltString and rand::rngs::OsRng are correctly added to support the salt generation functionality for privacy improvements.

Also applies to: 19-20

---

64-118: Excellent documentation that enhances code maintainability.

The comprehensive documentation clearly explains the function's purpose, flow, arguments, return values, errors, and security considerations. This significantly improves code maintainability and developer experience.

---

135-138: Critical security fix: Seller permission check correctly implemented.

This permission check addresses the core issue mentioned in the PR by ensuring only the seller can release funds. The check compares the seller's pubkey from the order with the event's rumor pubkey, preventing unauthorized releases.

---

212-219: Helper function provides clean abstraction for encryption.

The store_encrypted_pubkey function properly encapsulates the encryption logic with appropriate error handling and mapping to internal errors.

---

221-254: Well-structured helper functions improve code modularity.

The handle_buy_child_order and handle_sell_child_order functions effectively separate the logic for different order types, making the code more maintainable and easier to understand. The functions properly handle field assignments, salt generation, and return the necessary information for notifications.

Also applies to: 256-291

---

365-382: Robust error handling ensures proper notification delivery.

The code correctly validates that both notification_pubkey and new_trade_index are present before attempting to send notifications. The error message at lines 379-381 clearly indicates when the notification cannot be sent, which helps with debugging.

---

384-389: Proper database operation with error handling.

The child order creation includes appropriate error mapping from database errors to internal service errors, maintaining consistent error handling patterns throughout the codebase.

---

348-360:

Details

❓ Verification inconclusive

Verify complex conditional logic for order type determination.

The logic checks both order type (buy/sell) and creator matching (buyer_pubkey/seller_pubkey == creator_pubkey). This determines which helper function to call for child order handling.

Please ensure this logic correctly identifies all valid scenarios and properly handles edge cases.

---

🏁 Script executed:

#!/bin/bash
# Description: Verify order type checking logic and creator matching patterns
# Expected: Understanding the relationship between order types and creator roles

# Search for is_buy_order and is_sell_order implementations
ast-grep --pattern 'fn is_buy_order($$$) {
  $$$
}'

ast-grep --pattern 'fn is_sell_order($$$) {
  $$$
}'

# Search for other usages of creator_pubkey matching patterns
rg -A 3 -B 3 "creator_pubkey.*==|==.*creator_pubkey"

# Search for order creation patterns to understand when creator matches buyer/seller
rg -A 5 "creator_pubkey.*=|=.*creator_pubkey"

Length of output: 3895

---

Review child order conditional branches

Please confirm that the is_buy_order() and is_sell_order() checks accurately capture every valid order type and that comparing buyer_pubkey/seller_pubkey to creator_pubkey handles None or mismatched values correctly. In particular:

• Verify that is_buy_order().is_ok() only succeeds for legitimate buy orders, and is_sell_order().is_ok() only for legitimate sell orders.
• Ensure the .as_ref() == Some(&order.creator_pubkey) comparisons don’t unintentionally exclude valid purchase or sale scenarios when the optional pubkeys are present or absent.
• Add unit tests covering:
  • A valid buy order where buyer_pubkey == creator_pubkey
  • A valid sell order where seller_pubkey == creator_pubkey
  • Cases with buyer_pubkey or seller_pubkey unset or not matching, to verify the UnexpectedError branch only fires on truly invalid states.

---

237-242:

Details

✅ Verification successful

Verify salt generation logic for privacy modes.

The current logic generates salt when normal_buyer_idkey/normal_seller_idkey is Some, which suggests salt is added in normal/rating mode but not in full privacy mode. This seems counterintuitive from a privacy perspective.

Please verify this behavior is intentional and aligns with the privacy model design.

Also applies to: 275-280

---

🏁 Script executed:

#!/bin/bash
# Description: Verify the is_full_privacy_order function behavior and salt usage patterns
# Expected: Understanding when salt should be used based on privacy mode

# Search for is_full_privacy_order function definition
ast-grep --pattern 'fn is_full_privacy_order($$$) {
  $$$
}'

# Search for other usages of salt in encryption
rg -A 5 -B 5 "SaltString::generate"

# Search for documentation or comments about privacy modes
rg -A 3 -B 3 "full.privacy|normal.*privacy|rating.*mode"

Length of output: 5743

---

🏁 Script executed:

#!/bin/bash
# Locate the definition and implementation of is_full_privacy_order across the repository
rg -n "fn is_full_privacy_order" -C 5
rg -n "is_full_privacy_order" -C 5

Length of output: 1680

---

Salt generation logic is correct for non–full-privacy orders

The code intentionally only generates a salt when normal_*_idkey is Some—i.e. when a buyer/seller identity key is available (normal/rating mode) and encryption is performed. In full-privacy mode (None), there is no identity key and no need for a salt or encryption, so salt remains None.

No changes required.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[Catrya]

Hi @arkanoider
Now in purchase orders Mostrod sends the new-order message of the child order to the correct user, but when someone takes the child order that goes to waiting-buyer-invoice status, it sends the add-invoice message to the taker of the first order. In the database, in creator_pubkey, there is the trade key of the taker, not the maker (i mean the next trade key, no the key the used in 1st time)

In sales orders it still works well

[arkanoider]

great...i will revert this. It's weird it did not work before, will look deeper.

[Catrya]

Hi @arkanoider mostro crashes when it receives the release action in a purchase range order.

2025-06-06T02:24:02.104833Z  INFO mostrod::util: Sending DM, Event ID: be5822df7adf89c9dca0dd339b88b66e097efeb9fb7854b4f8e1f1ff37825db4 with payload: "{\"order\":{\"version\":1,\"request_id\":null,\"trade_index\":null,\"id\":\"02f51039-bd0f-492b-989d-e0fd7695f806\",\"action\":\"released\",\"payload\":null}}"
thread 'main' panicked at src/app/release.rs:187:74:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10ffe3d and 71ee253.

📒 Files selected for processing (2)

• src/app/fiat_sent.rs (5 hunks)
• src/app/release.rs (2 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (2)

src/app/fiat_sent.rs (1)

src/util.rs (1)

• update_order_event (510-548)

src/app/release.rs (2)

src/util.rs (1)

• enqueue_order_msg (848-863)

src/app.rs (1)

• e (372-372)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: tests

🔇 Additional comments (5)

src/app/fiat_sent.rs (2)

23-27: Good security check for buyer validation.

The explicit check and clear comments help prevent unauthorized fiat sent actions.

---

37-91: Well-structured order update flow for range orders.

The reorganization to defer database updates until after setting next trade fields is a good approach. This ensures all necessary fields are properly set before persisting to the database, which is crucial for child order routing in range orders.

src/app/release.rs (3)

77-80: Essential security check for seller validation.

Good addition to ensure only the seller can release funds, maintaining proper order flow security.

---

182-215: Correct implementation for buy order child routing.

This logic properly routes child orders to the buyer (maker) in buy orders by using the next_trade_pubkey and next_trade_index fields that were set during the fiat sent action. This fixes the issue where child orders were incorrectly sent to takers.

---

216-252: Well-structured conditional logic for order type handling.

The explicit conditional branches for buy vs sell orders with creator validation ensure child orders are correctly routed to the appropriate party. The error handling for missing next trade information is comprehensive.

[Catrya]

@arkanoider I did some quick testing, it's assigning the child orders correctly now.
I haven’t tested it in privacy mode yet; I’ll do a bit more testing, but it’s heading in the right direction!

[arkanoider]

@Catrya seems working on my side. Tested also privacy orders and seems ok. I fixed and refactored the code for better readability. Improved privacy: now child order master seller/buyer key is salted in case of normal user, previously they were the same of parent order.
As usual you have the final sentence on tests! 😉

[Catrya]

tACK great job @arkanoider

[grunch]

utACK 🔥