Add DealStages to track and log Deal status updates #502

nonsense · 2021-03-04T11:11:21Z

Closes #501

raulk · 2021-03-09T12:37:43Z

storagemarket/dealstatus.go

@@ -141,3 +141,37 @@ var DealStates = map[StorageDealStatus]string{
 	StorageDealClientTransferRestart:        "StorageDealClientTransferRestart",
 	StorageDealProviderTransferAwaitRestart: "StorageDealProviderTransferAwaitRestart",
 }
+
+// DealStatesDescriptions maps StorageDealStatus codes to string description for better UX
+var DealStatesDescriptions = map[StorageDealStatus]string{


I'd merge this with the above map, making it a map[StorageDealStatus]struct{Name: string, Desc: string}

The DealStates map is already used in a lot of places across the code so if you unify the maps, may be a good idea to add an init() function that also recreates the DealStates map

The DealStates map is already used in a lot of places across the code

This was the main reason I created a new map.

I suggest we do that refactor as a next step, as I am sure we will want to adjust messages and descriptions very soon.

raulk

There is a Notifier primitive in go-statemachine/fsm that we could use if we want to move the user friendly messages away from the state machine and into Lotus. Just a thought that came up while discussing with @nonsense today.

https://pkg.go.dev/github.com/filecoin-project/go-statemachine/fsm#Notifier

codecov-io · 2021-03-18T16:16:38Z

Codecov Report

Merging #502 (d96bb00) into master (d5b1990) will increase coverage by 0.42%.
The diff coverage is 85.14%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   65.41%   65.83%   +0.42%     
==========================================
  Files          51       51              
  Lines        3431     3505      +74     
==========================================
+ Hits         2244     2307      +63     
- Misses        958      969      +11     
  Partials      229      229

Impacted Files	Coverage Δ
storagemarket/impl/client.go	`20.08% <0.00%> (-0.07%)`	⬇️
storagemarket/impl/clientstates/client_fsm.go	`81.65% <79.17%> (-1.08%)`	⬇️
storagemarket/types.go	`86.21% <100.00%> (+86.21%)`	⬆️

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 d5b1990...d96bb00. Read the comment docs.

dirkmc

Looking good, just some suggestions on re-wording the messages

storagemarket/impl/clientstates/client_fsm.go

storagemarket/types.go

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Fix/deal stages nil

…markets into add-deal-stages

raulk · 2021-03-25T12:45:35Z

storagemarket/impl/clientstates/client_fsm.go

-		From(storagemarket.StorageDealFailing).To(storagemarket.StorageDealError),
+		From(storagemarket.StorageDealFailing).To(storagemarket.StorageDealError).
+		Action(func(deal *storagemarket.ClientDeal) error {
+			deal.AddLog("")


Is this empty string intended?

Yes, so that we add the given stage with a timestamp to the event log, even if we don't want to add any message.

storagemarket/types.go

raulk · 2021-03-25T12:47:01Z

storagemarket/types_test.go

+func TestDealStagesNil(t *testing.T) {
+	var ds *storagemarket.DealStages
+	ds.GetStage("none")
+	ds.AddStageLog("MyStage", "desc", "duration", "msg")


Anything that we want to assert on?

raulk · 2021-03-25T13:23:34Z

storagemarket/types.go

+// EXPERIMENTAL; subject to change.
+func (ds *DealStages) AddStageLog(stage, description, expectedDuration, msg string) {
+	if ds == nil {
+		return


We don't want to create a DealStages instance here? I guess it would allow us to display partial logs for deals that were in progress before this code got deployed?

if ds == nil { *ds = DealStages{} }

Actually, this is not possible. We would need to return the new DealStages object, similar to how hashicorp/go-multierror.Append works: https://pkg.go.dev/github.com/hashicorp/go-multierror#Append

This is done for backward compatibility with deals that are created with earlier versions - in these cases we just don't want to have to have DealStages.

nonsense force-pushed the add-deal-stages branch 2 times, most recently from 68ce94b to 5ef2fe8 Compare March 4, 2021 11:45

raulk reviewed Mar 9, 2021

View reviewed changes

raulk added the x/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label Mar 9, 2021

Add DealStages field to track and keep history of lifecycle of deal

1de66af

nonsense force-pushed the add-deal-stages branch from f0adf33 to 1de66af Compare March 18, 2021 12:56

nonsense mentioned this pull request Mar 18, 2021

add inspect-deal command to lotus client filecoin-project/lotus#5833

Merged

fixup

6458ab5

nonsense force-pushed the add-deal-stages branch from c0390f6 to 6458ab5 Compare March 18, 2021 15:38

fixup

d96bb00

dirkmc reviewed Mar 18, 2021

View reviewed changes

nonsense and others added 13 commits March 19, 2021 13:23

Update storagemarket/impl/clientstates/client_fsm.go

ea81924

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

779071d

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

36bdc80

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

3a26ef0

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

9fb4cd6

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

a9a76d3

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

093be6f

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

86f83ba

Co-authored-by: dirkmc <dirkmdev@gmail.com>

decrease log level to debug

b6c677d

Update storagemarket/impl/clientstates/client_fsm.go

afdc591

Co-authored-by: dirkmc <dirkmdev@gmail.com>

Update storagemarket/impl/clientstates/client_fsm.go

074a8a3

Co-authored-by: dirkmc <dirkmdev@gmail.com>

explicit set of log with empty value

00df6da

fix test

a848de8

nonsense force-pushed the add-deal-stages branch from 58d0bf7 to a848de8 Compare March 19, 2021 11:35

nonsense marked this pull request as ready for review March 19, 2021 11:42

dirkmc and others added 2 commits March 19, 2021 15:34

fix: dont panic when adding log to nil DealStages

d9118f7

Merge pull request #506 from filecoin-project/fix/deal-stages-nil

c190288

Fix/deal stages nil

dirkmc approved these changes Mar 19, 2021

View reviewed changes

nonsense and others added 3 commits March 23, 2021 14:00

resolved conflicts

6e52629

Merge branch 'add-deal-stages' of github.com:filecoin-project/go-fil-…

dfc7924

…markets into add-deal-stages

Merge branch 'master' into add-deal-stages

5d6a7df

raulk reviewed Mar 25, 2021

View reviewed changes

add godocs to deal stages objects.

3f13328

raulk force-pushed the add-deal-stages branch from 39106a7 to 3f13328 Compare March 25, 2021 13:31

raulk approved these changes Mar 25, 2021

View reviewed changes

Merge branch 'master' into add-deal-stages

c7ea918

dirkmc merged commit 39a8025 into master Mar 30, 2021

dirkmc deleted the add-deal-stages branch March 30, 2021 07:57

dirkmc mentioned this pull request Mar 30, 2021

release: v1.2.4 #524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DealStages to track and log Deal status updates #502

Add DealStages to track and log Deal status updates #502

nonsense commented Mar 4, 2021 •

edited by raulk

Loading

raulk Mar 9, 2021

dirkmc Mar 9, 2021

nonsense Mar 19, 2021

raulk left a comment

codecov-io commented Mar 18, 2021

dirkmc left a comment

raulk Mar 25, 2021

nonsense Mar 29, 2021

raulk Mar 25, 2021

raulk Mar 25, 2021

raulk Mar 25, 2021

nonsense Mar 29, 2021 •

edited

Loading

Add DealStages to track and log Deal status updates #502

Add DealStages to track and log Deal status updates #502

Conversation

nonsense commented Mar 4, 2021 • edited by raulk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2021

Codecov Report

dirkmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

nonsense commented Mar 4, 2021 •

edited by raulk

Loading

nonsense Mar 29, 2021 •

edited

Loading