Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Bitcoinj usage refactor example #4269

Closed

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented May 18, 2020

Related project proposal: bisq-network/projects#30

This example PR is meant to be read commit by commit.

It's a simple refactor of WalletConfig focusing on moving PeerGroup related code. I chose PeerGroup, because it's probably the simplest important group of BitcoinJ endpoints. Moving code is the simplest (and least interesting) form of refactoring, but while doing it (similarly to shuffling puzzle pieces while solving a puzzle) you come up with more interesting refactorings, though it's a time-intensive process. This example is about that shuffling.

There are a few quirks here that are still to be ironed out:

  • Naming of low.PeerGroup clashes with BitcoinJ's PeerGroup. Might come up with something better;
  • The whole PeerGroup extends PeerGroupProxy scheme has its upsides, but a few downsides too. Might come up with something better;
  • PeerGroupProxy name clashes with Socks5Proxy and the like; naming isn't worked out in general;
  • Part of the logic moved from WalletConfig to low.PeerGroup in this example might actually do better left in WalletConfig, which will require dissecting it further.

dmos62 added 6 commits May 18, 2020 16:38
Involves turning implicit dependencies into explicit method arguments.
Code inside the method is only changed so much as to allow the move.
Reduce API access to reflect which BitcoinJ endpoints are used within
low.PeerGroup and which outside it.
Realised that these constructors have to stay on the subclass. Would
like to put them with the rest of BitcoinJ endpoints. This
subclass+superclass setup is something I just came up with, might find
something better.
@dmos62 dmos62 closed this May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant