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

add lotus client cancel-retrieval cmd to lotus CLI #5871

Merged
merged 7 commits into from
Mar 31, 2021

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Mar 24, 2021

Fixes #5870


Added by @raulk:

This command might seem redudant with lotus client cancel-transfer. However:

  1. This cancels the retrieval deal no matter which stage it's at (transfer or not).
  2. Retrieval currently has some edge cases where cancelling the transfer will not cancel the retrieval deal.
  3. We should probably get rid of lotus client cancel-transfer, and instead rely on the commands to cancel the deals themselves, which would terminate the deal regardless of the stage.

@nonsense nonsense force-pushed the nonsense/add-cancel-retrieval-deal-cmd branch from 336648d to 03c9422 Compare March 24, 2021 15:45
@nonsense nonsense force-pushed the nonsense/add-cancel-retrieval-deal-cmd branch from 03c9422 to a202f9d Compare March 24, 2021 16:18
@nonsense nonsense requested a review from dirkmc March 24, 2021 17:40
@nonsense nonsense marked this pull request as ready for review March 24, 2021 17:42
@nonsense
Copy link
Member Author

  1. test and test-short seem flaky and I think failures are not related.

  2. After running make docsgen, I get three files that are modified:

	modified:   build/openrpc/full.json.gz
	modified:   build/openrpc/miner.json.gz
	modified:   build/openrpc/worker.json.gz

Not sure if I should be replacing these as part of the PR?

@dirkmc dirkmc mentioned this pull request Mar 24, 2021
11 tasks
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

2 comments

(needs make gen)

cli/client.go Outdated Show resolved Hide resolved
cli/client.go Outdated Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor

@magik6k @dirkmc Have addressed the review comments.

But, on running make gen, I seem to be running into some weird import error:

# github.com/filecoin-project/lotus/api/apistruct
api/apistruct/struct.go:6:1: imported and not used: "bytes"
api/apistruct/struct.go:8:1: imported and not used: "encoding/json"
api/apistruct/struct.go:9:1: imported and not used: "fmt"
api/apistruct/struct.go:19:1: imported and not used: "github.com/filecoin-project/go-state-types/big"
api/apistruct/struct.go:23:1: imported and not used: "github.com/filecoin-project/lotus/chain/actors/builtin"
api/apistruct/struct.go:27:1: imported and not used: "github.com/filecoin-project/lotus/chain/actors/builtin/power"
api/apistruct/struct.go:35:1: market redeclared as imported package name
	previous declaration at api/apistruct/struct.go:24:1
api/apistruct/struct.go:35:1: imported and not used: "github.com/filecoin-project/specs-actors/v2/actors/builtin/market"
api/apistruct/struct.go:43:1: imported and not used: "github.com/libp2p/go-libp2p-pubsub" as pubsub
api/apistruct/struct.go:44:1: imported and not used: "github.com/multiformats/go-multiaddr" as multiaddr
api/apistruct/struct.go:44:1: too many errors
make: *** [type-gen] Error 2

What should I do to fix this ?

@dirkmc
Copy link
Contributor

dirkmc commented Mar 26, 2021

I was able to successfully run make gen and pushed the changes

@dirkmc
Copy link
Contributor

dirkmc commented Mar 26, 2021

It wasn't possible to add the command as a subcommand of lotus client retrieve because lotus client retrieve is itself a command that takes arguments.
Instead I've named it lotus client retrieve-cancel

@dirkmc dirkmc requested a review from magik6k March 26, 2021 09:48
@raulk raulk changed the title add cancel-retrieval-deal cmd to lotus CLI add lotus client cancel-retrieval cmd to lotus CLI Mar 26, 2021
raulk pushed a commit that referenced this pull request Mar 26, 2021
Squash of #5871.

add cancel-retrieval-deal cmd

changes as per review

fix: cancel retrieval deal - disallow negative deal ID

fix: rename command to retrieve-cancel

rename command to cancel-retrieval; rename args to follow Lotus style.
@raulk
Copy link
Member

raulk commented Mar 26, 2021

This has been squashed into https://github.com/filecoin-project/lotus/tree/minerx/staging for minerX early testing.

@arajasek
Copy link
Contributor

Fixes #5870

@magik6k
Copy link
Contributor

magik6k commented Mar 29, 2021

An integration test would be great to have

dirkmc pushed a commit that referenced this pull request Mar 31, 2021
Squash of #5871.

add cancel-retrieval-deal cmd

changes as per review

fix: cancel retrieval deal - disallow negative deal ID

fix: rename command to retrieve-cancel

rename command to cancel-retrieval; rename args to follow Lotus style.
@magik6k magik6k merged commit 9adb30c into master Mar 31, 2021
@magik6k magik6k deleted the nonsense/add-cancel-retrieval-deal-cmd branch March 31, 2021 17:20
@nonsense
Copy link
Member Author

An integration test would be great to have

Note that this is not forgotten, I just got pulled into something else. I think when we add list retrieval deals command, it should be trivial to add an integration test for this cmd as well.

@magik6k magik6k mentioned this pull request Apr 13, 2021
69 tasks
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.

[Feature Request] lotus CLI command to cancel retrieval deal
6 participants