-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle events with action TradePubkey #412
Conversation
WalkthroughThe pull request introduces a new module Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant Database
User->>App: Send TradePubkey Message
App->>Database: Retrieve Order
alt Order Not Found
App-->>User: Error: No Order
else Order Not Pending
App-->>User: Error: Invalid Order Status
else Sender is Valid
App->>Database: Update Order with Public Key
App->>User: Confirm Public Key Trade
else Sender is Invalid
App-->>User: Error: Invalid Public Key
end
Possibly related PRs
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/trade_pubkey.rs (3)
33-43
: Status validation is thorough, but note consistency with non-existent orders.
When the status is invalid (notPending
), you send a “can't do” message to the user. For non-existent orders, the code simply returnsOk(())
. Consider using a similar approach as here, sending a user notification if the order does not exist, for consistency in user messaging.
59-59
: Minor typo in comment.
“// We a message to the seller” could be corrected to “// We send a message to the seller”.-// We a message to the seller +// We send a message to the seller
70-70
: Consider transactional updates for atomicity.
Updating the order in the database is crucial. If other modifications (e.g., sending messages) need to happen atomically, wrapping them in a database transaction can prevent inconsistent states if errors occur.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app.rs
(4 hunks)src/app/trade_pubkey.rs
(1 hunks)
🔇 Additional comments (8)
src/app/trade_pubkey.rs (4)
1-11
: Imports look consistent and purposeful.
All imported symbols (send_cant_do_msg
, send_new_order_msg
, etc.) are referenced in this file, and no redundant imports are evident. Good job!
25-31
: Consider informing the user when the order is not found.
Currently, the code logs an error but then returns Ok(())
without sending any user-facing message. If the caller expects a notification (e.g., a DM) indicating the order does not exist, you may wish to send it before returning. Otherwise, this silent flow might confuse end-users.
45-56
: Match logic correctly restricts trades to known buyer/seller.
This prevents unauthorized users from trading on an order by matching sender.to_string()
to master_buyer_pubkey
or master_seller_pubkey
. Looks secure and well-implemented.
57-57
: Double-check the semantics of overwriting creator_pubkey
.
By setting order.creator_pubkey = event.rumor.pubkey.to_string()
, you are effectively reassigning the order’s creator_pubkey
. Is that intentional, or should you store this as a separate “last updated by” pubkey?
src/app.rs (4)
18-18
: New module declaration is cleanly added.
Publishes the trade_pubkey
module for use in other parts of the application. This is straightforward and organized.
34-34
: Importing the new action handler.
Importation of trade_pubkey_action
is appropriately placed among other action imports, maintaining clarity.
76-76
: Handled new action in check_trade_index
.
Including Action::TradePubkey
in the trading-related actions ensures check_trade_index
logic runs for public key trades. This is consistent with your approach for buy/sell actions.
195-195
: Routing Action::TradePubkey
to trade_pubkey_action
.
Seamlessly integrates the new action into the existing handling structure. Confirm if you also need to pass my_keys
or ln_client
for any key-related or LN tasks. If not, this is perfectly fine.
Summary by CodeRabbit modified by @grunch
New Features
Bug Fixes