<p></p>
<p><b>@chimp1984</b> approved this pull request.</p>
<p>utACK, some nits and suggestions but looks ok... not reviewed UI part much</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543568033">core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java</a>:</p>
<pre style='color:#555'>> @@ -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());
</pre>
<p>Is that a debug util or is the "," concatenation used in normal cases?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543569089">desktop/src/main/java/bisq/desktop/main/account/content/PaymentAccountsView.java</a>:</p>
<pre style='color:#555'>> @@ -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)) {
</pre>
<p>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...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543571219">core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>Why not move return into the try block?</p>
<pre><code>return getWitnessByHash(accountAgeWitnessHash)
.map(ageWitness -> new Tuple2<>(ageWitness, pubKeyHash))
.orElse(null);
</code></pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543572729">desktop/src/main/java/bisq/desktop/main/overlays/windows/SignSpecificWitnessWindow.java</a>:</p>
<pre style='color:#555'>>
+ private ECKey checkedArbitratorKey() {
+ var arbitratorKey = arbitratorManager.getRegistrationKey(privateKey.getText());
</pre>
<p>So that use old arbitrators, right? there are none registered anymore. Wouldn't it be better to use mediators or refund agents?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543573557">core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java</a>:</p>
<pre style='color:#555'>> @@ -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(",");
</pre>
<p>Ah now I see why u use "," for concatination....<br>
A pipe or a static field for it would make it a bit more intuitive... but no worry... being picky ;-)</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543568296">core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java</a>:</p>
<pre style='color:#555'>> @@ -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());
</pre>
<p>If debug tool better to rename method to make it more clear and add a comment...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543575871">core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java</a>:</p>
<pre style='color:#555'>> @@ -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());
</pre>
<p>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</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4957#discussion_r543569491">desktop/src/main/java/bisq/desktop/main/account/content/PaymentAccountsView.java</a>:</p>
<pre style='color:#555'>> @@ -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)) {
</pre>
<p>As its only used for fiat, better move it to fiat class instead of base class</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4957#pullrequestreview-552728596">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNUONBE4WCOP65W74ULSU6RZ7ANCNFSM4U4P2ZVA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNWPU7VLPER45BYJBGLSU6RZ7A5CNFSM4U4P2ZVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEDY7QFA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4957#pullrequestreview-552728596",
"url": "https://github.com/bisq-network/bisq/pull/4957#pullrequestreview-552728596",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>