[bisq-network/bisq] Trade protocol domain improvements (#4566)
sqrrm
notifications at github.com
Mon Sep 28 16:33:49 UTC 2020
@sqrrm requested changes on this pull request.
I like the changes to the protocol, easier to read and better contained like this.
Added a few comments that would be good to tend to. I haven't looked at the desktop changes at all yet.
> + Stream<AddressEntry> availableAndPayout = Stream.concat(getAddressEntries(AddressEntry.Context.TRADE_PAYOUT)
+ .stream(), getFundedAvailableAddressEntries().stream());
+ Stream<AddressEntry> available = Stream.concat(availableAndPayout,
+ getAddressEntries(AddressEntry.Context.ARBITRATOR).stream());
Why create multiple streams here, they are concatenated into one in the end anyway.
Naming is also suspect, AVAILABLE is a keyword for a certain type of address, perhaps spendable would be better?
Something like
```
var spendable = Stream.concat...
spendable = Stream.concat(spendable, ...
...
spendable.filter...
```
> case INPUTS_FOR_DEPOSIT_TX_REQUEST:
- return InputsForDepositTxRequest.fromProto(proto.getInputsForDepositTxRequest(), this, messageVersion);
+ return TakeOfferRequest.fromProto(proto.getInputsForDepositTxRequest(), this, messageVersion);
Would be nice to rename the protobuf class to `TakeOfferRequest` as well to conform with the rest of the code, this looks confusing.
> 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);
I'm not following why this is done. A comment would likely clarify.
> @@ -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
Why change the copyright notice?
> + }
+
+ // 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) {
Might want to return early to avoid large blocks
> + 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) {
Could return early for non null taskRunner...
> @@ -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
Changed copyright text
> + 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)
Should `DEPOSIT_PUBLISHED` really be expected? I thought deposit would only be published after this message was acked.
> +
+ // 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 " +
This log is quite odd, `We with a...`
> + })))
+ .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();
+
```suggestion
```
> +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;
+
+ at Slf4j
+public class DisputeProtocol extends TradeProtocol {
+
+ enum DisputeEvent implements FluentProtocol.Event {
+ MEDIATION_RESULT_ACCEPTED,
+ MEDIATION_RESULT_REJECTED,
+ ARBITRATION_REQUESTED
`ARBITRATION` generally refers to legacy arbitration in code. `REFUND` is what's been used everywhere for refund cases.
> + 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);
+ }
```suggestion
onTradeMessage(message, peer);
```
> + protected abstract TradeMessage getMessage(String id);
+
+ protected abstract void setStateSent();
+
+ protected abstract void setStateArrived();
+
+ protected abstract void setStateStoredInMailbox();
+
+ protected abstract void setStateFault();
Could default these to {}
> @@ -305,59 +266,118 @@ public void setTakeOfferFeeTx(Transaction takeOfferFeeTx) {
public PaymentAccountPayload getPaymentAccountPayload(Trade trade) {
This is not new in this PR, but would be better named `getMyPaymentAccountPayload`
>
- // 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
```suggestion
// message but that cannot be changed due backward compatibility issues. It is a left over from the
```
>
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.
```suggestion
* buyer does not have the delayed payout tx and would not be able to open arbitration.
```
> + private static final int MAX_RESEND_ATTEMPTS = 7;
+ private int delayInSec = 4;
This is about 4 minutes until fail, I think that's too long, 5 resend attempts would be better in my opinion
> @Slf4j
-public class SellerSendsDepositTxAndDelayedPayoutTxMessage extends TradeTask {
- @SuppressWarnings({"unused"})
- public SellerSendsDepositTxAndDelayedPayoutTxMessage(TaskRunner taskHandler, Trade trade) {
+public class SellerSendsDepositTxAndDelayedPayoutTxMessage extends SendMailboxMessageTask {
This resend type class has a lot of duplicate code from `BuyerSendCounterCurrencyTransferStartedMessage`, could probably use a common base class, or helper class.
--
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/4566#pullrequestreview-497359483
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200928/8e9a3286/attachment-0001.html>
More information about the bisq-github
mailing list