-
Notifications
You must be signed in to change notification settings - Fork 59
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
Use go-statemachine + FSMs in retrieval market #124
Conversation
convert retrieval market to using go-statemachine, specifically the FSM module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay, I don't see anything blocking.
} | ||
|
||
// Process Blocks | ||
totalProcessed := uint64(0) | ||
completed := false | ||
completed := deal.Status == rm.DealStatusBlocksComplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if completed is true, do we still need to consume Blocks? Or will response.Blocks be empty and the for loop never executes? My memory is fuzzy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be empty but it's a good point -- we really shouldn't be consuming blocks period once all blocks are consumed -- arguably if anything is in response.Blocks it should error at that point.
if err != nil { | ||
return errorFunc(xerrors.Errorf("consuming block: %w", err)) | ||
return ctx.Event(rm.ClientEventConsumeBlockFailed, err) | ||
} | ||
totalProcessed += processed | ||
if done { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't you just say processed, completed, err := environment.ConsumeBlock...
and then if completed { break }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yea, probably :) had not considered
@@ -28,133 +26,100 @@ func errorFunc(err error) func(*rm.ClientDealState) { | |||
|
|||
// ClientHandlerFunc is a function that handles a client deal being in a specific state | |||
// It processes the state and returns a modification function for a deal | |||
type ClientHandlerFunc func(ctx context.Context, environment ClientDealEnvironment, deal rm.ClientDealState) func(*rm.ClientDealState) | |||
type ClientHandlerFunc func(ctx fsm.Context, environment ClientDealEnvironment, deal rm.ClientDealState) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is no longer returning a func, "Func" should be removed from the name. Also the comment should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
require.Empty(t, dealState.Message) | ||
require.Equal(t, dealState.PaymentRequested, abi.NewTokenAmount(0)) | ||
require.Equal(t, dealState.FundsSpent, big.Add(defaultFundsSpent, defaultPaymentRequested)) | ||
require.Equal(t, dealState.BytesPaidFor, defaultTotalReceived) | ||
require.Equal(t, dealState.CurrentInterval, defaultCurrentInterval+defaultIntervalIncrease) | ||
require.Equal(t, dealState.Status, retrievalmarket.DealStatusCompleted) | ||
require.Equal(t, dealState.Status, retrievalmarket.DealStatusFinalizing) | ||
}) | ||
|
||
t.Run("not enough funds left", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would really benefit from either table tests or helpers to reduce copy-pasta, or both.
}) | ||
} | ||
|
||
func TestProcessNextResponse(t *testing.T) { | ||
ctx := context.Background() | ||
node := testnodes.NewTestRetrievalClientNode(testnodes.TestRetrievalClientNodeParams{}) | ||
|
||
eventMachine, err := fsm.NewEventMachine(retrievalmarket.ClientDealState{}, "Status", clientstates.ClientEvents) | ||
require.NoError(t, err) | ||
environment := func(netParams testnet.TestDealStreamParams, | ||
responses []consumeBlockResponse) clientstates.ClientDealEnvironment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above re: table tests
update statemachine to latest and address PR comments
Goals
Implementation
provider_fsm.go
&client_fsm.go
For discussion