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

Nonblocking storage deals [#80] #194

Merged
merged 4 commits into from
Apr 28, 2020
Merged

Conversation

ingar
Copy link
Contributor

@ingar ingar commented Apr 20, 2020

Summary

Break apart states such that chain mutating operations are in their own state and don't block. This is groundwork for being able to resume storage deals in progress.

Resolves #80

@ingar ingar force-pushed the feat/noblock-storage-deals branch from b2c508a to c1622a6 Compare April 21, 2020 23:03
@ingar ingar marked this pull request as ready for review April 21, 2020 23:19
@ingar ingar force-pushed the feat/noblock-storage-deals branch from c1622a6 to fe29b07 Compare April 21, 2020 23:20
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.

Most of the comments are non-blocking but I'd like to see the missing tests get written.

However, there's one big pice I had imagined being part of this that's missing -- breaking up publishing deals. This seems like potentially the most harmful of the "double submit" cases and I think we should cover it.

I think that covers all the messages this module directly submits to the chain, which was the original scope of this ticket.

However, I'm ok with merging this (it's a definite improvement and works) and deferring to a seperate ticket, as long as we make the seperate ticket.

@ingar ingar force-pushed the feat/noblock-storage-deals branch 2 times, most recently from 66de90d to 3722225 Compare April 27, 2020 18:12
@ingar ingar requested a review from hannahhoward April 27, 2020 18:15
@hannahhoward hannahhoward changed the base branch from master to development April 27, 2020 23:18
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.

LGTM but let's change the PublishDeals api on the node interface before we merge this.

storagemarket/impl/providerstates/provider_states.go Outdated Show resolved Hide resolved
@ingar ingar force-pushed the feat/noblock-storage-deals branch from 3722225 to dc5e001 Compare April 27, 2020 23:50
@codecov-io
Copy link

Codecov Report

Merging #194 into development will decrease coverage by 0.14%.
The diff coverage is 66.67%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #194      +/-   ##
===============================================
- Coverage        67.15%   67.02%   -0.13%     
===============================================
  Files               40       40              
  Lines             2161     2231      +70     
===============================================
+ Hits              1451     1495      +44     
- Misses             601      626      +25     
- Partials           109      110       +1     
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <0.00%> (ø)
storagemarket/impl/provider.go 5.86% <0.00%> (-0.33%) ⬇️
storagemarket/types.go 33.34% <ø> (ø)
...oragemarket/impl/providerstates/provider_states.go 87.96% <88.89%> (+0.32%) ⬆️
storagemarket/impl/clientstates/client_states.go 93.66% <93.34%> (+1.06%) ⬆️
storagemarket/impl/clientstates/client_fsm.go 100.00% <100.00%> (ø)
storagemarket/impl/providerstates/provider_fsm.go 100.00% <100.00%> (ø)

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 09fa1c0...dc5e001. Read the comment docs.

@hannahhoward hannahhoward merged commit 34ac4b7 into development Apr 28, 2020
hannahhoward pushed a commit that referenced this pull request Apr 30, 2020
* Add more states in Client and Provider FSM representing async ops:

- Waiting for storage market funds to appear
- Waiting for deals to be published

* Publishing deals doesn't block

* WaitForFunding state tests for Client, Provider

* Remove deal id from the provider node api PublishDeals
@hannahhoward hannahhoward deleted the feat/noblock-storage-deals branch April 30, 2020 21:34
@dirkmc dirkmc mentioned this pull request May 6, 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.

Storage Market should not block on chain operations
3 participants