<p></p>
<p><b>@sqrrm</b> requested changes on this pull request.</p>
<p>I like the changes to the protocol, easier to read and better contained like this.</p>
<p>Added a few comments that would be good to tend to. I haven't looked at the desktop changes at all yet.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495822500">core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java</a>:</p>
<pre style='color:#555'>> + Stream<AddressEntry> availableAndPayout = Stream.concat(getAddressEntries(AddressEntry.Context.TRADE_PAYOUT)
+ .stream(), getFundedAvailableAddressEntries().stream());
+ Stream<AddressEntry> available = Stream.concat(availableAndPayout,
+ getAddressEntries(AddressEntry.Context.ARBITRATOR).stream());
</pre>
<p>Why create multiple streams here, they are concatenated into one in the end anyway.</p>
<p>Naming is also suspect, AVAILABLE is a keyword for a certain type of address, perhaps spendable would be better?</p>
<p>Something like</p>
<pre><code>var spendable = Stream.concat...
spendable = Stream.concat(spendable, ...
...
spendable.filter...
</code></pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495825646">core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java</a>:</p>
<pre style='color:#555'>> case INPUTS_FOR_DEPOSIT_TX_REQUEST:
- return InputsForDepositTxRequest.fromProto(proto.getInputsForDepositTxRequest(), this, messageVersion);
+ return TakeOfferRequest.fromProto(proto.getInputsForDepositTxRequest(), this, messageVersion);
</pre>
<p>Would be nice to rename the protobuf class to <code>TakeOfferRequest</code> as well to conform with the rest of the code, this looks confusing.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495856345">core/src/main/java/bisq/core/trade/TradeManager.java</a>:</p>
<pre style='color:#555'>> onTradesChanged();
- getAddressEntriesForAvailableBalanceStream()
+ btcWalletService.getAddressEntriesForAvailableBalanceStream()
.filter(addressEntry -> addressEntry.getOfferId() != null)
.forEach(addressEntry -> {
log.warn("Swapping pending OFFER_FUNDING entries at startup. offerId={}", addressEntry.getOfferId());
btcWalletService.swapTradeEntryToAvailableEntry(addressEntry.getOfferId(), AddressEntry.Context.OFFER_FUNDING);
</pre>
<p>I'm not following why this is done. A comment would likely clarify.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495902634">core/src/main/java/bisq/core/trade/protocol/BuyerAsMakerProtocol.java</a>:</p>
<pre style='color:#555'>> @@ -11,7 +11,7 @@
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
* License for more details.
*
- * You should have received a copy of the GNU Affero General Public License
+ * You should have with a copy of the GNU Affero General Public License
</pre>
<p>Why change the copyright notice?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495952660">core/src/main/java/bisq/core/trade/protocol/FluentProtocol.java</a>:</p>
<pre style='color:#555'>> + }
+
+ // Can be used before or after executeTasks
+ public FluentProtocol run(Runnable runnable) {
+ Condition.Result result = condition.getResult();
+ if (result.isValid) {
+ runnable.run();
+ } else if (resultHandler != null) {
+ resultHandler.accept(result);
+ }
+ return this;
+ }
+
+ public FluentProtocol executeTasks() {
+ Condition.Result result = condition.getResult();
+ if (result.isValid) {
</pre>
<p>Might want to return early to avoid large blocks</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495960627">core/src/main/java/bisq/core/trade/protocol/FluentProtocol.java</a>:</p>
<pre style='color:#555'>> + this.tasks = tasks;
+ return this;
+ }
+
+ public Setup withTimeout(int timeoutSec) {
+ this.timeoutSec = timeoutSec;
+ return this;
+ }
+
+ public Setup using(TradeTaskRunner taskRunner) {
+ this.taskRunner = taskRunner;
+ return this;
+ }
+
+ public TradeTaskRunner getTaskRunner(@Nullable TradeMessage message, @Nullable Event event) {
+ if (taskRunner == null) {
</pre>
<p>Could return early for non null taskRunner...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495966800">core/src/main/java/bisq/core/trade/protocol/BuyerAsTakerProtocol.java</a>:</p>
<pre style='color:#555'>> @@ -11,7 +11,7 @@
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
* License for more details.
*
- * You should have received a copy of the GNU Affero General Public License
+ * You should have with a copy of the GNU Affero General Public License
</pre>
<p>Changed copyright text</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495990587">core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java</a>:</p>
<pre style='color:#555'>> + handle((PayoutTxPublishedMessage) message, peer);
+ }
+ }
+
+ protected abstract void handle(DelayedPayoutTxSignatureRequest message, NodeAddress peer);
+
+ // The DepositTxAndDelayedPayoutTxMessage is a mailbox message as earlier we use only the deposit tx which can
+ // be also with from the network once published.
+ // Now we send the delayed payout tx as well and with that this message is mandatory for continuing the protocol.
+ // We do not support mailbox message handling during the take offer process as it is expected that both peers
+ // are online.
+ // For backward compatibility and extra resilience we still keep DepositTxAndDelayedPayoutTxMessage as a
+ // mailbox message but the stored in mailbox case is not expected and the seller would try to send the message again
+ // in the hope to reach the buyer directly.
+ protected void handle(DepositTxAndDelayedPayoutTxMessage message, NodeAddress peer) {
+ expect(anyPhase(Trade.Phase.TAKER_FEE_PUBLISHED, Trade.Phase.DEPOSIT_PUBLISHED)
</pre>
<p>Should <code>DEPOSIT_PUBLISHED</code> really be expected? I thought deposit would only be published after this message was acked.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r495991120">core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java</a>:</p>
<pre style='color:#555'>> +
+ // The DepositTxAndDelayedPayoutTxMessage is a mailbox message as earlier we use only the deposit tx which can
+ // be also with from the network once published.
+ // Now we send the delayed payout tx as well and with that this message is mandatory for continuing the protocol.
+ // We do not support mailbox message handling during the take offer process as it is expected that both peers
+ // are online.
+ // For backward compatibility and extra resilience we still keep DepositTxAndDelayedPayoutTxMessage as a
+ // mailbox message but the stored in mailbox case is not expected and the seller would try to send the message again
+ // in the hope to reach the buyer directly.
+ protected void handle(DepositTxAndDelayedPayoutTxMessage message, NodeAddress peer) {
+ expect(anyPhase(Trade.Phase.TAKER_FEE_PUBLISHED, Trade.Phase.DEPOSIT_PUBLISHED)
+ .with(message)
+ .from(peer)
+ .preCondition(trade.getDepositTx() == null || trade.getDelayedPayoutTx() == null,
+ () -> {
+ log.warn("We with a DepositTxAndDelayedPayoutTxMessage but we have already processed the deposit and " +
</pre>
<p>This log is quite odd, <code>We with a...</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496004773">core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java</a>:</p>
<pre style='color:#555'>> + })))
+ .run(() -> trade.setState(Trade.State.BUYER_CONFIRMED_IN_UI_FIAT_PAYMENT_INITIATED))
+ .executeTasks();
+ }
+
+ ///////////////////////////////////////////////////////////////////////////////////////////
+ // Incoming message Payout tx
+ ///////////////////////////////////////////////////////////////////////////////////////////
+
+ protected void handle(PayoutTxPublishedMessage message, NodeAddress peer) {
+ expect(anyPhase(Trade.Phase.FIAT_SENT, Trade.Phase.PAYOUT_PUBLISHED)
+ .with(message)
+ .from(peer))
+ .setup(tasks(BuyerProcessPayoutTxPublishedMessage.class))
+ .executeTasks();
+
</pre>
⬇️ Suggested change
<pre style="color: #555">-
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496007891">core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java</a>:</p>
<pre style='color:#555'>> +import bisq.core.trade.protocol.tasks.mediation.SignMediatedPayoutTx;
+
+import bisq.network.p2p.NodeAddress;
+
+import bisq.common.handlers.ErrorMessageHandler;
+import bisq.common.handlers.ResultHandler;
+
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DisputeProtocol extends TradeProtocol {
+
+ enum DisputeEvent implements FluentProtocol.Event {
+ MEDIATION_RESULT_ACCEPTED,
+ MEDIATION_RESULT_REJECTED,
+ ARBITRATION_REQUESTED
</pre>
<p><code>ARBITRATION</code> generally refers to legacy arbitration in code. <code>REFUND</code> is what's been used everywhere for refund cases.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496018211">core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java</a>:</p>
<pre style='color:#555'>> + if (message instanceof MediatedPayoutTxSignatureMessage) {
+ handle((MediatedPayoutTxSignatureMessage) message, peer);
+ } else if (message instanceof MediatedPayoutTxPublishedMessage) {
+ handle((MediatedPayoutTxPublishedMessage) message, peer);
+ } else if (message instanceof PeerPublishedDelayedPayoutTxMessage) {
+ handle((PeerPublishedDelayedPayoutTxMessage) message, peer);
+ }
</pre>
⬇️ Suggested change
<pre style="color: #555">- if (message instanceof MediatedPayoutTxSignatureMessage) {
- handle((MediatedPayoutTxSignatureMessage) message, peer);
- } else if (message instanceof MediatedPayoutTxPublishedMessage) {
- handle((MediatedPayoutTxPublishedMessage) message, peer);
- } else if (message instanceof PeerPublishedDelayedPayoutTxMessage) {
- handle((PeerPublishedDelayedPayoutTxMessage) message, peer);
- }
+ onTradeMessage(message, peer);
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496054996">core/src/main/java/bisq/core/trade/protocol/tasks/SendMailboxMessageTask.java</a>:</p>
<pre style='color:#555'>> + protected abstract TradeMessage getMessage(String id);
+
+ protected abstract void setStateSent();
+
+ protected abstract void setStateArrived();
+
+ protected abstract void setStateStoredInMailbox();
+
+ protected abstract void setStateFault();
</pre>
<p>Could default these to {}</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496067547">core/src/main/java/bisq/core/trade/protocol/ProcessModel.java</a>:</p>
<pre style='color:#555'>> @@ -305,59 +266,118 @@ public void setTakeOfferFeeTx(Transaction takeOfferFeeTx) {
public PaymentAccountPayload getPaymentAccountPayload(Trade trade) {
</pre>
<p>This is not new in this PR, but would be better named <code>getMyPaymentAccountPayload</code></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496068073">core/src/main/java/bisq/core/trade/protocol/tasks/maker/MakerProcessesInputsForDepositTxRequest.java</a>:</p>
<pre style='color:#555'>>
- // Taker has to sign offerId (he cannot manipulate that - so we avoid to have a challenge protocol for passing the nonce we want to get signed)
+ // We set the taker fee only in the processModel yet not in the trade as the tx was only created but not
+ // published yet. Once it was published we move it to trade. The takerFeeTx should be sent in a later
+ // message bu that cannot be changed due backward compatibility issues. It is a left over from the
</pre>
⬇️ Suggested change
<pre style="color: #555">- // message bu that cannot be changed due backward compatibility issues. It is a left over from the
+ // message but that cannot be changed due backward compatibility issues. It is a left over from the
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496073838">core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSendsDepositTxAndDelayedPayoutTxMessage.java</a>:</p>
<pre style='color:#555'>>
import lombok.extern.slf4j.Slf4j;
import static com.google.common.base.Preconditions.checkNotNull;
+/**
+ * We send the buyer the deposit and delayed payout tx. We wait to receive a ACK message back and resend the message
+ * in case that does not happen in 4 seconds or if the message was stored in mailbox or failed. We keep repeating that
+ * with doubling the interval each time and until the MAX_RESEND_ATTEMPTS is reached. If never successful we fail and
+ * do not continue the protocol with publishing the deposit tx. That way we avoid that a deposit tx is published but the
+ * buyer does not has the delayed payout tx and would not be able to open arbitration.
</pre>
⬇️ Suggested change
<pre style="color: #555">- * buyer does not has the delayed payout tx and would not be able to open arbitration.
+ * buyer does not have the delayed payout tx and would not be able to open arbitration.
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496075930">core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSendsDepositTxAndDelayedPayoutTxMessage.java</a>:</p>
<pre style='color:#555'>> + private static final int MAX_RESEND_ATTEMPTS = 7;
+ private int delayInSec = 4;
</pre>
<p>This is about 4 minutes until fail, I think that's too long, 5 resend attempts would be better in my opinion</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4566#discussion_r496077030">core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSendsDepositTxAndDelayedPayoutTxMessage.java</a>:</p>
<pre style='color:#555'>> @Slf4j
-public class SellerSendsDepositTxAndDelayedPayoutTxMessage extends TradeTask {
- @SuppressWarnings({"unused"})
- public SellerSendsDepositTxAndDelayedPayoutTxMessage(TaskRunner taskHandler, Trade trade) {
+public class SellerSendsDepositTxAndDelayedPayoutTxMessage extends SendMailboxMessageTask {
</pre>
<p>This resend type class has a lot of duplicate code from <code>BuyerSendCounterCurrencyTransferStartedMessage</code>, could probably use a common base class, or helper class.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4566#pullrequestreview-497359483">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNU3U45V7F3SM5O4R4TSIC3G3ANCNFSM4R3L7WRA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNROXJCAYVKQ5TENNSTSIC3G3A5CNFSM4R3L7WRKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODWSRU6Y.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4566#pullrequestreview-497359483",
"url": "https://github.com/bisq-network/bisq/pull/4566#pullrequestreview-497359483",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>