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

Normalize final states for cancelled retrieval deals #464

Merged
merged 6 commits into from
Dec 4, 2020

Conversation

ingar
Copy link
Contributor

@ingar ingar commented Dec 2, 2020

Problem

A retrieval deal can be cancelled by either the client or provider. The deals, when cancelled, were either going to error states or cancelled states depending on if you were a client or provider. In addition, this state transition was done without cleaning up the tracking, closing channels, etc.

Solution

Make all deals that are "cancelled" by either the client or provider have a final state of DealStatusCancelled, with the reason for the cancel stored in the msg field.

@ingar ingar force-pushed the feat/cancel-retrieval-deals-on-cancelled-transfer branch from ee2bff5 to 2617dcc Compare December 3, 2020 18:10
@codecov-io
Copy link

Codecov Report

Merging #464 (2617dcc) into master (a56856f) will increase coverage by 0.07%.
The diff coverage is 33.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   65.51%   65.58%   +0.07%     
==========================================
  Files          46       46              
  Lines        3198     3198              
==========================================
+ Hits         2095     2097       +2     
+ Misses        877      875       -2     
  Partials      226      226              
Impacted Files Coverage Δ
retrievalmarket/impl/clientstates/client_fsm.go 74.34% <0.00%> (ø)
...etrievalmarket/impl/providerstates/provider_fsm.go 6.46% <ø> (ø)
...ievalmarket/impl/providerstates/provider_states.go 90.25% <100.00%> (ø)
retrievalmarket/impl/dtutils/dtutils.go 80.19% <0.00%> (+1.89%) ⬆️

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 a56856f...2617dcc. Read the comment docs.

@ingar ingar marked this pull request as ready for review December 3, 2020 18:25
@ingar ingar requested a review from dirkmc December 3, 2020 18:25
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

One question but looks great 🎉

DealStatusSendFunds

// DealStatusSendFundsLastPayment indicats the client is now going to send final funds because
Copy link
Contributor

Choose a reason for hiding this comment

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

indicats! 🐱

fsm.Event(rm.ClientEventCancel).FromAny().To(rm.DealStatusCancelling).Action(func(deal *rm.ClientDealState) error {
deal.Message = "Retrieval Cancelled"
deal.Message = "Client cancelled retrieval"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the provider can also cancel the deal - are we guaranteed to only get to this action if the client cancels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when the provider cancels it comes from the data transfer event, translated into the ClientEventProviderCancelled event.

@ingar ingar merged commit 6667494 into master Dec 4, 2020
@ingar ingar deleted the feat/cancel-retrieval-deals-on-cancelled-transfer branch December 4, 2020 19:18
@dirkmc dirkmc mentioned this pull request Dec 8, 2020
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.

3 participants