[bisq-network/bisq] Autoconfirm XMR trade if tx check was validated by proof service (#4421)

chimp1984 notifications at github.com
Sun Aug 23 04:49:06 UTC 2020


@chimp1984 requested changes on this pull request.



> @@ -57,6 +60,19 @@ public AddressValidationResult validate(String address) {
             return AddressValidationResult.invalidStructure();
         }
     }
+
+    // obtain the raw hex version of a monero address
+    public static String addressToHex(String address) {

I would suggest to name it more specifically. As we remove type and checksum it is not really converting but we use it for verifying if it is a valid XMR address. I think the hex check might be good enough for now but maybe there is a more exact validation available. If we rename the method to 'isAddressValid' and return a boolean I think it gets more clean. 

> @@ -109,6 +109,9 @@
     @Nullable
     private final List<String> btcFeeReceiverAddresses;
 
+    // added after v1.3.7
+    private final boolean disableAutoconf;

I would suggest to stick with camelCase -> `disableAutoConf`

> +        this.txHash = txHash;
+        this.txKey = txKey;
+        this.recipientAddress = recipientAddress;
+        this.amount = amount;
+        this.confirmsRequired = confirmsRequired;
+        this.serviceAddress = serviceAddress;
+    }
+
+    // something to uniquely identify this object by
+    public String getKey() {
+        return txHash + "|" + serviceAddress;
+    }
+
+    public XmrProofResult checkApiResponse(String jsonTxt) {
+        try {
+            final JsonObject json = new Gson().fromJson(jsonTxt, JsonObject.class);

We don't use `final` for local variables.

> +            final JsonObject jsonData = json.get("data").getAsJsonObject();
+            final String jsonStatus = json.get("status").getAsString();
+            if (jsonStatus.matches("fail")) {
+                // the API returns "fail" until the transaction has successfully reached the mempool.
+                // we return TX_NOT_FOUND which will cause a retry later
+                return new XmrProofResult(XmrProofResult.State.TX_NOT_FOUND, null);
+            } else if (!jsonStatus.matches("success")) {
+                return new XmrProofResult(XmrProofResult.State.API_FAILURE, "unhandled status value");
+            }
+
+            // validate that the address matches
+            final JsonElement jsonAddress = jsonData.get("address");
+            if (jsonAddress == null) {
+                return new XmrProofResult(XmrProofResult.State.API_INVALID, "missing address field");
+            } else {
+                String expectedAddressHex = CryptoNoteAddressValidator.addressToHex(this.recipientAddress);

Ah it seems you need the trimmed address for the comparision, if so ignore my comment earlier. Still might be good to have a validate method and a convert method to make it more explicit.

> @@ -110,6 +117,20 @@ public XmrProofResult checkApiResponse(String jsonTxt) {
                 }
             }
 
+            // validate that the txDate matches within tolerance
+            final JsonElement jsonTimestamp = jsonData.get("tx_timestamp");
+            if (jsonTimestamp == null) {
+                return new XmrProofResult(XmrProofResult.State.API_INVALID, "missing tx_timestamp field");
+            } else {
+                long tradeDateSeconds = tradeDate.getTime() / 1000;
+                long difference = abs(jsonTimestamp.getAsLong() - tradeDateSeconds);

I think the `abs` call here is leading to undesired behaviour. We only want to protect against old txs, so then the trade data  > tx_timestamp. If we allow both directions we limit the confirmation time to 1 day, which might be mostly ok for XMR but not sure... E.g. if tradeDateSeconds is today 18:00 and tx_timestamp is tomorrow 19:00 because miner fee was low and tx got included very late it would fail, but should be still valid.
I think correct impl would be:
`long difference = tradeDateSeconds - jsonTimestamp.getAsLong();
    if (difference > 60 * 60 * 24) {....
`
Not sure if jsonTimestamp is from confirmed block or from mempool, but should not make a difference.

> +    public enum State {
+        FEATURE_DISABLED,
+        TX_NOT_FOUND,
+        TX_NOT_CONFIRMED,
+        PROOF_OK,
+        CONNECTION_FAIL,
+        API_FAILURE,
+        API_INVALID,
+        TX_KEY_REUSED,
+        TX_HASH_INVALID,
+        TX_KEY_INVALID,
+        ADDRESS_INVALID,
+        AMOUNT_NOT_MATCHING;
+    }
+
+    @Getter

Annotating the class with @Value would make it more explicit that its a immutable class. @Getter annotations can be removed then as @Value included those.

> +    }
+
+    // alternate constructor for error scenarios
+    public XmrProofResult(State state, @Nullable String errorMsg) {
+        this.confirmCount = 0;
+        this.confirmsRequired = 0;
+        this.state = state;
+        if (isErrorState())
+            log.error(errorMsg != null ? errorMsg : state.toString());
+    }
+
+    public String getTextStatus() {
+        switch (state) {
+            case TX_NOT_CONFIRMED:
+                return Res.get("portfolio.pending.autoConfirmPending")
+                        + " " + Integer.toString(confirmCount)

Integer.toString is not needed.

>  import lombok.extern.slf4j.Slf4j;
 
 import org.jetbrains.annotations.NotNull;
 
 @Slf4j
- at Singleton
-class XmrTransferProofRequester {
+public class XmrTransferProofRequester {
 
     private final ListeningExecutorService executorService = Utilities.getListeningExecutorService(
             "XmrTransferProofService", 3, 5, 10 * 60);

Better use class name 'XmrTransferProofRequester'

>      ///////////////////////////////////////////////////////////////////////////////////////////
     // API
     ///////////////////////////////////////////////////////////////////////////////////////////
 
     public void request() {
-        // todo dev test address for a real tx proof
-        /*
-        txID: 5e665addf6d7c6300670e8a89564ed12b5c1a21c336408e2835668f9a6a0d802
-        txKey: f3ce66c9d395e5e460c8802b2c3c1fff04e508434f9738ee35558aac4678c906
-        address: 85q13WDADXE26W6h7cStpPMkn8tWpvWgHbpGWWttFEafGXyjsBTXxxyQms4UErouTY5sdKpYHVjQm6SagiCqytseDkzfgub
-        ammount : 8.90259736 XMR
-         */
-
+        if (terminated) {
+            // the XmrTransferProofService has asked us to terminate i.e. not make any further api calls
+            // this scenario may happen if a re-request is scheduled from the callback below
+            log.info("request() aborted, this object has been terminated: {}", httpClient.toString());
+            return;
+        }
         ListenableFuture<XmrProofResult> future = executorService.submit(() -> {
             Thread.currentThread().setName("XmrTransferProofRequest-" + this.toString());

 this.toString() would not result it any readable meaningful data (shows java internal id). Also if someone adds a toString mehtod we would get all that into the thread name rendering it unreadable...
Better use the tx hash + service url, that should be unique enough

>      private long firstRequest;
-    //todo dev settings
-    private long REPEAT_REQUEST_SEC = TimeUnit.SECONDS.toMillis(5);
+    // these settings are not likely to change and therefore not put into Config
+    private long REPEAT_REQUEST_PERIOD = TimeUnit.SECONDS.toMillis(30);

Is 30 seconds not a bit too aggressive for the nodes? Not sure whats the best value here. What are average block confirmation times on XMR? I guess some value in between 1-3 min. might be better.

>                  }
+                if (System.currentTimeMillis() - firstRequest > MAX_REQUEST_PERIOD) {
+                    log.warn("we have tried this service API for too long, giving up: {}", httpClient.toString());
+                    return;
+                }
+                if (result.isPendingState())

Better use alwqays curly brackets also for one-liners. We had some bugs related to that in the past and changed then code style (but still in the code from old days).

>                                Consumer<XmrProofResult> resultHandler,
                               FaultHandler faultHandler) {
-        this.httpClient = httpClient;
-        this.tradeDate = tradeDate;
-        this.txHash = txHash;
-        this.txKey = txKey;
-        this.recipientAddress = recipientAddress;
-        this.amount = amount;
+        this.httpClient = new XmrTxProofHttpClient(socks5ProxyProvider);
+        this.httpClient.setBaseUrl("http://" + xmrProofInfo.getServiceAddress());
+        if (xmrProofInfo.getServiceAddress().matches("^192.*")) {
+            log.info("ignoring Socks5 proxy for local net address: {}", xmrProofInfo.getServiceAddress());

I think there is another use case where we dont want to use the onion address. For regtest we usually dont use tor, so for dev testing its more convenient to use the clearnet IP of a service (at least one should provide clearnet access for that reason).

>          requester.request();
     }
 
-    private void cleanup(String tradeId) {
-        map.remove(tradeId);
+    public void terminateRequest(XmrProofInfo xmrProofInfo) {
+        final String key = xmrProofInfo.getKey();
+        XmrTransferProofRequester requester = map.getOrDefault(key, null);
+        if (requester != null) {
+            log.info("Terminating API request for {}", key);
+            requester.setTerminated(true);

I would find it better to call a method like stop() where the property is set instead of exposing the property itself. Small issue but makes API of the class more clear IMO.

> @@ -465,6 +475,7 @@ protected Trade(Offer offer,
         processModel = new ProcessModel();
         lastRefreshRequestDate = takeOfferDate;
         refreshInterval = Math.min(offer.getPaymentMethod().getMaxTradePeriod() / 5, MAX_REFRESH_INTERVAL);
+        xmrProofResult = new XmrProofResult(0, 0, XmrProofResult.State.FEATURE_DISABLED);

Why is xmrProofResult set here? It should be marked as a @Nullable fields and left null until used. Also we should use the setXmrProofResult method only to avoid that the xmrProofResultProperty gets out or sync.

>      // We use that for the XMR txKey but want to keep it generic to be flexible for other payment methods or assets.
     @Getter
     @Setter
     private String counterCurrencyExtraData;
 
+    @Getter
+    private XmrProofResult xmrProofResult;
+    public void setXmrProofResult(XmrProofResult xmrProofResult) {
+        this.xmrProofResult = xmrProofResult;
+        xmrProofResultProperty.setValue(xmrProofResult);
+    }
+    @Getter
+    // This observable property can be used for UI to show a notification to user of the XMR proof status
+    transient final private ObjectProperty<XmrProofResult> xmrProofResultProperty = new SimpleObjectProperty<>(xmrProofResult);

I think we should not pass xmrProofResult here as it is null at construction time.

> @@ -1170,6 +1181,7 @@ public String toString() {
                 ",\n     takerPaymentAccountId='" + takerPaymentAccountId + '\'' +
                 ",\n     errorMessage='" + errorMessage + '\'' +
                 ",\n     counterCurrencyTxId='" + counterCurrencyTxId + '\'' +
+                ",\n     counterCurrencyExtraData='" + counterCurrencyExtraData + '\'' +

We could add xmrProofResult as well 

>          }
+
+        log.warn("Tx Proof Failure {}, shutting down all open API requests for this trade {}",
+                result.getState(), trade.getShortId());
+        trade.setXmrProofResult(result);         // this updates the GUI with the status..
+        resultsCountdown = -1;  // signal all API requestors to cease
+        txProofResultsPending.put(trade.getId(), resultsCountdown);   // track proof result count
+        return failure;
+    }
+
+    private boolean isDisableAutoconf() {

Maybe rename to `isAutoConfDisabledByFilter` to make it more distinct to preferences value.

>  
-                                    if (!walletsSetup.isDownloadComplete()) {
-                                        return;
-                                    }
+        if (!p2PService.isBootstrapped()) {
+            return failure;
+        }
+        if (!walletsSetup.hasSufficientPeersForBroadcast()) {
+            return failure;
+        }
+        if (!walletsSetup.isDownloadComplete()) {

I am not sure if that handling is ok (same for hasSufficientPeersForBroadcast). I guess the message which triggers the processCounterCurrencyExtraData can be received while user is syncing wallet and we dont want to stop the service in such a case but delay the requests. As it cannot be handled here I would suggest to move the 
```
 if (!walletsSetup.hasSufficientPeersForBroadcast()) {
            return failure;
        }
        if (!walletsSetup.isDownloadComplete()) {
            return failure;
        }
```
code to the 
```
result -> {
                            if (!handleProofResult(result, trade))
                                xmrTransferProofService.terminateRequest(xmrProofInfo);
                        }
```
part of the calling method.

But not sure if that is really enough as it need to be handled in other places as well where the result gets processed. Need to get a bit more overview to be able to make more meaningful suggestions....

>  
-                                    if (!trade.isPayoutPublished()) {
-                                        trade.setState(Trade.State.SELLER_CONFIRMED_IN_UI_FIAT_PAYMENT_RECEIPT);
-                                    }
+        // here we count the Trade's API results from all
+        // different serviceAddress and figure out when all have finished
+        Integer resultsCountdown = txProofResultsPending.getOrDefault(trade.getId(), 0);

We dont use boxed types if not needed.

> -                            }
-
-                            proofResultWithTradeIdProperty.set(new XmrProofResultWithTradeId(result, trade.getId()));
-                        },
-                        (errorMsg, throwable) -> {
-                            log.warn(errorMsg);
-                        });
+        if (trade.isPayoutPublished()) {
+            log.warn("trade payout already published, shutting down all open API requests for this trade {}",
+                    trade.getShortId());
+            txProofResultsPending.remove(trade.getId());
+            return failure;
+        }
+
+        if (result.isPendingState()) {
+            log.info("Received a {} message for tradeId {}, retry will happen automatically",

This log is too generic. it should contain more specific info about the actual use case/feature.

> +        }
+
+        if (result.isSuccessState()) {
+            resultsCountdown -= 1;
+            log.info("Received a {} message, remaining proofs needed: {}, tradeId {}",
+                    result.getState(), resultsCountdown, trade.getShortId());
+            if (resultsCountdown > 0) {
+                txProofResultsPending.put(trade.getId(), resultsCountdown);   // track proof result count
+                return success; // not all APIs have confirmed yet
+            }
+            // we've received the final PROOF_OK, all good here.
+            txProofResultsPending.remove(trade.getId());
+            trade.setXmrProofResult(result);            // this updates the GUI with the status..
+            log.info("Auto confirm was successful, transitioning trade {} to next step...", trade.getShortId());
+            if (!trade.isPayoutPublished()) {
+                trade.setState(Trade.State.SELLER_CONFIRMED_IN_UI_FIAT_PAYMENT_RECEIPT);

Maybe we should add a comment here and to the `Trade.State.SELLER_CONFIRMED_IN_UI_FIAT_PAYMENT_RECEIPT` state that this can be also set by the autoConf feature and not only by UI activity. Renaming the state might be another option but not sure if its used in protobuf as well and if it would have any backward compatibility issues (likely not, but prob. not worth the risk and effort).

>          }
+
+        log.warn("Tx Proof Failure {}, shutting down all open API requests for this trade {}",

Can you comment more details when this case is expected?

> @@ -120,6 +120,10 @@
             new BlockChainExplorer("bsq.bisq.cc (@m52go)", "https://bsq.bisq.cc/tx.html?tx=", "https://bsq.bisq.cc/Address.html?addr=")
     ));
 
+    // list of XMR proof providers : this list will be used if no preference has been set
+    public static final List<String> DEFAULT_XMR_PROOF_PROVIDERS = new ArrayList<> (Arrays.asList(
+            "monero3bec7m26vx6si6qo7q7imlaoz45ot5m2b5z2ppgoooo6jx2rqd.onion"));

We should get at least 2 at release time.

> @@ -234,6 +238,9 @@ public void readPersisted() {
             prefPayload.setPreferredTradeCurrency(preferredTradeCurrency);
             setFiatCurrencies(CurrencyUtil.getMainFiatCurrencies());
             setCryptoCurrencies(CurrencyUtil.getMainCryptoCurrencies());
+            // default values for autoconfirmsettings when persisted payload is empty:
+            prefPayload.setAutoConfirmSettings(new AutoConfirmSettings(
+                    false, 5, 10000000, DEFAULT_XMR_PROOF_PROVIDERS));

Instead of 10000000 better use Coin.COIN.value

> @@ -127,8 +127,8 @@
     private int blockNotifyPort;
     private boolean tacAcceptedV120;
 
-    // Added with 1.3.7 false be default
-    private boolean autoConfirmXmr;
+    // Added after 1.3.7
+    private AutoConfirmSettings autoConfirmSettings;

Annotate with @Nullable

> @@ -190,7 +190,7 @@ public Message toProtoMessage() {
                 .setBuyerSecurityDepositAsPercentForCrypto(buyerSecurityDepositAsPercentForCrypto)
                 .setBlockNotifyPort(blockNotifyPort)
                 .setTacAcceptedV120(tacAcceptedV120)
-                .setAutoConfirmXmr(autoConfirmXmr);
+                .setAutoConfirmSettings((protobuf.AutoConfirmSettings) autoConfirmSettings.toProtoMessage());

Has to be handled as nullable value: Optional.ofNullable(

> @@ -279,6 +280,6 @@ public static PreferencesPayload fromProto(protobuf.PreferencesPayload proto, Co
                 proto.getBuyerSecurityDepositAsPercentForCrypto(),
                 proto.getBlockNotifyPort(),
                 proto.getTacAcceptedV120(),
-                proto.getAutoConfirmXmr());
+                AutoConfirmSettings.fromProto(userAutoConfirmSettings));

Has to be handled as nullable value

> @@ -569,6 +569,11 @@ portfolio.pending.step2_buyer.startPayment=Start payment
 portfolio.pending.step2_seller.waitPaymentStarted=Wait until payment has started
 portfolio.pending.step3_buyer.waitPaymentArrived=Wait until payment arrived
 portfolio.pending.step3_seller.confirmPaymentReceived=Confirm payment received
+portfolio.pending.step3_seller.autoConfirmStatus=Auto-Confirm Status
+portfolio.pending.autoConfirmTxNotFound=Tx Not Found

If space in UI permits better use full term `transaction` instead of tx.

> @@ -569,6 +569,11 @@ portfolio.pending.step2_buyer.startPayment=Start payment
 portfolio.pending.step2_seller.waitPaymentStarted=Wait until payment has started
 portfolio.pending.step3_buyer.waitPaymentArrived=Wait until payment arrived
 portfolio.pending.step3_seller.confirmPaymentReceived=Confirm payment received
+portfolio.pending.step3_seller.autoConfirmStatus=Auto-Confirm Status

 portfolio.pending.step3_seller.autoConfirmStatus=Auto-confirm status

> @@ -718,8 +723,8 @@ portfolio.pending.step3_seller.amountToReceive=Amount to receive
 portfolio.pending.step3_seller.yourAddress=Your {0} address
 portfolio.pending.step3_seller.buyersAddress=Buyers {0} address
 portfolio.pending.step3_seller.yourAccount=Your trading account
-portfolio.pending.step3_seller.xmrTxHash=Tx hash
-portfolio.pending.step3_seller.xmrTxKey=Tx private key
+portfolio.pending.step3_seller.xmrTxHash=Tx id

We use `Transaction ID` if space permits, otherwise `Tx ID`

> @@ -1232,6 +1241,9 @@ setting.about.shortcuts.sendFilter=Set Filter (privileged activity)
 setting.about.shortcuts.sendPrivateNotification=Send private notification to peer (privileged activity)
 setting.about.shortcuts.sendPrivateNotification.value=Open peer info at avatar or dispute and press: {0}
 
+setting.info.headline=New XMR Autoconfirm feature
+setting.info.msg=When selling BTC for XMR you can use the autoconfirm feature to verify that the correct amount of XMR has been deposited to your wallet and automatically confirm the trade, releasing BTC to the counterparty.\n\n\
+  This can make the trade process more efficient for both peers.  Limits can also be set on the max amount of BTC that can be auto-confirmed.  See the auto-confirm section on this page for more info and to enable the feature.

We should request from m52go to review/improve the text. 

> @@ -2493,8 +2506,8 @@ showWalletDataWindow.walletData=Wallet data
 showWalletDataWindow.includePrivKeys=Include private keys
 
 setXMRTxKeyWindow.headline=Prove sending of XMR
-setXMRTxKeyWindow.txHash=Transaction hash
-setXMRTxKeyWindow.txKey=Tx private key
+setXMRTxKeyWindow.txHash=Transaction id
+setXMRTxKeyWindow.txKey=Tx key

We should stay consistent with the term. I think `Transaction private key` is used in monero.

> @@ -718,8 +723,8 @@ portfolio.pending.step3_seller.amountToReceive=Amount to receive
 portfolio.pending.step3_seller.yourAddress=Your {0} address
 portfolio.pending.step3_seller.buyersAddress=Buyers {0} address
 portfolio.pending.step3_seller.yourAccount=Your trading account
-portfolio.pending.step3_seller.xmrTxHash=Tx hash
-portfolio.pending.step3_seller.xmrTxKey=Tx private key
+portfolio.pending.step3_seller.xmrTxHash=Tx id
+portfolio.pending.step3_seller.xmrTxKey=Tx key
 portfolio.pending.step3_seller.xmrTxVerificationError=The XMR transfer validation for trade with ID ''{0}'' failed.\nReason: ''{1}''

Seems xmrTxVerificationError is not used anymore

> @@ -76,8 +76,8 @@
     private BusyAnimation busyAnimation;
     private Subscription tradeStatePropertySubscription;
     private Timer timeoutTimer;
-    private final ChangeListener<XmrProofResultWithTradeId> xmrProofResultWithTradeIdListener;
-
+    TextFieldWithCopyIcon autoConfirmStatusField;

should be private

> @@ -895,9 +915,63 @@ private void activateDaoPreferences() {
         blockNotifyPortTextField.textProperty().addListener(blockNotifyPortListener);
     }
 
+    private void activateAutoConfirmPreferences() {
+        {
+            AutoConfirmSettings x = preferences.getAutoConfirmSettings();
+            autoConfirmXmr.setSelected(x.enabled);
+            autoConfRequiredConfirmations.setText(String.valueOf(x.requiredConfirmations));
+            autoConfTradeLimit.setText(formatter.formatCoin(Coin.valueOf(x.tradeLimit)));
+            autoConfServiceAddress.setText(String.join(", ", x.serviceAddresses));
+        }
+
+        autoConfirmXmr.setOnAction(e -> {
+            boolean bEnabled = autoConfirmXmr.isSelected();

We dont use type prefixes

> @@ -895,9 +915,63 @@ private void activateDaoPreferences() {
         blockNotifyPortTextField.textProperty().addListener(blockNotifyPortListener);
     }
 
+    private void activateAutoConfirmPreferences() {
+        {

Why you use an initializer block here?

> +            String serviceAddress) {
+        this.txHash = txHash;
+        this.txKey = txKey;
+        this.recipientAddress = recipientAddress;
+        this.amount = amount;
+        this.confirmsRequired = confirmsRequired;
+        this.serviceAddress = serviceAddress;
+    }
+
+    // something to uniquely identify this object by
+    public String getKey() {
+        return txHash + "|" + serviceAddress;
+    }
+
+    public XmrProofResult checkApiResponse(String jsonTxt) {
+        try {

I think it would be useful to log the full jsonTxt.

>                                Consumer<XmrProofResult> resultHandler,
                               FaultHandler faultHandler) {
-        this.httpClient = httpClient;
-        this.tradeDate = tradeDate;
-        this.txHash = txHash;
-        this.txKey = txKey;
-        this.recipientAddress = recipientAddress;
-        this.amount = amount;
+        this.httpClient = new XmrTxProofHttpClient(socks5ProxyProvider);
+        this.httpClient.setBaseUrl("http://" + xmrProofInfo.getServiceAddress());
+        if (xmrProofInfo.getServiceAddress().matches("^192.*")) {
+            log.info("ignoring Socks5 proxy for local net address: {}", xmrProofInfo.getServiceAddress());

See ProvidersRepository how it uses @Named(Config.USE_LOCALHOST_FOR_P2P) boolean useLocalhostForP2P to determine if a clearnet address need to be used.

>                                Consumer<XmrProofResult> resultHandler,
                               FaultHandler faultHandler) {
-        this.httpClient = httpClient;
-        this.tradeDate = tradeDate;
-        this.txHash = txHash;
-        this.txKey = txKey;
-        this.recipientAddress = recipientAddress;
-        this.amount = amount;
+        this.httpClient = new XmrTxProofHttpClient(socks5ProxyProvider);
+        this.httpClient.setBaseUrl("http://" + xmrProofInfo.getServiceAddress());
+        if (xmrProofInfo.getServiceAddress().matches("^192.*")) {
+            log.info("ignoring Socks5 proxy for local net address: {}", xmrProofInfo.getServiceAddress());

The regex with `^192.*` would not match if a user sets `localhost`. 

> @@ -234,6 +238,9 @@ public void readPersisted() {
             prefPayload.setPreferredTradeCurrency(preferredTradeCurrency);
             setFiatCurrencies(CurrencyUtil.getMainFiatCurrencies());
             setCryptoCurrencies(CurrencyUtil.getMainCryptoCurrencies());
+            // default values for autoconfirmsettings when persisted payload is empty:
+            prefPayload.setAutoConfirmSettings(new AutoConfirmSettings(
+                    false, 5, 10000000, DEFAULT_XMR_PROOF_PROVIDERS));

This code need to be moved outside the if/else clause. 
```
 if (getAutoConfirmSettings() == null) {
            prefPayload.setAutoConfirmSettings(new AutoConfirmSettings(
                    false, 5, Coin.COIN.value, DEFAULT_XMR_PROOF_PROVIDERS));
        }
```
Otherwise it will be only set for new users but not for those who have already the prefPayload but not the autoConfirmSettings

> @@ -569,6 +569,11 @@ portfolio.pending.step2_buyer.startPayment=Start payment
 portfolio.pending.step2_seller.waitPaymentStarted=Wait until payment has started
 portfolio.pending.step3_buyer.waitPaymentArrived=Wait until payment arrived
 portfolio.pending.step3_seller.confirmPaymentReceived=Confirm payment received
+portfolio.pending.step3_seller.autoConfirmStatus=Auto-Confirm Status
+portfolio.pending.autoConfirmTxNotFound=Tx Not Found

also lower case...
portfolio.pending.autoConfirmTxNotFound=Transaction not found

-- 
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/4421#pullrequestreview-472970684
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200822/3dbcad0f/attachment-0001.html>


More information about the bisq-github mailing list