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

Tentative acceptance protocol #244

Merged
merged 3 commits into from
May 20, 2020

Conversation

ingar
Copy link
Contributor

@ingar ingar commented May 11, 2020

Summary

Switch piece data transfer to a client push.

resolves #192

@ingar ingar marked this pull request as ready for review May 11, 2020 17:47
@ingar ingar requested a review from hannahhoward May 11, 2020 17:49
@codecov-io
Copy link

codecov-io commented May 11, 2020

Codecov Report

Merging #244 into master will decrease coverage by 0.16%.
The diff coverage is 72.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
- Coverage   64.95%   64.80%   -0.15%     
==========================================
  Files          39       40       +1     
  Lines        2285     2332      +47     
==========================================
+ Hits         1484     1511      +27     
- Misses        683      697      +14     
- Partials      118      124       +6     
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <0.00%> (ø)
storagemarket/impl/provider.go 5.54% <0.00%> (ø)
storagemarket/impl/providerstates/provider_fsm.go 93.94% <ø> (-6.06%) ⬇️
storagemarket/impl/providerutils/providerutils.go 95.46% <ø> (+5.63%) ⬆️
storagemarket/types.go 33.34% <ø> (ø)
storagemarket/impl/dtutils/dtutils.go 70.59% <70.59%> (ø)
storagemarket/impl/clientstates/client_states.go 85.89% <83.34%> (-1.00%) ⬇️
...oragemarket/impl/providerstates/provider_states.go 83.07% <83.34%> (-1.75%) ⬇️
storagemarket/impl/clientstates/client_fsm.go 93.34% <100.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99c556b...77d65c9. Read the comment docs.

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Yay for the assert helper and this is mostly there.

However, as written, every deal with get data transfer events for every other deal (and non storage market data transfers)

You need to inspect the voucher in the data transfer state and verify it's a Storage Voucher, and then route it based on the proposal CID. I think this should be done not with a deal specific subscription but rather with a single subscription for all deals that just dispatches events outside the state handler. There's no need for a state handler unless events are triggered internally from the statemachine as a result of the previous transition.

you can look at providerutils/providerutils.go & DataTransferSubscriber for an example

shared_testutil/testutil.go Show resolved Hide resolved
storagemarket/impl/client.go Outdated Show resolved Hide resolved
storagemarket/impl/provider.go Outdated Show resolved Hide resolved
storagemarket/impl/providerstates/provider_states.go Outdated Show resolved Hide resolved
storagemarket/impl/clientstates/client_states.go Outdated Show resolved Hide resolved
@ingar ingar force-pushed the feat/tentative-acceptance-protocol branch 2 times, most recently from bf508c6 to da31069 Compare May 13, 2020 21:34
@ingar ingar requested a review from hannahhoward May 13, 2020 21:42
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Mostly minor cleanups but ones that should be done before merge:

  • remove dead code
  • fix use of provider event on client side
  • fix set of starting states for ClientEventDataTransferFailed
  • add missing test for StartDataTransfer returns err (cause would have caught provider event dispatched)

storagemarket/impl/client.go Outdated Show resolved Hide resolved
storagemarket/impl/clientstates/client_states.go Outdated Show resolved Hide resolved
storagemarket/impl/clientstates/client_fsm.go Outdated Show resolved Hide resolved
storagemarket/impl/provider.go Outdated Show resolved Hide resolved
storagemarket/impl/providerstates/provider_states.go Outdated Show resolved Hide resolved
storagemarket/impl/providerstates/provider_states_test.go Outdated Show resolved Hide resolved
storagemarket/impl/clientstates/client_states_test.go Outdated Show resolved Hide resolved
@ingar ingar requested a review from hannahhoward May 15, 2020 21:08
@ingar ingar force-pushed the feat/tentative-acceptance-protocol branch from 77d65c9 to 82d04c9 Compare May 19, 2020 16:40
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #244 into master will decrease coverage by 0.26%.
The diff coverage is 71.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
- Coverage   65.04%   64.78%   -0.25%     
==========================================
  Files          39       40       +1     
  Lines        2288     2331      +43     
==========================================
+ Hits         1488     1510      +22     
- Misses        683      696      +13     
- Partials      117      125       +8     
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <0.00%> (ø)
storagemarket/impl/provider.go 5.56% <0.00%> (ø)
storagemarket/impl/providerstates/provider_fsm.go 93.94% <ø> (-6.06%) ⬇️
storagemarket/impl/providerutils/providerutils.go 95.46% <ø> (+5.63%) ⬆️
storagemarket/types.go 33.34% <ø> (ø)
storagemarket/impl/dtutils/dtutils.go 70.59% <70.59%> (ø)
storagemarket/impl/clientstates/client_states.go 83.73% <75.00%> (-3.37%) ⬇️
...oragemarket/impl/providerstates/provider_states.go 83.07% <83.34%> (-1.75%) ⬇️
storagemarket/impl/clientstates/client_fsm.go 100.00% <100.00%> (+7.55%) ⬆️
retrievalmarket/impl/blockio/traverser.go 68.89% <0.00%> (-3.33%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 950d428...6b48786. Read the comment docs.

@ingar ingar force-pushed the feat/tentative-acceptance-protocol branch from af9f828 to 6b48786 Compare May 19, 2020 21:56
ingar and others added 3 commits May 20, 2020 16:27
State tests, rebase madness
linting
Fix erroneous state!
remove debugging statement
Remove dead function

WIP PR feedback

Fix client-side manual transfer transitions

Remove dead code

Refactor client state test fake environment initialization

Refactor client state test

Remove redundant Provider state

Allow ClientEventDataTransferFailed event to transition from StorageDealTransferring!
- Remove unused event
- Re-re-refactor client states test
add one state test and fix compile error
@hannahhoward hannahhoward force-pushed the feat/tentative-acceptance-protocol branch from 6b48786 to 04c42dc Compare May 20, 2020 23:36
@hannahhoward hannahhoward merged commit 095b388 into master May 20, 2020
@ingar ingar deleted the feat/tentative-acceptance-protocol branch June 24, 2020 16:47
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.

Storage Provider sends tentative acceptance
4 participants