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

refactor(coinjoin): refactor Wallet #191

Merged
merged 17 commits into from
Feb 3, 2023

Conversation

HashEngineering
Copy link
Collaborator

@HashEngineering HashEngineering commented Jan 18, 2023

Issue being fixed or feature implemented

What was done?

  • Refactor CoinJoin keychain code into CoinJoinExtension
  • Add CoinJoin send function
  • Fix some issues with connectivity

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@HashEngineering HashEngineering force-pushed the feature-coinjoin-refactor branch from 64dac26 to 6180554 Compare January 18, 2023 18:12
@HashEngineering HashEngineering changed the base branch from master to feature-coinjoin January 18, 2023 18:13
@HashEngineering HashEngineering changed the title refactor(coinjoin): refactor refactor(coinjoin): refactor Wallet Jan 18, 2023
@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Base: 46.44% // Head: 46.47% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (2583b05) compared to base (8ac47d8).
Patch coverage: 47.86% of modified lines in pull request are covered.

Additional details and impacted files
@@                  Coverage Diff                   @@
##             feature-coinjoin     #191      +/-   ##
======================================================
+ Coverage               46.44%   46.47%   +0.02%     
- Complexity               6002     6093      +91     
======================================================
  Files                     416      423       +7     
  Lines                   44643    45181     +538     
  Branches                 6249     6294      +45     
======================================================
+ Hits                    20734    20997     +263     
- Misses                  21588    21856     +268     
- Partials                 2321     2328       +7     
Impacted Files Coverage Δ
...va/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java 38.46% <0.00%> (-20.37%) ⬇️
...ava/org/bitcoinj/coinjoin/CoinJoinSendRequest.java 0.00% <0.00%> (ø)
...bitcoinj/coinjoin/UnmixedZeroConfCoinSelector.java 0.00% <0.00%> (ø)
...coinj/coinjoin/progress/MixingProgressTracker.java 0.00% <0.00%> (ø)
.../org/bitcoinj/coinjoin/utils/CoinJoinReporter.java 0.00% <0.00%> (ø)
.../org/bitcoinj/coinjoin/utils/KeyHolderStorage.java 81.25% <ø> (ø)
...a/org/bitcoinj/coinjoin/utils/ProTxToOutpoint.java 72.72% <ø> (ø)
core/src/main/java/org/bitcoinj/core/Utils.java 57.97% <0.00%> (-0.17%) ⬇️
...a/org/bitcoinj/coinjoin/utils/CoinJoinManager.java 57.14% <26.66%> (-8.72%) ⬇️
core/src/main/java/org/bitcoinj/wallet/Protos.java 30.44% <29.60%> (-0.06%) ⬇️
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@HashEngineering HashEngineering marked this pull request as ready for review January 30, 2023 19:05
@HashEngineering HashEngineering self-assigned this Jan 30, 2023
Comment on lines +516 to +521
message CoinJoin {
// key chain
repeated Key key = 1;
// last value of rounds from mixing
required int32 rounds = 2;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rounds is used to calculate the balance of coinjoin coins.

if all coins were mixed for 4 rounds, but the rounds = 5 then the coinjoin balance will be zero. This value needs to persist.

Comment on lines +47 to +48
public class WalletEx extends Wallet {
private static final Logger log = LoggerFactory.getLogger(WalletEx.class);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WalletEx is a temporary name.

Comment on lines +20501 to +20506
public interface CoinJoinOrBuilder extends
// @@protoc_insertion_point(interface_extends:wallet.CoinJoin)
com.google.protobuf.MessageLiteOrBuilder {

/**
* <pre>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why the bitcoinj developers like this autogenerated file to be committed into the repo.

@@ -93,7 +93,7 @@ public void setUp(BlockStore blockStore) throws Exception {
// Reduce the number of keys we need to work with to speed up these tests.
KeyChainGroup kcg = KeyChainGroup.builder(UNITTEST).lookaheadSize(4).lookaheadThreshold(2).
fromRandom(Script.ScriptType.P2PKH).build();
wallet = new Wallet(UNITTEST, kcg);
wallet = createWallet(kcg);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to allow derived classes to create their own wallet objects.

Copy link
Member

@Syn-McJ Syn-McJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like great work.

@HashEngineering HashEngineering merged commit 476f800 into feature-coinjoin Feb 3, 2023
@HashEngineering HashEngineering deleted the feature-coinjoin-refactor branch March 7, 2023 06:27
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.

2 participants