[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