[bisq-network/bisq] Sign single account (#4957)

chimp1984 notifications at github.com
Tue Dec 15 19:17:03 CET 2020


@chimp1984 approved this pull request.

utACK, some nits and suggestions but looks ok... not reviewed UI part much

> @@ -880,4 +880,10 @@ public boolean isSignWitnessTrade(Trade trade) {
                 !peerHasSignedWitness(trade) &&
                 tradeAmountIsSufficient(trade.getTradeAmount());
     }
+
+    public String signInfoFromAccount(PaymentAccount paymentAccount) {
+        var pubKey = keyRing.getSignatureKeyPair().getPublic();
+        var witness = getMyWitness(paymentAccount.getPaymentAccountPayload());
+        return Utilities.bytesAsHexString(witness.getHash()) + "," + Utilities.bytesAsHexString(pubKey.getEncoded());

Is that a debug util or is the "," concatenation used in normal cases?

> @@ -62,6 +62,8 @@ public void initialize() {
                 accountAgeWitnessService.getAccountAgeWitnessUtils().logSigners();
             } else if (Utilities.isCtrlShiftPressed(KeyCode.U, event)) {
                 accountAgeWitnessService.getAccountAgeWitnessUtils().logUnsignedSignerPubKeys();
+            } else if (Utilities.isAltOrCtrlPressed(KeyCode.C, event)) {

Cmd+c is a very common key command, not sure if its better to use a more exotic one so that user do not trigger that accidentally. Prob. no harm by doing that, but confusing...

> +    public Tuple2<AccountAgeWitness, byte[]> signInfoFromString(String signInfo) {
+        var parts = signInfo.split(",");
+        if (parts.length != 2){
+            return null;
+        }
+        byte[] pubKeyHash;
+        Optional<AccountAgeWitness> accountAgeWitness;
+        try {
+            var accountAgeWitnessHash = Utilities.decodeFromHex(parts[0]);
+            pubKeyHash = Utilities.decodeFromHex(parts[1]);
+            accountAgeWitness = getWitnessByHash(accountAgeWitnessHash);
+        } catch (Exception e) {
+            return null;
+        }
+
+        return accountAgeWitness

Why not move return into the try block?

```
return getWitnessByHash(accountAgeWitnessHash)
                .map(ageWitness -> new Tuple2<>(ageWitness, pubKeyHash))
                .orElse(null);
```

>  
+    private ECKey checkedArbitratorKey() {
+        var arbitratorKey = arbitratorManager.getRegistrationKey(privateKey.getText());

So that use old arbitrators, right? there are none registered anymore. Wouldn't it be better to use mediators or refund agents?

> @@ -886,4 +887,24 @@ public String signInfoFromAccount(PaymentAccount paymentAccount) {
         var witness = getMyWitness(paymentAccount.getPaymentAccountPayload());
         return Utilities.bytesAsHexString(witness.getHash()) + "," + Utilities.bytesAsHexString(pubKey.getEncoded());
     }
+
+    public Tuple2<AccountAgeWitness, byte[]> signInfoFromString(String signInfo) {
+        var parts = signInfo.split(",");

Ah now I see why u use "," for concatination....
A pipe or a static field for it would make it a bit more intuitive... but no worry... being picky ;-)

> @@ -880,4 +880,10 @@ public boolean isSignWitnessTrade(Trade trade) {
                 !peerHasSignedWitness(trade) &&
                 tradeAmountIsSufficient(trade.getTradeAmount());
     }
+
+    public String signInfoFromAccount(PaymentAccount paymentAccount) {
+        var pubKey = keyRing.getSignatureKeyPair().getPublic();
+        var witness = getMyWitness(paymentAccount.getPaymentAccountPayload());
+        return Utilities.bytesAsHexString(witness.getHash()) + "," + Utilities.bytesAsHexString(pubKey.getEncoded());

If debug tool better to rename method to make it more clear and add a comment...

> @@ -880,4 +880,10 @@ public boolean isSignWitnessTrade(Trade trade) {
                 !peerHasSignedWitness(trade) &&
                 tradeAmountIsSufficient(trade.getTradeAmount());
     }
+
+    public String signInfoFromAccount(PaymentAccount paymentAccount) {
+        var pubKey = keyRing.getSignatureKeyPair().getPublic();
+        var witness = getMyWitness(paymentAccount.getPaymentAccountPayload());
+        return Utilities.bytesAsHexString(witness.getHash()) + "," + Utilities.bytesAsHexString(pubKey.getEncoded());

After reviewing the rest I understand now better... I stil would recommend to rename the method so it is more clear its not a normal user method but a special tool... also i am old-school and prefer get prefix... here it could read as intent as well: sign infoFromAccount.... instead of: get signInfo from account

> @@ -62,6 +62,8 @@ public void initialize() {
                 accountAgeWitnessService.getAccountAgeWitnessUtils().logSigners();
             } else if (Utilities.isCtrlShiftPressed(KeyCode.U, event)) {
                 accountAgeWitnessService.getAccountAgeWitnessUtils().logUnsignedSignerPubKeys();
+            } else if (Utilities.isAltOrCtrlPressed(KeyCode.C, event)) {

As its only used for fiat, better move it to fiat class instead of base class

-- 
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/4957#pullrequestreview-552728596
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201215/307087da/attachment.htm>


More information about the bisq-github mailing list