[bisq-network/bisq] Use different security deposit for fiat-crypto and crypto-crypto trades (#2742)

[sqrrm]

sqrrm commented on this pull request.

I think this is mostly correct as far as I know, I'm not that familiar with the trade protocol though so I can't tell if there is some case where the deposit should be updated.

Also, see inline comments.

>  
 import org.bitcoinj.core.Coin;
 
+import static bisq.core.payment.PaymentAccountUtil.isCryptoCurrencyAccount;

Is it appropriate to do a static import here? It probably doesn't matter in this class, I feel it makes code less readable when it's not that frequently used. More a matter of our policy than anything.

> @@ -50,16 +53,25 @@ public static Coin getMinTradeAmount() {
         return MIN_TRADE_AMOUNT;
     }
 
-    public static double getDefaultBuyerSecurityDepositAsPercent() {
-        return 0.1; // 10% of trade amount.
+    public static double getDefaultBuyerSecurityDepositAsPercent(PaymentAccount paymentAccount) {

It's nice to know when nullable is allowed

> @@ -282,6 +282,8 @@ public boolean initWithData(OfferPayload.Direction direction, TradeCurrency trad
         setTradeCurrencyFromPaymentAccount(paymentAccount);
         tradeCurrencyCode.set(this.tradeCurrency.getCode());
 
+        setTradeCurrencyFromPaymentAccount(paymentAccount);

I don't understand why this needs to be done twice, it's set three lines up.

-- 
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/2742#pullrequestreview-228379961
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190418/69343283/attachment-0001.html>