-
Notifications
You must be signed in to change notification settings - Fork 5
Two proposals for a trade index synch message #21
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
Conversation
…a user between clients and mostro
WalkthroughAdds protocol documentation for a new restore action "last-trade-index" (version 1): client sends a Gift-wrapped Nostr event with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant R as Nostr Relay
participant S as Restore Service
Note over C: Build Gift-wrapped event<br/>`restore.version: 1`<br/>`restore.action: "last-trade-index"`<br/>`payload: null`
C->>R: Publish request event
R->>S: Deliver request
rect rgba(232,245,233,0.6)
Note over S: Lookup user's last trade index
alt Trades exist
S-->>R: Response event<br/>`restore.version: 1`<br/>`restore.trade_index = N`<br/>`payload: null`
else No trades
S-->>R: Response event<br/>`restore.version: 1`<br/>`restore.trade_index = 1`<br/>`payload: null`
end
end
R-->>C: Relay response event
Note over C: Consume `trade_index` (next trade should use `trade_index + 1`)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
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: 3
🧹 Nitpick comments (3)
src/restore_trade_index.md (1)
37-43: Tighten normative statements and forward‑compat guidanceAdd that unknown fields MUST be ignored and response MUST echo version/action for robustness and evolvability.
Suggested additions:
- Response MUST echo the same
restore.versionandrestore.action.- Receivers MUST ignore unknown fields in
restoreandpayloadfor forward compatibility.- Validation: request MUST contain
payload.last_trade_index: null; any other non‑null value SHOULD be rejected with an error.src/restore_trade_index_second_option.md (2)
1-3: Normalize action naming: “sync” vs “synch” and align with examplesHeader says “Sync”, description says “synch‑trade‑index”, fields/examples use
last-trade-index. Use a single action name; per examples, preferlast-trade-index, and drop “synch”.Apply this diff:
-# Sync Trade Index - -Defines the `synch-trade-index` action used to retrieve the user's last `trade_index`. +# Last Trade Index + +Defines the `last-trade-index` action used to retrieve the user's last `trade_index`.-* `restore.action`: Must be `last-trade-index`. +* `restore.action`: MUST be `last-trade-index`.Also applies to: 35-37
40-52: Consider returning both last and next to eliminate client ambiguityOptional: include
next_trade_indexin the response to avoid client off‑by‑one mistakes and document the source of truth.Example:
{ "restore": { "version": 1, "action": "last-trade-index", "trade_index": 7, "next_trade_index": 8, "payload": null } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/restore_trade_index.md(1 hunks)src/restore_trade_index_second_option.md(1 hunks)
🔇 Additional comments (1)
src/restore_trade_index.md (1)
7-7: Consistent naming: “Mostro” vs “Mostrod”The PR text references “Mostrod”, while this doc uses “Mostro”. Pick one term and use it consistently in all docs/specs.
Would you confirm the canonical name and update accordingly?
Also applies to: 23-23
grunch
left a comment
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.
great proposal, let's go with the last-trade-index new action
grunch
left a comment
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.
great add it to the index in overview.md
grunch
left a comment
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.
LGTM
@grunch @Catrya @AndreaDiazCorreia ,
This PR introduces two approaches to add a message for synchronizing the user's last trade index between clients and Mostrod. Such synchronization is useful in several scenarios:
These are the two options i propose, both are considered restore messages:
LastTradeIndexwith a null value in the request, mostro in the response will send last trade index as the value inside new payload answerLastTradeIndexwith a null payload as request, mostro in the response will send last trade index inside trade index field.In my opinion, probably, a new action is the more linear approach, more similar to other messages and we can leverage the
trade_indexinside MessageKind struct to get the index without adding other fieldsNow tell me your opinion about...
Summary by CodeRabbit