<p><b>@ManfredKarrer</b> approved this pull request.</p>

<p>ACK</p>
<p>In Peer the EqualsAndHashCode should include a super call. I will push that afterwards.<br>
IntelliJ reports such issues with static code analysis (colored bg), otherwise I would not have spotted it as well ;-)<br>
<code>@EqualsAndHashCode(exclude = {"date"}, callSuper = true)</code></p>
<p>I tested just lightly. As that comes with many "risky" changes we have to observe well if there are any issues reported which might be connected to the topics of that PR.</p><hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260561049">p2p/src/main/java/bisq/network/p2p/peers/peerexchange/Peer.java</a>:</p>
<pre style='color:#555'>> @@ -39,47 +38,43 @@
 @Getter
 @EqualsAndHashCode(exclude = {"date"}) // failedConnectionAttempts is transient and therefore excluded anyway
 @Slf4j
-public final class Peer implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
+public final class Peer extends Capabilities implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
</pre>
<p>Missing call to super in <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=34339257" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/EqualsAndHashCode">@EqualsAndHashCode</a></p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260561250">p2p/src/main/java/bisq/network/p2p/peers/peerexchange/Peer.java</a>:</p>
<pre style='color:#555'>> @@ -39,47 +38,43 @@
 @Getter
 @EqualsAndHashCode(exclude = {"date"}) // failedConnectionAttempts is transient and therefore excluded anyway
 @Slf4j
-public final class Peer implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
+public final class Peer extends Capabilities implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
</pre>
<p>Extension feels not really right to me here. A peer "has capabilities" and not "is capabilities".</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260561540">p2p/src/main/java/bisq/network/p2p/peers/peerexchange/Peer.java</a>:</p>
<pre style='color:#555'>> @@ -39,47 +38,43 @@
 @Getter
 @EqualsAndHashCode(exclude = {"date"}) // failedConnectionAttempts is transient and therefore excluded anyway
 @Slf4j
-public final class Peer implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
+public final class Peer extends Capabilities implements NetworkPayload, PersistablePayload, SupportedCapabilitiesListener {
</pre>
<p>Same with SharedModel in Connection class..</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260562066">seednode/src/main/java/bisq/seednode/SeedNodeMain.java</a>:</p>
<pre style='color:#555'>> @@ -60,7 +63,10 @@ protected void doExecute(OptionSet options) {
 
     @Override
     protected void addCapabilities() {
-        Capabilities.addCapability(Capabilities.Capability.SEED_NODE.ordinal());
+        // TODO got even worse after refactoring
+        List<Integer> current = Capabilities.toIntList(Capabilities.app);
+        current.add(Capability.SEED_NODE.ordinal());
+        Capabilities.app.resetCapabilities(Capabilities.fromIntList(current));
</pre>
<p>Why not add a method for appending a capability? Check to not add duplicates can be done with capability object itself rather then ordinal, no (missing here but was at another class where it was added after checking if not already there)?</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260562520">p2p/src/main/java/bisq/network/p2p/network/Connection.java</a>:</p>
<pre style='color:#555'>>          } else {
-            return msg instanceof CapabilityRequiringPayload && sharedModel.isCapabilitySupported(((CapabilityRequiringPayload) msg).getRequiredCapabilities());
+            return true;
</pre>
<p>The noCapabilityRequiredOrCapabilityIsSupported is very hard to understand (was worse before). Maybe we should use that refactoring moment for improving the method so it becomes more easier.<br>
I would need more time to review the corrtectness (compared to the old version), but would prefer to invest that time in a improved method version.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260563307">common/src/main/java/bisq/common/app/Capabilities.java</a>:</p>
<pre style='color:#555'>> -    // Sequence in the enum must not be changed (append only).
-    public enum Capability {
-        TRADE_STATISTICS,
-        TRADE_STATISTICS_2,
-        ACCOUNT_AGE_WITNESS,
-        SEED_NODE,
-        DAO_FULL_NODE,
-        PROPOSAL,
-        BLIND_VOTE,
-        ACK_MSG,
-        BSQ_BLOCK
+
+    /**
+     * The global set of capabilities, i.e. the capabilities if the local app.
+     */
+    public static final Capabilities app = new Capabilities();
</pre>
<p>I would prefer a more descriptive name here. When I read Capabilities.app it was not very clear what it means.<br>
Also does not need to be static?<br>
Should the class extend HashSet instead of using the field capabilities - as that is basically what it holds?</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260563513">common/src/main/java/bisq/common/app/Capabilities.java</a>:</p>
<pre style='color:#555'>>      }
 
-    // Application need to set supported capabilities at startup
-    @Getter
-    @Setter
-    private static List<Integer> supportedCapabilities = new ArrayList<>();
+    public void resetCapabilities(Capabilities capabilities) {
+        resetCapabilities(capabilities.capabilities);
</pre>
<p>capabilities.capabilities reads strange. If we would use a HashSet as super class i think it becomes more natural.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260563693">common/src/main/java/bisq/common/app/Capabilities.java</a>:</p>
<pre style='color:#555'>>      }
 
-    // Application need to set supported capabilities at startup
-    @Getter
-    @Setter
-    private static List<Integer> supportedCapabilities = new ArrayList<>();
+    public void resetCapabilities(Capabilities capabilities) {
+        resetCapabilities(capabilities.capabilities);
+    }
+
+    public void resetCapabilities(Collection<Capability> capabilities) {
</pre>
<p>Would replaceCapabilities be better than resetCapabilities? With reset I would expect more the clear funtion rather then the additional add.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2469#discussion_r260564921">common/src/main/java/bisq/common/app/Capabilities.java</a>:</p>
<pre style='color:#555'>> -    // Sequence in the enum must not be changed (append only).
-    public enum Capability {
-        TRADE_STATISTICS,
-        TRADE_STATISTICS_2,
-        ACCOUNT_AGE_WITNESS,
-        SEED_NODE,
-        DAO_FULL_NODE,
-        PROPOSAL,
-        BLIND_VOTE,
-        ACK_MSG,
-        BSQ_BLOCK
+
+    /**
+     * The global set of capabilities, i.e. the capabilities if the local app.
+     */
+    public static final Capabilities app = new Capabilities();
</pre>
<p>Maybe we should make it a Guice instance scoped as singleton?<br>
And then use it in peer and shared model as parameter passed to the constructor (I know there is missing guice and passing is painful)... So maybe keeping the Capabilities.app static is less painful?<br>
But then maybe its better to move it to another class which reflects that it holds the global capability state of the app?</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/2469#pullrequestreview-208296865">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZttgcKOB7j_thwaR47FgSHdOvcl4iks5vUFF4gaJpZM4bRz-x">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtmgXFhpkOULqbF8ybzlsxgj6xzQTks5vUFF4gaJpZM4bRz-x.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@ManfredKarrer approved #2469"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2469#pullrequestreview-208296865"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2469#pullrequestreview-208296865",
"url": "https://github.com/bisq-network/bisq/pull/2469#pullrequestreview-208296865",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>