Refactor capabilities and other P2P-related stuff #2469

freimair · 2019-02-26T10:12:41Z

Got rid of the Capability ordinals being used all over the place - a first and major step towards a cleaner, more readable Bisq code base where Bugs have less places to hide. (Done in my intention to clean up the p2p part)

Doing that I even managed to remove some 60 loc: some stats using github.com/AlDanial/cloc v 1.80

EDIT: after a quick chat with @ManfredKarrer I did some more refactoring...

_	files	blank	comment	code
same	0	0	1087	3842
modified	32	0	7	208
added	1	18	37	114
removed	0	40	5	304

Note

this is a fairly big change where functionality should remain the same. However, this needs thorough testing.
My suggestion is that every reviewer of this PR should have a testing session with me where we do the basic things such as a simple trade so major bugs are discovered before merging.
In-depth testing is done anyhow before the next release...

github.com/AlDanial/cloc v 1.80 T=0.19 s (5.3 files/s, 5.3 lines/s) ------------------------------------------------------------------------------- files blank comment code ------------------------------------------------------------------------------- same 0 0 913 3405 modified 28 0 6 145 added 1 18 36 58 removed 0 9 3 103 -------------------------------------------------------------------------------

ManfredKarrer

ACK

In Peer the EqualsAndHashCode should include a super call. I will push that afterwards.
IntelliJ reports such issues with static code analysis (colored bg), otherwise I would not have spotted it as well ;-)
@EqualsAndHashCode(exclude = {"date"}, callSuper = true)

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.

ManfredKarrer · 2019-02-27T01:30:58Z

p2p/src/main/java/bisq/network/p2p/peers/peerexchange/Peer.java

@@ -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 {


Missing call to super in @EqualsAndHashCode

Extension feels not really right to me here. A peer "has capabilities" and not "is capabilities".

Same with SharedModel in Connection class..

aggreed. but peer.isCapabilitySupported(...) and connection.isCapabilitySupported(...) feels quite right. Having them implement some sort of HasCapabilities interface would result in peer.getCapabilities().isCapabilitySupported(...) which imho is confusing.

ManfredKarrer · 2019-02-27T01:36:30Z

seednode/src/main/java/bisq/seednode/SeedNodeMain.java

+        // 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));


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)?

first, there is a TODO and second, we need that kind of code exactly once. and once more for the monitor.

ManfredKarrer · 2019-02-27T01:39:11Z

