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

Refactor Storage Provider to FSM Module #145

Merged
merged 5 commits into from Mar 13, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

Convert the storage provider to the FSM module, add many unit tests, generally refactor to make code more manageable and legible

Implementation

  • Refactor all provider states to the FSM module
  • Factor our some common utility functions
  • Factor out storage ask code from storage provider -- this is a fairly discreet unity of functionality which makes the storage provider code much larger
  • Factor out common code on client & provider used to manage connections
  • Add unit tests to: new provider FSM code, utility functions, factored out modules

@hannahhoward hannahhoward requested a review from ingar March 11, 2020 04:01
@hannahhoward hannahhoward force-pushed the feat/storage-market-provider-fsm branch 2 times, most recently from 2777641 to 4c3c9ef Compare March 11, 2020 04:36
@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

Merging #145 into master will increase coverage by 3.21%.
The diff coverage is 93.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   78.86%   82.07%   +3.21%     
==========================================
  Files          27       33       +6     
  Lines        1225     1561     +336     
==========================================
+ Hits          966     1281     +315     
- Misses        189      200      +11     
- Partials       70       80      +10
Impacted Files Coverage Δ
storagemarket/types.go 100% <ø> (ø) ⬆️
storagemarket/impl/clientutils/clientutils.go 100% <ø> (ø)
storagemarket/impl/clientstates/client_fsm.go 100% <ø> (ø) ⬆️
storagemarket/impl/providerstates/provider_fsm.go 100% <100%> (ø)
storagemarket/impl/connmanager/connmanager.go 100% <100%> (ø)
storagemarket/impl/storedask/storedask.go 81.43% <81.43%> (ø)
storagemarket/impl/providerutils/providerutils.go 88.24% <88.24%> (ø)
...oragemarket/impl/providerstates/provider_states.go 97.13% <97.13%> (ø)
... and 4 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 5201b29...7e1773c. Read the comment docs.

Copy link
Contributor

@ingar ingar left a comment

Choose a reason for hiding this comment

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

Looks good.

One minor point: was a little confused by the use of the storagemarket.StorageDealProposalAccepted state. On the provider, it's used to indicate sort of a "conditional acceptance", pending data transfer and validation and funds. On the Client it means something different (staged I think). Not a huge deal, but it could be confusing if you just looked at the existing deal states without looking through the FSM.

@hannahhoward
Copy link
Collaborator Author

@ingar -- my next step is to finally do that conditional acceptance ticket (where the provider sends conditional accepts and the client creates the data transfer)

Extracts ConnManager, StoredAsk, reorgs files, and cleans up provider code down to one file
add unit tests for provider state fsm, utils, stored ask, connection manager
@hannahhoward hannahhoward force-pushed the feat/storage-market-provider-fsm branch from 4c3c9ef to 7e1773c Compare March 13, 2020 18:23
@hannahhoward hannahhoward merged commit d682069 into master Mar 13, 2020
@hannahhoward hannahhoward deleted the feat/storage-market-provider-fsm branch April 30, 2020 21:33
@dirkmc dirkmc mentioned this pull request Mar 22, 2021
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.

None yet

3 participants