[bisq-network/bisq] Fix issue of Trade Step 1 validation done too soon (#5746)

[jmacxx]

This PR fixes an issue where some trade data validation was performed too soon, before the transaction has been applied to the trade, resulting in an erroneous popup informing the user "There is an issue with a missing or invalid transaction".

Resolves: #5494.

---

The trade data validation is done at transition to `Trade Step 1`, it does some basic [sanity checking of the deposit transaction](https://github.com/bisq-network/bisq/blob/2e38c3cf3525ff4275f26b755c9eea88b322af9d/core/src/main/java/bisq/core/trade/TradeDataValidation.java#L358-L377): that it is present, that it has 2 inputs, etc.  Unfortunately there is a timing issue whereby Trade Step 1 is sometimes reached before the transaction has been applied to the trade.

The trade currently transitions to Trade Step 1 when [the deposit transaction is exchanged P2P](https://github.com/bisq-network/bisq/blob/2e38c3cf3525ff4275f26b755c9eea88b322af9d/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSendsDepositTxAndDelayedPayoutTxMessage.java#L82-L104)
 in task `SellerSendsDepositTxAndDelayedPayoutTxMessage`.  

After broadcasting, the deposit transaction is [stored on the Trade](https://github.com/bisq-network/bisq/blob/2e38c3cf3525ff4275f26b755c9eea88b322af9d/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerPublishesDepositTx.java#L50-L52) via `applyDepositTx()`.  This can be up to 5 seconds later.

Within that window of time, the trade shows with red "failed" iconography and the validation done at Trade Step 1 fails because of missing information in the Trade.




The goal of this PR is to make Trade Step 1 compatible with the assumption that the trade is initialized enough to check.  It makes a simple change moving the Trade Step 1 milestone further along the trade init path, after the broadcast, when the deposit tx has been applied to the trade.  This way we can be safe in the assumption that the trade is initialized enough to perform our validation.

---

**Other considerations:**

There are a number of other ways this could be addressed, such that the deposit tx is applied to the trade early enough in initialization.

1. Task SellerPublishesDepositTx could be invoked before SellerSendsDepositTxAndDelayedPayoutTxMessage.  
2. `applyDepositTx()` to the trade in SellerAsMakerFinalizesDepositTx.
 

Currently, I have picked the least intrusive change to fix this particular bug, although testing showed that these other two options also would work.  Upon reflection. I notice the chosen solution is slightly incongruent with the the [ordering of Trade.State](https://github.com/bisq-network/bisq/blob/2e38c3cf3525ff4275f26b755c9eea88b322af9d/core/src/main/java/bisq/core/trade/Trade.java#L113-L131) and the concept of trade Phase.  Perhaps it would be better to consider the options above, of applying deposit Tx to Trade earlier.

An added benefit of applying the deposit transaction earlier would be that the trade would no longer erroneously flash the failed icons prior to broadcast completing (which currently has the effect of causing undue alarm).
![failedtrade2](https://user-images.githubusercontent.com/47253594/136661869-b0466203-bb6f-4f4e-aab6-e3c65176d0f9.png)

So in fact I'd prefer to have solution (2) above; it is cleaner although it might carry more change risk.
You can view, comment on, or merge this pull request online at:

  https://github.com/bisq-network/bisq/pull/5746

-- Commit Summary --

  * <a href="https://github.com/bisq-network/bisq/pull/5746/commits/4b74bd82ba48a0722cbe11f7f8a4975d0eb2c4b7">Fix issue of Trade Step 1 validation done too soon</a>

-- File Changes --

    M desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesViewModel.java (14)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/5746.patch
https://github.com/bisq-network/bisq/pull/5746.diff

-- 
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/5746
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20211009/00b46c10/attachment-0001.htm>