Refactor trade model & pass contract-forming txids to RPC client#125
Merged
ChrisSon15 merged 6 commits intobisq-network:mainfrom Mar 5, 2026
Merged
Conversation
Add separate '(Deposit|Swap|Warning|Redirect|Claim)Tx' structs to group related tx-builder and 'SigCtx' fields of 'rpc::protocol::TradeModel', forming a nested structure, instead of using a flat list of fields. This hopefully makes 'TradeModel' a bit more manageable (and may promote code reuse).
Group the six new buyer & seller Warning/Redirect/Claim Tx fields of
'TradeModel' into two separate '(buyer|seller)_txs' fields, creating a
new 'ArbitrationTxs' struct (with its own 'warning', 'redirect' and
'claim' fields) for this purpose. This allows many of the 'TradeModel'
method bodies to be deduplicated, by merging separate branches for the
buyer and seller's prepared txs.
Also shorten the 'rpc::protocol::{ExchangedAddresses, ExchangedNonces}'
struct fields by (partially) removing common suffices, as was suggested
by Clippy, for consistency with the new 'ArbitrationTxs' struct.
Provide a 'ContractualTxIds' proto to return the Deposit, Redirect and Warning txids as an extra field of 'musigrpc.PartialSignaturesMessage', prior to calling 'SignDepositTx' during trade setup. This avoids the client having to decode the merged Deposit PSBT in order to learn its ID. It also allows both clients to include all four Redirect & Warning txids in any signed and cross-checked trade contract, so that there can be no dispute over whose Warning Tx was used, in the event that the trade goes into arbitration. Also add a corresponding 'ContractualTxids' struct and (optional) field to 'rpc::protocol::ExchangedSigs' (cased slightly differently to better conform to Rust-Bitcoin's naming conventions), to hand the five txids to the 'Musig' service. Leave the new field empty and ignore any txids passed back by the client when receiving partial signatures/sighashes in the subsequent 'SignDepositTx' RPC call, however.
Add a 'txid' property to '(Deposit|Warning|Redirect)TxBuilder', with a corresponding cached field in the case of the Warning & Redirect Tx (but pulling it out of the buyer payout 'TxOutput' struct in the case of the Deposit Tx), populated at the same time the unsigned tx is computed. (Don't bother caching the Swap & Claim txids as they're not yet needed.) (Also make the builder errors a bit more consistent, by always returning a 'TransactionErrorKind::MissingTransaction' error when the unsigned tx isn't yet known and any downstream objects are requested.) Use the cached txids to avoid repeated direct & indirect 'compute_txid' calls in 'protocol::protocol_musig_adaptor' and 'rpc::protocol'.
Add a new 'KeyCtxs' struct, to group the '(buyer|seller)_output_key_ctx' fields of 'TradeModel' into a single field. This is to bring the methods acting only on those fields closer together, shortening the 'TradeModel' impl block a little. It also makes it easier to pass around both payout 'KeyCtx' objects together, to help any future refactorings.
Further simplify 'rpc::protocol::TradeModel' by removing the redundant
'prepared_tx_fee_rate' & 'redirection_receivers' fields, instead pulling
the data from the Swap & buyer's Redirect Tx builders, respectively (as
all the relevant builders are populated with copies of each of those two
trade params at the same time).
Also simplify and improve the 'TradeModel::check_redirect_tx_params'
error handling, by returning Result's instead of Option's for various
methods in the "protocol::{receiver, transaction}" and 'rpc::protocol'
modules, wrapping Option-returning short-circuiting chains of checked
arithmetic as a lambda and mapping None to an overflow error, where
appropriate. (This is in lieu of try-blocks being stabilised in Rust.)
ChrisSon15
reviewed
Mar 4, 2026
Collaborator
ChrisSon15
left a comment
ChrisSon15
left a comment
There was a problem hiding this comment.
one question about the PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Group the fields of
rpc::protocol::TradeModelinto a nested structure, instead a flat list, as it was starting to become a bit unwieldy. Also do some fairly extensive refactoring of theTradeModelmethods (without changing its API significantly at this point), as the grouping of the fields enables a reasonable amount of code branch deduplication. Slightly improve some of the error handling and make some minor optimisations, as well.Additionally, modify
rpc.prototo return the arbitration-relevant (Deposit, Warning and Redirect) txids together as an extra field ofmusigrpc.PartialSignaturesMessage, just prior to the Deposit Tx being signed by each party. This provides the clients with the missing Deposit txid directly, which they would otherwise have to derive from the exchanged PSBTs. It also allows them to incorporate the Warning & Redirect txids into any cross-checked and individually signed trade contract, before committing by signing the Deposit Tx, so that there can be no dispute over whose Warning Tx was used in the event of arbitration.