[bisq-network/bisq] Autoconfirm XMR trade if tx check was validated by proof service (#4421)
James Cox
notifications at github.com
Mon Aug 24 19:44:30 UTC 2020
> 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.
Changed proto definition to allow a list of AutoConfirmSettings.
Commit https://github.com/bisq-network/bisq/pull/4421/commits/4dde9e19cf518af5cd2609cf893d0e03f0b29564
> 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...
Moved the new code out to AutoConfirmManager.java
Commit https://github.com/bisq-network/bisq/pull/4421/commits/13978e33c2e0882ebb291f3a713bdc5d612ef500
> 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.
Is this referring to the Transaction ID and key input fields? I should not restrict them to hex input?
> The addressRegexValidator does not allow http:// only the IP or onion.
`address:port` is all that is needed. Bisq chooses the protocol (http).
> 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.
It shows the number/expected confirmations.
> The Auto-confirmed badge label is not veritcally centered. Might be a rounding issue with layout...seems to be 1 pixel off.
I intentionally oriented the badge slightly above and to the right of the title to make it appear similar to other badges shown on menu items. I think it looks great that way, but could certainly move it down a bit if that is what you want.
> 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.
Done. Commit https://github.com/bisq-network/bisq/pull/4421/commits/99146009b6bfdb22e333d11f99c0110436166f71
Will post a screenshot so @m52go can review.
> 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.
It allows empty entry; I've also added the warning in that case. Will post a screenshot so @m52go can review.
> 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.
Done.
> 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.
Done.
> I would recommend to use Capitalistion in comments and log messages. Proper sentences makes logs easier to read.
> Translation strings should be lower case inside sentence...
Duly noted; I have attempted to correct all those instances of mis-capitalization.
--
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-679328214
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200824/fb2c6f78/attachment-0001.html>
More information about the bisq-github
mailing list