p2p/src/main/java/bisq/network/p2p/network/Connection.java

        } else {
-            return msg instanceof CapabilityRequiringPayload && sharedModel.isCapabilitySupported(((CapabilityRequiringPayload) msg).getRequiredCapabilities());
+            return true;


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.
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.

Agreed. I thought about changing stuff around but it quickly gets dirty.

Most beautiful would be that we do not need the method at all but instead can use a connection.isCapabilitySupported(msg) or isCapabilitySupported(msg) directly. However, the class hierarchy has grown overly complicated over time and such a solution is not possible as things stand now (would result in a diamond pattern if Java would allow for that).

Even looks worse if depicted like that:

I even tried to use java8/9 defaults in interfaces and create some sort of mixins. Nasty stuff. Definitely needs cleanup! Small steps...

ManfredKarrer · 2019-02-27T01:43:47Z

common/src/main/java/bisq/common/app/Capabilities.java

+    /**
+     * The global set of capabilities, i.e. the capabilities if the local app.
+     */
+    public static final Capabilities app = new Capabilities();


I would prefer a more descriptive name here. When I read Capabilities.app it was not very clear what it means.
Also does not need to be static?
Should the class extend HashSet instead of using the field capabilities - as that is basically what it holds?

Maybe we should make it a Guice instance scoped as singleton?
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?
But then maybe its better to move it to another class which reflects that it holds the global capability state of the app?

I would prefer a more descriptive name here. When I read Capabilities.app it was not very clear what it means.

make a suggestion

Also does not need to be static?

yes, as it holds the capabilities of the running Bisq itself and is used in multiple places all over the code.

Should the class extend HashSet instead of using the field capabilities - as that is basically what it holds?

see below

ManfredKarrer · 2019-02-27T01:44:56Z

common/src/main/java/bisq/common/app/Capabilities.java

-    @Setter
-    private static List<Integer> supportedCapabilities = new ArrayList<>();
+    public void resetCapabilities(Capabilities capabilities) {
+        resetCapabilities(capabilities.capabilities);


capabilities.capabilities reads strange. If we would use a HashSet as super class i think it becomes more natural.

yes I thought about that but having Set as superclass adds all the methods as well and well, makes the whole Capabilities class superfluous which then results in handling capabilities like a set. I do not know if we want that.

ManfredKarrer · 2019-02-27T01:46:04Z

common/src/main/java/bisq/common/app/Capabilities.java

+        resetCapabilities(capabilities.capabilities);
+    }
+
+    public void resetCapabilities(Collection<Capability> capabilities) {


Would replaceCapabilities be better than resetCapabilities? With reset I would expect more the clear funtion rather then the additional add.

hm interesting. A setCapabilities seems to me clear as it replaces all capabilities, a resetCaps indicates more strongly, that the capabilities are going to be replaced. I would understand a clearCapabilities as a clear without add.

Again, in a collection of event logs, a reset clears the log.

Hm. setCapabilities or simply set?

freimair requested a review from ManfredKarrer as a code owner February 26, 2019 10:12

freimair requested a review from ripcurlx as a code owner February 27, 2019 05:44

freimair changed the title ~~Refactor capabilities~~ [WIP] Refactor capabilities Feb 27, 2019

ManfredKarrer added the in:network label Feb 27, 2019

freimair mentioned this pull request Feb 27, 2019

For February 2019 bisq-network/compensation#226

Closed

freimair force-pushed the refactor_capabilities branch from c9a8e0e to d2eae71 Compare March 1, 2019 14:50

ManfredKarrer added this to the v0.9.6 milestone Mar 1, 2019

freimair added 11 commits March 6, 2019 09:29

Removed duplicate and superfluous code

ed799b7

Make Monitor aware of refactoring

3909fee

Get rid of 'sharedModel' in 'Connection' class

7fa5c19

Get rid of 'InputHandler' in 'Connection' class

4d21d25

Simplified connection shutdown

0320261

Merged methods that did the same

d21bf3d

Removed field that is only used once

b501a20

Removed superfluous imports

b85075a

Compressed shutdown members

3bb38b8

Removed closeConnectionReason from class scope

2450117

freimair force-pushed the refactor_capabilities branch from d2eae71 to 1875b88 Compare March 6, 2019 08:36

freimair added 3 commits March 6, 2019 09:50

Capabilities optimizations

7c52108

Use Lombok Getter

22b4ef9

Inline declaration of default value

4bafea8

freimair force-pushed the refactor_capabilities branch from 1875b88 to 4bafea8 Compare March 6, 2019 08:50

freimair mentioned this pull request Mar 6, 2019

For March 2019 bisq-network/compensation#236

Closed

freimair changed the title ~~[WIP] Refactor capabilities~~ Refactor capabilities and other P2P-related stuff Mar 6, 2019

ManfredKarrer approved these changes Mar 6, 2019

View reviewed changes

ManfredKarrer merged commit 73eea79 into bisq-network:master Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor capabilities and other P2P-related stuff #2469

Refactor capabilities and other P2P-related stuff #2469

freimair commented Feb 26, 2019 •

edited

Loading

ManfredKarrer left a comment

ManfredKarrer Feb 27, 2019

ManfredKarrer Feb 27, 2019

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019 •

edited

Loading

ManfredKarrer Feb 27, 2019

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019 •

edited

Loading

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019

ManfredKarrer Feb 27, 2019

freimair Mar 7, 2019 •

edited

Loading

Refactor capabilities and other P2P-related stuff #2469

Refactor capabilities and other P2P-related stuff #2469

Conversation

freimair commented Feb 26, 2019 • edited Loading

Note

ManfredKarrer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freimair Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freimair Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freimair Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

freimair commented Feb 26, 2019 •

edited

Loading

freimair Mar 7, 2019 •

edited

Loading

freimair Mar 7, 2019 •

edited

Loading

freimair Mar 7, 2019 •

edited

Loading