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

Factor proof calculation out of storage miner for testability. #2972

Merged
merged 1 commit into from Jun 21, 2019

Conversation

@anorth
Copy link
Contributor

commented Jun 20, 2019

Interesting tests will come later when some of the outstanding TODOs are addressed.

I think the boundary that I've drawn could actually be pushed a bit further, incorporating much of the logic in Miner.OnNewHeaviestTipSet. But right now I need to test late fee calculations.

@anorth anorth requested review from shannonwells and laser Jun 20, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 20, 2019

Codecov Report

Merging #2972 into master will decrease coverage by <1%.
The diff coverage is 25%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2972   +/-   ##
======================================
- Coverage      43%     43%   -1%     
======================================
  Files         212     213    +1     
  Lines       12749   12756    +7     
======================================
- Hits         5558    5547   -11     
- Misses       6339    6362   +23     
+ Partials      852     847    -5
Impacted Files Coverage Δ
protocol/storage/deals_awaiting_seal.go 95% <ø> (ø) ⬆️
protocol/storage/prover.go 53% <53%> (ø)
protocol/storage/miner.go 25% <6%> (+1%) ⬆️
commands/id.go 0% <0%> (-16%) ⬇️
tools/migration/internal/runner.go 77% <0%> (-8%) ⬇️
gengen/util/gengen.go 43% <0%> (-6%) ⬇️
protocol/hello/hello.go 84% <0%> (+6%) ⬆️
@laser
laser approved these changes Jun 20, 2019
Copy link
Contributor

left a comment

A wonderful refactor.

protocol/storage/deals_awaiting_seal.go Outdated Show resolved Hide resolved
protocol/storage/prover.go Show resolved Hide resolved
protocol/storage/prover.go Show resolved Hide resolved
protocol/storage/miner_test.go Outdated Show resolved Hide resolved
protocol/storage/miner.go Show resolved Hide resolved
protocol/storage/prover.go Outdated Show resolved Hide resolved

if err := cbu.NewMsgWriter(s).WriteMsg(resp); err != nil {
log.Errorf("failed to write query response: %s", err)
res, err := sm.node.SectorBuilder().GeneratePoSt(req)

This comment has been minimized.

Copy link
@shannonwells

shannonwells Jun 20, 2019

Contributor

Miner only needs SectorBuilder and BlockService from node. I wonder if there is a good way to do this where miner doesn't have to access node. I'm saying this b/c I'm not sure how much I like having Miner be the default ProofCalculator and ProofReader rather than their being separate (either separate structs or in a separate package), since the SectorBuilder is the one that generates the PoSt. It feels a little like mixing concerns, OTOH what else is concerned with coming up with proofs but a Miner....

This comment has been minimized.

Copy link
@anorth

anorth Jun 21, 2019

Author Contributor

I agree. I resisted the urge to clean up the miner's deps in this change and think I will factor out these methods in the future.

Factor proof calculation out of storage miner for testability.
Interesting tests will come later when some of the outstanding TODOs are addressed.

@anorth anorth force-pushed the anorth/prefactor branch from 75428d1 to 2e0c9c3 Jun 21, 2019

@anorth anorth merged commit 29c54df into master Jun 21, 2019

5 checks passed

ci/circleci: build_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: deps_linux Your tests passed on CircleCI!
Details
ci/circleci: functional_test_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: integration_test_linux Your tests passed on CircleCI!
Details
ci/circleci: unit_test_linux Your tests passed on CircleCI!
Details
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.