fix: stop inbound payloads from restating what a trade is - #665
fix: stop inbound payloads from restating what a trade is#665AndreaDiazCorreia wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b368fe0b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Bond acks (3.5) and the slash notice (4) carry a bond-sized SmallOrder | ||
| // with a null status; it was never the trade order. | ||
| if (isBondPayoutAck) return order; | ||
| if (message.payload is! Order) return order; |
There was a problem hiding this comment.
When a user takes an existing order, AbstractMostroNotifier starts with order: null, the outbound take carries an Amount or PaymentRequest rather than an Order, and the first node response can itself be a pay-invoice or pay-bond-invoice PaymentRequest. This return therefore leaves the tracked order null: the invoice screen renders fiat as 0 with no currency, while bond validation receives a null orderAmountSats and deliberately performs only the floor check, allowing an arbitrarily oversized bond. Seed the state from the public order/local take before discarding the embedded payment order.
Useful? React with 👍 / 👎.
| final info = ref.watch(orderRepositoryProvider).mostroInstance; | ||
| final instance = info == null ? null : MostroInstance.fromEvent(info); |
There was a problem hiding this comment.
If the bond screen opens before the kind-38385 info event arrives, mostroInstance is null and the legitimate bond is shown as unadvertised. Watching orderRepositoryProvider only watches the repository object's identity; mutation of its mostroInstance field does not rebuild this widget, even though the repository explicitly publishes later values through mostroInstanceStream. The refusal can therefore remain until an unrelated rebuild or route re-entry, blocking a valid bond payment during the normal startup race.
Useful? React with 👍 / 👎.
| advertised: instance?.bondPolicy == BondPolicy.enabled, | ||
| amountPct: instance?.bondAmountPct, | ||
| baseAmountSats: instance?.bondBaseAmountSats, |
There was a problem hiding this comment.
When an enabled policy advertises bond_apply_to=take, this check still accepts a correctly sized maker bond, and likewise accepts a taker bond under a maker-only policy; a missing or malformed bondApplyTo is also ignored. Since MostroInstance explicitly models which side must lock the bond, requests that contradict that field can still make users lock funds and potentially be slashed despite the advertised policy. Include the current maker/taker context and require bondApplyTo to authorize that side.
Useful? React with 👍 / 👎.
1 participant
Depends on #660
Cut from
fix/replay-and-censorship-protection, which rewrote the two files this builds on. Merge #660 first, or review against that branch.What
Three payloads were taken at their word about things they should not have been able to state.
Trade terms. An inbound Order payload replaced the tracked order outright, so any later message could restate every economic field — including the payment method the trade screen renders as the account to send fiat to. The fiat terms now freeze once the order leaves pending, since nothing in the protocol renegotiates them. The sats amount stays free: a market-price order has none until it is taken.
A PaymentRequest's embedded order no longer becomes the tracked one either. Its amount is the figure for that payment — the hold invoice is the order plus the seller's fee, the payout the order less the buyer's — so it was never the trade. The pay screen now reads its figure from the payment request, where the node states it, rather than from an order the payment request had overwritten.
Bond amounts. The node publishes in its kind-38385 event that it charges bonds, the percentage it takes and the floor it never goes below, and sizes every bond from exactly those. Nothing gated an inbound
pay-bond-invoiceon any of it. A request is refused when no policy was advertised, when it falls below the floor, and when it is not what the percentage yields for this order.Trade key index. Index 0 is what the master identity is derived from, so deriving "trade key 0" hands back that key and a session built on it signs and runs ECDH under the identity a trade depends on not sharing.
setCurrentKeyIndexhas always refused it; derivation is reachable without going through the counter, and restore derives straight from an index in the response.Notes
pay_lightning_invoice_screen.dart: both change where the payment figure comes from. Keep this branch's reading frompaymentRequest.order.flutter analyzeclean;flutter test1215 passing.