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

pool: refactor payDividends. #250

Merged
merged 8 commits into from
Oct 9, 2020
Merged

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Sep 17, 2020

This refactors the payDividends function into more targeted helper functions that are easier to test.

@dnldd dnldd force-pushed the refactor_payDividend branch from ba62f16 to e682103 Compare September 21, 2020 23:28
@dnldd dnldd marked this pull request as ready for review September 21, 2020 23:29
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

I have reviewed the changes in paymentmgr.go, but I have not yet looked at the tests - I will review those after these comments are addressed.

pool/paymentmgr.go Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
@dnldd dnldd force-pushed the refactor_payDividend branch from e682103 to 93010ea Compare October 6, 2020 17:38
dnldd added 6 commits October 6, 2020 17:40
This updates payment manager related errors to leverage the
updated error types.
This refactors payDividends into smaller scoped helper
functions that are easier to test.
This adds tests for payDividends and its helper functions.
This adds the --conftimeout config option to allow for
custom tx confirmation timeouts.
This upates the tx confirmation streaming process to be
non-blocking in order to terminate cleanly on context cancellation.
@dnldd dnldd force-pushed the refactor_payDividend branch from 93010ea to 9828e99 Compare October 6, 2020 17:42
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Show resolved Hide resolved
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Just a few minor things in the test file and then this looks good to merge.

pool/paymentmgr_test.go Outdated Show resolved Hide resolved
pool/paymentmgr_test.go Outdated Show resolved Hide resolved
pool/paymentmgr_test.go Outdated Show resolved Hide resolved
pool/paymentmgr_test.go Outdated Show resolved Hide resolved
pool/paymentmgr_test.go Outdated Show resolved Hide resolved
initialAccountPayments := xValue + yValue
updatedAccountPaymentsPlusTxFee := out[xAddr] + out[yAddr] + txFee
if initialAccountPayments-updatedAccountPaymentsPlusTxFee <= maxRoundingDiff {
t.Fatalf("initial account payment total %v to be equal to updated "+
Copy link
Member

Choose a reason for hiding this comment

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

cancel()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, applyTxFees is not using the context, therefore does not need the context cancelled for a clean shutdown.

@dnldd dnldd merged commit 45b9b20 into decred:master Oct 9, 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.

2 participants