[bisq-network/bisq] Add cancel trade feature (#4514)
sqrrm
notifications at github.com
Wed Sep 16 12:35:53 UTC 2020
@sqrrm requested changes on this pull request.
During testing I get the accept/reject/decide later popup twice if the trade is not open when receiving the request. First I Accept/Reject and the same popup shows again and I must choose again. Perhaps a listener is attached when switching to that view or something like that.
The buyer should not be able to continue with the trade after the rejection with the current protocol since he's signing the payout with all going to seller. Better might be:
Accept path
1. Buyer proposes cancellation
1. Seller accepts, signs payout and sends signature back
1. Buyer finalizes cancellation
Reject path
1. Buyer proposes cancellation
1. Seller rejects
1. Buyer can continue with trade. No risk to buyer since the seller doesn't have the signed payouttx. No risk to seller since the potential cancellation tx would send all funds to seller.
Adding signedwitness to last message in protocol makes a lot of sense.
The handling of mailboxmessages wasn't obivous to me. Hence the failure to remove them from the network. Would it make sense to make a separate service to pop mailboxmessages from the p2p network store them locally to be processed by the trade protocol later. That way there is no need to worry about the post trade tast mailbox handling.
Added some minor comments inline.
> @@ -833,6 +832,67 @@ public Transaction sellerSignsAndFinalizesPayoutTx(Transaction depositTx,
return payoutTx;
}
+ ///////////////////////////////////////////////////////////////////////////////////////////
+ // Canceled trade payoutTx
+ ///////////////////////////////////////////////////////////////////////////////////////////
+
+ public byte[] signCanceledTradePayoutTx(Transaction depositTx,
+ Coin buyerPayoutAmount,
+ Coin sellerPayoutAmount,
+ String buyerPayoutAddressString,
+ String sellerPayoutAddressString,
+ DeterministicKey myMultiSigKeyPair,
+ byte[] buyerPubKey,
+ byte[] sellerPubKey)
+ throws AddressFormatException, TransactionVerificationException {
+ Transaction preparedPayoutTx = createPayoutTx(depositTx, buyerPayoutAmount, sellerPayoutAmount, buyerPayoutAddressString, sellerPayoutAddressString);
Long line is long
> + Transaction preparedPayoutTx = createPayoutTx(depositTx, buyerPayoutAmount, sellerPayoutAmount, buyerPayoutAddressString, sellerPayoutAddressString);
+ // MS redeemScript
+ Script redeemScript = get2of2MultiSigRedeemScript(buyerPubKey, sellerPubKey);
+ // MS output from prev. tx is index 0
+ Sha256Hash sigHash = preparedPayoutTx.hashForSignature(0, redeemScript, Transaction.SigHash.ALL, false);
+ checkNotNull(myMultiSigKeyPair, "myMultiSigKeyPair must not be null");
+ if (myMultiSigKeyPair.isEncrypted()) {
+ checkNotNull(aesKey);
+ }
+ ECKey.ECDSASignature mySignature = myMultiSigKeyPair.sign(sigHash, aesKey).toCanonicalised();
+ WalletService.printTx("prepared canceled trade payoutTx for sig creation", preparedPayoutTx);
+ WalletService.verifyTransaction(preparedPayoutTx);
+ return mySignature.encodeToDER();
+ }
+
+ public Transaction finalizeCanceledTradePayoutTx(Transaction depositTx,
I feel it would be easier and clearer to make more generic sign and finalize methods for the multisig payout. This copy paste of the signing code makes it look like there's something special about this case, but it's just a normal payout and the cause for how the distribution was decided isn't really important.
Just one `finalizeTradePayoutTx` and one `signTradePayoutTx` should be quite enough, reduce code duplication and reduce confusion.
> + Transaction depositTx = checkNotNull(trade.getDepositTx());
+ String tradeId = trade.getId();
+ TradingPeer tradingPeer = processModel.getTradingPeer();
+ BtcWalletService walletService = processModel.getBtcWalletService();
+ Offer offer = checkNotNull(trade.getOffer(), "offer must not be null");
+ Coin tradeAmount = checkNotNull(trade.getTradeAmount(), "tradeAmount must not be null");
+ Contract contract = checkNotNull(trade.getContract(), "contract must not be null");
+
+ checkNotNull(trade.getTradeAmount(), "trade.getTradeAmount() must not be null");
+
+ byte[] mySignature = checkNotNull(processModel.getCanceledTradePayoutTxSignature(),
+ "processModel.getTxSignatureFromCanceledTrade must not be null");
+ byte[] peersSignature = checkNotNull(tradingPeer.getCanceledTradePayoutTxSignature(),
+ "tradingPeer.getTxSignatureFromCanceledTrade must not be null");
+
+ boolean isMyRoleBuyer = contract.isMyRoleBuyer(processModel.getPubKeyRing());
This is only used by sellercanceltradeprotocol, seems excessive to do these checks.
> @@ -909,6 +910,30 @@ portfolio.failed.cantUnfail=This trade cannot be moved back to open trades at th
portfolio.failed.depositTxNull=The trade cannot be completed. Deposit transaction is null
portfolio.failed.delayedPayoutTxNull=The trade cannot be completed. Delayed payout transaction is null
+portfolio.pending.cancelTrade=Cancel trade
+portfolio.pending.requestSent=Cancel trade request message sent...
+portfolio.pending.requestArrived=Cancel trade request message arrived
+portfolio.pending.requestInMailbox=Cancel trade request message stored in mailbox
+portfolio.pending.requestFailed=Sending cancel trade request message failed
+portfolio.pending.requestGotRejected=Your cancel trade request was rejected by the peer
+portfolio.pending.doNotDecideYet=Don't decide yet
+portfolio.pending.requestCancelTradePopup=You can send a request to your trading peer for canceling the trade.\n\n\
Would be good to get @m52go to review this text
> + }
+
+ private void rejectCancelTradeRequest() {
+ manager.rejectCancelTradeRequest(trade,
+ () -> {
+ }, errorMessage -> {
+ });
+ }
+
+
+ private void onCanceledTradeStateChanged(CanceledTradeState newValue) {
+ log.error("onCanceledTradeStateChanged {} {}", newValue, trade.getId());
+ if (newValue == null) {
+ return;
+ }
+ switch (newValue) {
Why such a switch when only one case is used. Could just check that one case with an `if` and avoid 30 lines of code.
> + string trade_id = 3;
+ bytes tx_signature = 2;
```suggestion
string trade_id = 2;
bytes tx_signature = 3;
```
> @@ -107,4 +107,15 @@ public Coin getPayoutAmount() {
return getOffer().getBuyerSecurityDeposit().add(getTradeAmount());
}
+
+ @Override
+ public boolean wasCanceledTrade() {
+ switch (buyersCancelTradeState) {
This is how I like switch cases, default for anything not specifically tested
> @@ -821,6 +821,8 @@ public void onComplete() {
public abstract Coin getPayoutAmount();
+ public abstract boolean wasCanceledTrade();
I would prefer `isCanceled()`. It's known that it's a `Trade` already and we use prefix `is` mostly everywhere
> @@ -90,7 +90,13 @@ protected void run() {
signedWitness = signedWitnessOptional.get();
- // Message sending is handled in base class.
+ // The signer broadcasts as well the signedWitness. In case the receiver will not get the message we have
+ // better resilience that the data is in the network. We set reBroadcast to true but do not expect that the
+ // signedWitness exists already as we just created it.
+ // TODO check with @sqrrm
+ processModel.getP2PService().addPersistableNetworkPayload(signedWitness, true);
This seems excessive as this is done during signing already.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4514#pullrequestreview-487226706
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200916/31be94e7/attachment.html>
More information about the bisq-github
mailing list