Storage Client Statemachine Refactor #136
Conversation
Refactor the storage client to use finite state machines - also add some more fine grain state to the beginning of deals on the client
Setup testing infrastructure for testsnodes and add several unit tests for different client states
hannahhoward
force-pushed
the
feat/storage-market-state-machine-testing
branch
from
b24b172
to
bcad340
Compare
| deal := &storagemarket.ClientDeal{ | ||
| ProposalCid: proposalNd.Cid(), | ||
| ClientDealProposal: *clientDealProposal, | ||
| State: storagemarket.StorageDealUnknown, |
There was a problem hiding this comment.
I know that these states predate this PR, but I find the use of StorageDealUnknown too overloaded. Maybe in another PR we can separate this out, since it currently means StorageDealInitial, StorageDealWaitingForSomeEvent, StorageDealUnknown, etc.
There was a problem hiding this comment.
I agree will fix in future PR
| c.handle(ctx, deal, c.sealing, storagemarket.StorageDealNoUpdate) | ||
| // TODO: StorageDealActive -> watch for faults, expiration, etc. | ||
| } | ||
| func (c *Client) Run(ctx context.Context) { |
There was a problem hiding this comment.
I guess this can boot up in-progress deals when restarting the node?
There was a problem hiding this comment.
yes, "in the future" :)
|
|
||
| // ClientEvents are the events that can happen in a storage client | ||
| var ClientEvents = fsm.Events{ | ||
| fsm.Event(storagemarket.ClientEventOpen). |
There was a problem hiding this comment.
I don't quite remember why we send an event like this. Is it because we need an arbitrary event for the FSM to start running? Doesn't Begin() do that?
There was a problem hiding this comment.
no Begin() does not do that -- it sets an initial state, but there is no implied initial event
| // ClientEvents are the events that can happen in a storage client | ||
| var ClientEvents = fsm.Events{ | ||
| fsm.Event(storagemarket.ClientEventOpen). | ||
| From(storagemarket.StorageDealUnknown).To(nil), |
There was a problem hiding this comment.
Should this be .ToNoChange()?
There was a problem hiding this comment.
right. #whenYouForGetYourOwnDSL
| return nil | ||
| }), | ||
| fsm.Event(storagemarket.ClientEventDealProposed). | ||
| From(storagemarket.StorageDealFundsEnsured).To(storagemarket.StorageDealValidating), |
There was a problem hiding this comment.
Ugh, I wonder what the race condition recovery looks like here with multiple simultaneous deals..
There was a problem hiding this comment.
well the upside is the provider validates it too... so maybe they'll catch it? But you are right that the logic ought to be ensure funds for all deals in progress... dunno what that looks like.
| return ctx.Trigger(storagemarket.ClientEventDealProposed) | ||
| } | ||
|
|
||
| // VerifyDealResponse reads and verifies the response from the provider to the proposed deal |
There was a problem hiding this comment.
Should read, "VerifyDealResponse waits, waits, and waits, and waits some more, and then reads and verifies..." :)
There was a problem hiding this comment.
omg... I am very ready to send that intent to accept a lot earlier and switch up the data transfer initiation... just gotta finish this here refactor first.
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
===========================================
+ Coverage 37.13% 56.21% +19.09%
===========================================
Files 38 31 -7
Lines 2869 2055 -814
===========================================
+ Hits 1065 1155 +90
+ Misses 1506 614 -892
+ Partials 298 286 -12
Continue to review full report at Codecov.
|
Goals
Make the storage client
Implementation