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

[chimp1984]

@jmacxx Thanks for the work on the feature! Looks pretty good already.

Some comments: 
- I would recommend to use Capitalistion in comments and log messages. Proper sentences makes logs easier to read.  
- Could you merge current master into your branch, should not cause any conflicts.
- Translation strings should be lower case inside sentence...
- Hex validation is done in several places. Would be better to move to Hex util class. Regex is not needed as encode/decode throws if input is wrong. Just need additional lenght check.
- Maybe we should consider to move the processCounterCurrencyExtraData method in TradeManager to a new class to not over burden the TradeManager too much with this specialized feature. It is quite large already...
- We mix a bit between generice feature and XMR specific. Even there are no concrete plans to add the autoconf feature to other altcoins we might add it (e.g. if it get sponsored by the altocin), so better to have all prepared to not get issues when we add more. UI can easily be changed, but data in protobuf, trade, messages are more problematic. 
- In preferences view the auto conf feature is at the very bottom. We  could decrease vertical space for currency selection to make it more prominent or even move it in front of currency selection. 
- The addressRegexValidator does not allow http:// only the IP or onion. 
- Deleting the service address does not refill it with default only at UI screen change or restart. Its a bit tricky to do that. Usually we used a focusOutHandler on the inputfield and apply it when the user leaves focus. 
- The Auto-confirmed badge label is not veritcally centered. Might be a rounding issue with layout...seems to be 1 pixel off.
- Not sure if needed, but might be nice to see the status information at the sellers pending trade view. e.g. how many confirmations from which services. But not sure if UI space allows that. Can be added also later if we see demand for it.
- For dev testing it would be good to deactivate certain checks, like that the key was not used in the past, the date check and the amount check. Otherwise testers need to create a XMR tx for each test run... The DevEnv.devMode property could be used to ignore those checks. Also would be convenient to autofill the buyers tx id and txkey fields with valid data.
- We should consider to allow emty tx ID and tx key input but show a warning if user does not want to send those data. I am not aware of any privacy issue by revealing those data, but it could be that there are some or at least some traders think there are some. Beside that if the users used a wallet which does not provide those data it would lead to a dispute case as he cannot confirm that he sent the xmr. It is mandatory to use a wallet which supports those data, but not sure if we should enforce disputes where otherwise if the seller recieved it there would not be a dispute.
- We should also state in the info popup that it is best for security and privacy that the user runs his own service node and should provide some easy setup instructions (e.g. install script) . beside that we should make clear that there might be reasons why the severice does not provide the result and the user still need to check in the permitted trade time the state of the trade.
- We should add some more detailed info text outside of the app (e.g. blog, wiki, docs). @m52go what do you think? We should make clear the limitations and context of the service. E.g. security is based on assumption that 2 bonded role owners do not collude to defraud traders. There is a privacy loss as the service operators learn about the matching of an onion address and a XMR tx id and amount. All that can be avoided if user runs own service. And of course it is an optional feature if traders dont feel comfortable with it.
- It would be nice to get some statistics how much the feature is used. We could get number of trades using the feature from the node operators (excluding those who run their own service).  
- Once we are in a more final state we should think about the edge cases like when wallet is not synced.... also to define test use cases would help ourself for dev testing and later the testers for release testing.

-- 
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#issuecomment-678728703
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200822/d998c012/attachment.html>