Complete the MuSig create and take offer rewrite - #4966
Conversation
|
Important Review skippedToo many files! This PR contains 257 files, which is 157 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (257)
You can disable this status message by setting the 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 |
| model.isPriceValid.set(true); | ||
| updateFromMarketPrice(); | ||
|
|
||
| marketPricePin = marketPriceService.getMarketPriceByCurrencyMap().addObserver(() -> { |
There was a problem hiding this comment.
Should we avoid writing the current market price back into PriceSelection on every market price map update?
onSetPriceQuote() also recalculates and persists the percentage, so this appears to reset a fixed price, or a non-zero floating markup, to the current market price / 0% without a user action. The same mutation also happens twice during activation above.
Should we preserve a fixed quote, and for floating pricing recompute the derived quote while preserving the selected percentage?
There was a problem hiding this comment.
Confirmed. Fixed in e4865f3 — the market observer now leaves a fixed price alone and, for a floating price, recomputes the quote from the kept percentage; activation no longer pushes the market price. While doing this I found a deeper, separate issue: the price-step controller round-trips every published quote back through onSetPricePercentage (no origin separation), which drifts the floating percentage on a market update and rewrites a fixed price on re-entry. Filed it as #4967 to fix on its own with proper testing.
| userSpecificAmountLimitsProvider = new UserSpecificAmountLimitsProvider(marketPriceService, marketService, directionService, priceService); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Should we explicitly calculate all three limit providers from the current market, direction, price, and payment state during initialization?
AbsoluteAmountLimitsProvider and UserSpecificAmountLimitsProvider only register custom listeners, but those listeners do not replay their current values. On the normal default path, those values were established before these listeners were attached, so AmountLimitsProvider can remain uninitialized and the amount input handlers silently ignore edits.
Could we also add a test proving that AmountSelection.initialize() becomes usable without manually firing dependency listeners?
There was a problem hiding this comment.
Confirmed. Fixed in 11a20d7. Each sub-provider now computes its value in initialize() instead of waiting for a listener that never replays. Added a test.
| @@ -259,10 +344,18 @@ public void takeOffer() { | |||
| delayedSuccessScheduler.stop(); | |||
| } | |||
| delayedSuccessScheduler = UIScheduler.run(() -> { | |||
There was a problem hiding this comment.
Should we keep this attempt in SENT or a pending state until we receive an actual protocol or delivery success signal?
MuSigTradeService.takeOffer() dispatches protocol handling asynchronously, so the absence of an error after 200 ms does not establish success. A rejection or delivery failure can arrive after this scheduler has already shown the success state and stopped the timeout.
If no completion signal is currently available, would a pending state be safer than reporting success based on elapsed time?
There was a problem hiding this comment.
Agreed, elapsed time isn't a real success signal. The protocol handler doesn't surface a delivery/completion event yet (that's the todo here), so there's nothing to drive a genuine pending→success transition — the 200 ms + error observer + timeout is the interim. Leaving it until the protocol reports delivery and fixing it there; I'll track it as a rewrite item on #4883.
| model.clearAccountsByPaymentMethod(); | ||
| } | ||
|
|
||
| public void putSelectedAccountByPaymentMethod(PaymentMethod<?> paymentMethod, Account<?, ?> account) { |
There was a problem hiding this comment.
Should we enforce the payment-selection invariant at this public mutation boundary?
Could we verify that the account's payment method equals paymentMethod, that the account belongs to the eligible account list for that method, and that the method is offered? getSelectedPaymentMethodSpec() later derives the spec from the map key while getSelectedAccount() returns the value, so an inconsistent entry would be handed to the protocol as a mismatched spec/account pair.
The current controller passes matching values, but the take offer specification identifies the use case as the enforcement point for this guarantee.
There was a problem hiding this comment.
Confirmed. Fixed in 9c43f9d. Checks the account matches the method key and is in the eligible list (which is offered-only, so that's covered too).
| checkArgument(min.getBaseSideMonetary().getCode().equals(max.getBaseSideMonetary().getCode()), | ||
| "this and max base side codes must match. this.base=%s; max.base=%s", | ||
| min.getBaseSideMonetary().getCode(), max.getBaseSideMonetary().getCode()); | ||
| checkArgument(min.getQuoteSideMonetary().getCode().equals(max.getQuoteSideMonetary().getCode()), |
There was a problem hiding this comment.
Should we also verify that this quote's quote side currency matches the min/max quote side currency?
The current checks compare this quote's base currency and verify that min/max match each other, but never compare this.getQuoteSideMonetary().getCode() with the limits. As a result, a BTC/EUR quote whose numeric value is inside BTC/USD limits can be returned unchanged.
Would comparing both sides of all three quotes here make this clamp fail closed across markets?
There was a problem hiding this comment.
Confirmed. The clamp now also compares this quote's quote side against the limits, so a quote from another market fails closed instead of being returned unchanged. Fixed in: 4d1799d
| // User interaction | ||
| /* --------------------------------------------------------------------- */ | ||
|
|
||
| public void onSetUseBaseCurrencyForAmountInput(boolean value) { |
There was a problem hiding this comment.
Should we recompute the input amount range, user limit marker, and normalized slider values when the active input side changes?
At present this only changes the boolean and persists it, leaving the derived state denominated and mapped using the previous input side until another limits update occurs. The create offer specification explicitly lists this recomputation as an update trigger, and the take flow performs the corresponding recalculation in TakeOfferUseCase.setUseBaseCurrencyForAmountInput().
There was a problem hiding this comment.
Confirmed. Fixed in c01269f. The switch now recomputes range/marker/slider on the new side, and the amount + slider controllers use the take flow's origin separation so the programmatic re-write on the switch no longer feeds back against the stale range (verified live for both fixed and range offers).
| min.getQuoteSideAmount().getCode(), max.getQuoteSideAmount().getCode()); | ||
| } | ||
|
|
||
| public String printRelevantStringa(Market market) { |
There was a problem hiding this comment.
printRelevantStringa appears to be a misspelled duplicate of printRelevantString immediately below it, and I could not find any callers.
There was a problem hiding this comment.
Confirmed, removed. Dead misspelled duplicate with no callers (the same method was also in PriceQuoteRange). Fixed in: 0f0adbd
|
|
||
| private void applyPricePercentage(double pricePercentage, boolean notifyListeners) { | ||
| if (Double.compare(pricePercentage, model.getPricePercentage()) != 0) { | ||
| pricePercentage = priceLimits.clamp(pricePercentage); |
There was a problem hiding this comment.
Should the create-offer UI route out-of-range percentage input through this clamp?
MuSigCreateOfferPriceController.applyPercentageString() retains the previous behavior of validating and returning before onSetPricePercentage() is called. That means this newly added clamp, and the specification saying that UI input is clamped, does not currently apply to percentage input.
Is the intended behavior to change the UI to use this clamp, or should we retain rejection and adjust the specification accordingly?
There was a problem hiding this comment.
Agreed, clamp it, spec stays. Can't do it on its own though — it hits the same feedback loop as #4967: even routing the clamped percentage into the domain, the quote observer derives it back from the rounded quote and validateQuote rejects it again (a valid small percentage on a low-sat crypto market gets rewritten the same way). So I'll fold the clamp into #4967 — the origin separation there is what makes it stick.
| // market price, keeping the percentage. Only when this market has a price available. | ||
| Market market = draftOfferUseCase.getMarket(); | ||
| if (market != null && marketPriceService.findMarketPrice(market).isPresent()) { | ||
| priceSelection.onSetPricePercentage(priceSelection.getPricePercentage()); |
There was a problem hiding this comment.
Should we refresh the price limits from the same market price update before recomputing the floating quote?
PriceLimits.initialize() only reacts to market selection, so its PriceQuoteRange remains based on the previous market price. This call computes a quote from the new market price, but PriceSelection.applyPriceQuote() then clamps it against that stale range. For example, if the market moves from 100 to 200, a 0% quote is calculated as 200 but clamped to the old +50% maximum of 150, while the stored percentage remains 0%.
Should this market-price update live in PriceSelection/PriceLimits so the limits and quote can be refreshed together? Could we also cover upward and downward market ticks, including the -10% and +50% endpoints, with domain tests?
| })); | ||
|
|
||
|
|
||
| pins.add(priceSelection.priceQuoteObservable().addObserver(priceQuote -> |
There was a problem hiding this comment.
Should we keep domain published quote updates display only before this PR merges?
This observer calls onQuoteInput() → applyPercentageFromQuote() → PriceSelection.onSetPricePercentage(), writing a displayed projection back into the domain. The percentage observer has a similar path through applyPercentageString() and setQuote(). Registration and market price updates can therefore rewrite a fixed price on re-entry or drift a floating percentage through monetary rounding.
#4967 documents the reproductions, but this controller is introduced by this PR and the behavior changes trade prices without user input. Should the origin separation and its regression tests land here rather than being deferred?
There was a problem hiding this comment.
I filed #4967 to document the issue and make scoping and review focused, since this refactor is estimated to be quite big chunk. The plan is to deliver the fix on this branch and close the issue pointing at that commit. IS that OK?
| checkNotNull(inputAmount, "inputAmount must not be null"); | ||
| Market market = marketSelection.getMarket(); | ||
| PriceQuote priceQuote = priceSelection.getPriceQuote(); | ||
| if (amountLimits.isInitialized() && market != null && priceQuote != null) { |
There was a problem hiding this comment.
Should we base these input guards on the currently available limit ranges rather than the latched isInitialized() flag? The invalid-dependency path now clears effectiveTradeAmountLimits and potentialTradeAmountLimits while leaving initialized true.
After a valid USD draft is switched to EUR while BTC/USD is temporarily unavailable, entering an amount reaches clamp(..., null) and throws an NPE. I reproduced this through onSetFixTradeAmountFromInputAmount.
Could we fail softly until the current limits are available, use both ranges where required by the slider handlers, and add a market-switch regression test?
| // Empty when a required market price is missing (e.g. the draft started before the | ||
| // first price arrived): the default seed is applied when the limits first initialize. | ||
| try { | ||
| return MarketBasedAmountConversion.findTradeAmountFromUsdAmount(marketPriceService, market, DEFAULT_TRADE_AMOUNT_IN_USD); |
There was a problem hiding this comment.
Should we treat every non-positive required price quote as unavailable here, matching PriceSelection and TradeAmountLimitUtils.findRates? MarketPrice.verify() validates only the timestamp.
With a positive BTC/EUR quote but a negative BTC/USD quote, default seeding produces a negative TradeAmount. The limit providers reject that rate, but isDraftReadyForReview() still returns true because it checks only for non-null values; materializing the negative amount spec later fails verification.
Should we validate every conversion leg as > 0?
| this.market = market; | ||
| return fresh; | ||
| } | ||
| if (rates != null && market.equals(this.market)) { |
There was a problem hiding this comment.
Should we invalidate the cached rates as soon as the requested market differs, even when no fresh rates are available?
With A(valid) → B(no rates), the A rates remain cached. Returning B → A while rates are still unavailable then returns the pre-switch A rates. This means retention does span a market change despite the class contract.
Could we clear the cache on the first market change and add an A → B → A regression test?
| // these with a snapshot so a concurrent market-price update cannot produce mixed values. | ||
| /* --------------------------------------------------------------------- */ | ||
|
|
||
| public static Optional<Monetary> findBaseSideFixedAmount(PriceQuote resolvedPriceQuote, AmountSpec amountSpec, Market market) { |
There was a problem hiding this comment.
Should we verify that resolvedPriceQuote.getMarket().equals(market) before performing these conversions?
Unlike the service-backed overloads, these six public methods accept the quote and market independently. PriceQuote conversion checks only the Fiat/Coin runtime type, not the currency code, so a BTC/USD quote passed with a BTC/EUR market silently treats an EUR amount as USD. A focused test expecting an IllegalArgumentException confirmed that no exception is currently thrown.
Should we centralize the preconditions for all six overloads and add mismatched-market coverage?
| // first price arrived): the default seed is applied when the limits first initialize. | ||
| try { | ||
| return MarketBasedAmountConversion.findTradeAmountFromUsdAmount(marketPriceService, market, DEFAULT_TRADE_AMOUNT_IN_USD); | ||
| } catch (ArithmeticException e) { |
There was a problem hiding this comment.
Should we remove this catch, or update it to describe the exception it is still intended to handle? MarketBasedAmountConversion now filters zero and negative quotes to Optional.empty() before conversion, so a zero price no longer reaches this ArithmeticException branch. The current comment therefore describes control flow that no longer occurs.
| String maxTradeLimit = MuSigTradeAmountLimits.getFormattedMaxTradeLimitInUsd(fiatPaymentRail); | ||
| private void applySelectFiatPaymentRail(FiatPaymentRail paymentRail) { | ||
| model.getSelectedFiatPaymentRail().set(paymentRail); | ||
|
|
There was a problem hiding this comment.
Should we remove the trailing spaces from this blank line so git diff --check is clean?
|
Open product related question. None of them requires an immediate decision. MuSig trade fee policy/schedule Max price-deviation bound Reputation source for the buyer amount cap Wallet affordability check |
bbbc435 to
42e691d
Compare
| Monetary quoteSideAmount; | ||
| if (market.isBtcFiatMarket()) { | ||
| PriceQuote btcFiatPriceQuote = marketPriceService.getMarketPriceQuoteOrThrow(market); | ||
| quoteSideAmount = AmountConversion.usdToFiat(btcUsdPriceQuote, btcFiatPriceQuote, usdAmount); |
There was a problem hiding this comment.
This “exact” take-side limit can still wrap before the final checked conversion. AmountConversion.usdToFiat() uses the legacy PriceQuote conversions, which end in longValue().
A focused test with positive rates turned the intended $10,000 cap into a positive wrapped limit of €155,325,592,629,044.8384 / 1,553,255,926.29044838 BTC instead of throwing. Because the result remains positive, the downstream guard accepts it and the absolute cap can effectively be bypassed.
| checkNotNull(usdAmount, "usdAmount must not be null"); | ||
|
|
||
| Market usdBitcoinMarket = MarketRepository.getUSDBitcoinMarket(); | ||
| PriceQuote btcUsdPriceQuote = marketPriceService.getMarketPriceQuoteOrThrow(usdBitcoinMarket); |
There was a problem hiding this comment.
One amount constraint recomputation can combine different market-price snapshots. This reads BTC/USD and the offer market separately; for a BTC/USD offer it reads the same map entry twice. A sequential 100,000 → 200,000 mock turns a $100 cap into $200. computeAmountConstraints() then calls this helper independently for the absolute minimum, absolute maximum, payment-rail limit, and user limit.
findRates() above already captures and reuses one context to avoid this race. Should we capture one Rates snapshot at the start of computeAmountConstraints(), pass it through a checked overload for every limit?
| * Like {@link #toTradeAmount(Market, PriceQuote, Monetary)}, but fails with an | ||
| * ArithmeticException when the converted side does not fit into a long. | ||
| */ | ||
| public static TradeAmount toTradeAmountExact(Market market, PriceQuote priceQuote, Monetary amount) { |
There was a problem hiding this comment.
toTradeAmountExact() checks that the amount belongs to market, but it does not verify that priceQuote belongs to the same market. A focused test with a BTC/EUR market, BTC/USD quote, and EUR amount succeeds and returns a BTC/EUR pair calculated using the USD rate, because PriceQuote checks only the Fiat/Coin runtime type.
The same invariant is now enforced in OfferAmountUtil. Should we validate market.equals(priceQuote.getMarket()) in both public conversion overloads and add mismatched-market coverage?
| formatBtcAmount(amount); | ||
| try { | ||
| formatBtcAmount(amount); | ||
| } catch (Exception ignored) { |
There was a problem hiding this comment.
This empty broad catch leaves the previously rendered amount visible. If the property changes from a valid value to malformed text, the control is made visible before parsing fails, so stale BTC data appears current. It also hides unexpected programming failures.
Should we catch only the expected parse failure, explicitly clear or hide an invalid value, and let unexpected exceptions remain visible?
| } | ||
| return quoteSideAmount.compareToRange(quoteSideLimits); | ||
| } | ||
| // |
KimStrand
left a comment
There was a problem hiding this comment.
Six newly added test classes start directly with the package declaration.
Can we please add the standard licence header to these files as well?
| Fiat usdAmount) { | ||
| checkNotNull(rates, "rates must not be null"); | ||
| checkNotNull(market, "market must not be null"); | ||
| checkNotNull(priceQuote, "priceQuote must not be null"); |
There was a problem hiding this comment.
The public limit conversion currently checks only for nulls. PriceQuote.toBaseSideMonetaryExact validates the monetary class rather than the currency code, so, for example, a BTC/GBP quote can be accepted for a BTC/EUR market and produce a numerically valid but incorrect limit. This relies on every caller keeping rates, market, and priceQuote coherent, which is fragile for code used to enforce create/take amount caps.
Should we validate that priceQuote.getMarket() matches market and that the fiat rate, when present, belongs to the same market in both conversion methods?
| .flatMap(stringValue -> { | ||
| try { | ||
| return Optional.of(Long.parseLong(stringValue)); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
The newly added long-cookie API currently has no call sites. It also extends the existing broad Throwable catch pattern, which can suppress VM or linkage failures even though malformed Long.parseLong input only requires handling NumberFormatException.
Should we remove these unused overloads for now, or, if they are intentionally retained, narrow both new catches to NumberFormatException?
|
Confirmed. Fixed in: 4683bc5. |
3c9d791 to
024aebe
Compare
Restructure offer creation and taking around per-concern domain components. Market, direction, price, amount and payment method each get a selection model and service, orchestrated by a DraftOfferUseCase base with explicit lifecycle scopes. Controllers become thin bindings onto the domain observables. The old create and take offer UI moves wholesale under the draft packages; navigation targets stay unchanged. Specifications for the create offer use case and the input validation policy are added alongside the code.
Add the take offer specification covering initialization validation, payment method eligibility, price handling, amount limits, review and handoff. Fix the foundations the take offer work builds on: recompute all range-derived slider values when the amount range changes so no stale clamp fractions survive a payment method switch, port the rounded-fiat range-versus-fixed decision into the amount spec factory, and rebuild the trade limit info endpoint on the new limits domain. Remove a test orphaned by the draft refactor.
Fill in the gutted TakeOfferUseCase and its per-concern services. Initialization validates the offer at the trust boundary (protocol type, own offer across all local identities, market price availability, price bounds, spec and option shape, collateral symmetry). Payment method selection enforces account eligibility including compatibility data, with the incompatibility explained in the no-account prompt. Price handling resolves a single quote snapshot, warns on configurable deviation and revalidates on market updates. Amount selection keeps the stored denomination authoritative, intersects offer, absolute and payment method limits, applies the buyer cap and blocks confirmation while limits are stale or amounts invalid. Review displays a deferred fee, guards resubmission while in flight, supports taking without a mediator after explicit consent, and hands off a single atomic amount-price snapshot to the protocol. All amount and price writers are serialized, callbacks are guarded against stale attempts and wizard close, conversions fail closed on overflow, and fixed-price quotes are verified at deserialization. The wizard keeps its step index valid on step removal and persists the amount input side.
Separate price data by origin so market ticks no longer overwrite the offer price, clear amounts on market switches, and recompute amounts on input side switches without feedback loops. Compute amount limits during initialization, degrade unpriced markets gracefully and gate the review step on a complete draft. Harden the guards: amount mutators require available limits instead of a latched initialized flag, market based conversions require positive quotes, the rates cache invalidates on market changes, and the resolved quote overloads verify the quote matches the market. Add UI harness coverage for the price step and clean up dead code left by the refactor.
a9f3ea1 to
facd241
Compare
Completes the MuSig create/take-offer rewrite started by @HenrikJannsen (his branch
Rebased-Apply-create-offer-workflow-to-UI-3), which I continued under #4883. Create-offer is Henrik's work; I added the take-offer use case and per-concern services, the two review slices, and the spec.What's here
TakeOfferUseCase+ per-concern take-offer services: init/validation, payment + eligibility, price, amount + limits, review + handoff, controller wiring.docs/specifications/offer/mu-sig/take-offer.md.mainvia one merge commit (main's fixes since the branch — PriceQuote overflow, offer-list validity, amount-limit policy move — are preserved; the create-offer area is superseded by the rewrite). Not rebased, to keep Henrik's authorship intact — squash or rebase on merge if you prefer linear history.Tests: offer, bisq-easy, trade, api, mu-sig and desktop suites green.
Deferred (product decisions, not blockers): the take-offer review shows
N/Afor the trade fee until the review and the protocol share one fee source — pending the fee-policy call. Tracking + open items in #4883.Part of #4883.