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: decouple components. #143

Merged
merged 15 commits into from
Jan 9, 2020
Merged

pool: decouple components. #143

merged 15 commits into from
Jan 9, 2020

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Dec 18, 2019

This decouples components initially bundled with the pool hub in order to better test them and ultimately improve test coverage for the pool.

This reactors  functionality from various structs
namely shares, db, hub and payments. This also
fixes lint related issues.
@dnldd dnldd marked this pull request as ready for review January 2, 2020 22:33
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've reviewed this and dropped some comments, but I cannot make any promises about the completeness of the review. This is a massive changeset and was very painful to review.

Please consider breaking future changes into multiple, smaller PRs for ease of reviewing and to reduce likelihood of defects sneaking through.

Also, please do not rebase/squash/force-push this PR, because it will make reviewing further changes even more painful.

dcrpool.go Outdated Show resolved Hide resolved
dcrpool.go Show resolved Hide resolved
gui/gui.go Show resolved Hide resolved
gui/gui.go Outdated Show resolved Hide resolved
pool/db.go Outdated Show resolved Hide resolved
pool/paymentmgr.go Show resolved Hide resolved
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_test.go Outdated Show resolved Hide resolved
@dnldd
Copy link
Member Author

dnldd commented Jan 4, 2020

Hmm, I mostly split the commits based on the introduced component or its associated tests. I'm curious to know what made reviewing each commit difficult.

This simplifies payment requests by only requiring
the address of an account instead of an account id.
pool/hub.go Show resolved Hide resolved
// addPaymentRequest creates a payment request from the provided account
// if not already requested.
func (pm *PaymentMgr) addPaymentRequest(addr string) error {
_, err := dcrutil.DecodeAddress(addr, pm.cfg.ActiveNet)
Copy link
Member

Choose a reason for hiding this comment

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

There are three places where Account() is preceeded by a call to DecodeAddress().

Perhaps DecodeAddress can be moved inside of Account()? It would remove duplication and ensure the a new caller of Account doesnt forget to call DecodeAddress first

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, rectifying.

This updates AccountID to check the provided
address is valid for the active network before
generating an id.
@dnldd dnldd merged commit 016a7b9 into decred:master Jan 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