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

Test transaction processing code #2092

Merged

Conversation

@lontivero
Copy link
Collaborator

commented Aug 8, 2019

Move the transaction processing code from WalletService to a new testeable TransactionProcessor class and creates tests for each scenario. This is important because this piece of code is complex and has been modified many times in the past to improve performance, add support for RBF, check for transaction relevance, detect coinjoins, fix TransactionCache status and other small changes and fixes; and it is also a critical in the sense that a bug here can have big consecuences. That's why IMO we need unit tests here.

@benthecarman benthecarman referenced this pull request Aug 8, 2019

@lontivero lontivero force-pushed the lontivero:Features/Extract-Transaction-Processing branch from c30d3fb to a147f1f Aug 13, 2019

@lontivero lontivero changed the title Test transaction processing code (refactoring) Test transaction processing code Aug 13, 2019

@lontivero lontivero force-pushed the lontivero:Features/Extract-Transaction-Processing branch from a147f1f to 8184014 Aug 13, 2019

@lontivero lontivero marked this pull request as ready for review Aug 13, 2019

@lontivero lontivero requested a review from molnard Aug 13, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

Concept ACK. I'm not sure how to review it, because of the code moving, but I guess it's just copypaste and you didn't touch any logic, right? If so, it can be merged. One thing that's missing: TransactionProcessorTests should be added to CI.

@lontivero lontivero force-pushed the lontivero:Features/Extract-Transaction-Processing branch from 3959a62 to 9e3ef71 Aug 14, 2019

nopara73 added 5 commits Aug 15, 2019
@nopara73
Copy link
Collaborator

left a comment

ACK. This can be merged.

@molnard
Copy link
Collaborator

left a comment

Reviewed with @nopara73.

@lontivero lontivero dismissed stale reviews from molnard and nopara73 via c0ca1d8 Aug 15, 2019

@lontivero

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 15, 2019

@molnard please take a look at my latest commit please, I think is trivial.

@nopara73
Copy link
Collaborator

left a comment

Trivial.

@lontivero lontivero merged commit 02e148c into zkSNACKs:master Aug 15, 2019

3 of 4 checks passed

Wasabi.Osx #20190815.7 failed
Details
CodeFactor No issues found.
Details
Wasabi.Linux #20190815.7 succeeded
Details
Wasabi.Windows #20190815.7 succeeded
Details

@lontivero lontivero deleted the lontivero:Features/Extract-Transaction-Processing branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.