[bisq-network/bisq] Dao ui improvements (#1751)

[sqrrm]

sqrrm commented on this pull request.

I would like some clarification on my comments on how calculations are done. Something seems strange about it, either it's correct and some code comments are needed or something isn't working as intended.

>  
 import org.bitcoinj.core.Coin;
 
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
 public class CompensationConsensus extends ProposalConsensus {
-    public static Coin getMinCompensationRequestAmount() {
-        return Coin.valueOf(1_000); // 10 BSQ
+    public static Coin getMinCompensationRequestAmount(BsqStateService bsqStateService, int chainHeadHeight) {

Why call the argument chainHeadHeight instead of just chainHeight?

> +        switch (param) {
+            case UNDEFINED:
+                break;
+            case BSQ_MAKER_FEE_IN_PERCENT:
+                break;
+            case BSQ_TAKER_FEE_IN_PERCENT:
+                break;
+            case BTC_MAKER_FEE_IN_PERCENT:
+                break;
+            case BTC_TAKER_FEE_IN_PERCENT:
+                break;
+            case PROPOSAL_FEE:
+                break;
+            case BLIND_VOTE_FEE:
+                break;
+            case COMPENSATION_REQUEST_MIN_AMOUNT:

There should also be a check that `COMPENSATION_REQUEST_MIN_AMOUNT < COMPENSATION_REQUEST_MAX_AMOUNT`, both current and proposed.

> +        long min = getNewValueByPercentChange(param, minPercentChange);
+        if (paramValue < min)
+            throw new ChangeParamValidationException("Value must not be smaller than " + min);
+    }
+
+    /**
+     * @param param         The param to change
+     * @param percentChange 100 means 100% more than current value -> 2 times current value. -50 means half of the current value
+     * @return The new value.
+     */
+    //TODO add test
+    // TODO use multiplier to make it more intuitive? (4,4) means 4 times current value for max and divided by 4 to get min value)
+    @VisibleForTesting
+    long getNewValueByPercentChange(Param param, long percentChange) {
+        checkArgument(percentChange > -100, "percentChange must be bigger than -100");
+        return (getCurrentValue(param) * 100 * (100 + percentChange)) / 100;

This multiplies by 100 one time too many.

> @@ -114,4 +116,101 @@ public Coin parseSatoshiToBtc(String satoshi) {
             return Coin.ZERO;
         }
     }
+
+
+    public String formatParamValue(Param param, long value) {
+        switch (param) {
+            case UNDEFINED:
+                return Res.get("shared.na");
+
+            case BSQ_MAKER_FEE_IN_PERCENT:
+            case BSQ_TAKER_FEE_IN_PERCENT:
+            case BTC_MAKER_FEE_IN_PERCENT:
+            case BTC_TAKER_FEE_IN_PERCENT:
+                return formatToPercentWithSymbol(value / 10000d);

Why is the value divided by 10000 rather than 100 to get percent?

-- 
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/1751#pullrequestreview-162251374
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20181008/892eeecd/attachment-0001.html>