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

feat: api/cli: beneficiary withdraw api and cli #9296

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

geoff-vball
Copy link
Contributor

@geoff-vball geoff-vball commented Sep 12, 2022

Related Issues

Closes #9257 and #9258

Proposed Changes

Adds an api call and cli command to withdraw funds from a miner as the beneficiary.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@geoff-vball geoff-vball requested a review from a team as a code owner September 12, 2022 22:02
Base automatically changed from jen/nv17 to master September 12, 2022 22:19
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

I don't think we want the new API method in the FullNodeAPI here, just the Storage API. See ActorWithdrawBalance for an equivalent method to use as a blueprint.

@geoff-vball geoff-vball force-pushed the gstuart/beneficiary-withdraw-api branch 3 times, most recently from 600624e to f6dcb01 Compare September 13, 2022 15:10
@geoff-vball
Copy link
Contributor Author

@arajasek Does the storage API get run on the lotus node or through lotus-miner? I think I feel pretty strongly that this is an action that shouldn't require running lotus-miner to achieve.

@arajasek
Copy link
Contributor

@geoff-vball Yes, api_storage is lotus-miner only. I would match what we currently have for (Owner)WithdrawBalance for now (which is to only have it on the miner API), and create a ticket to support both (Owner)WithdrawBalance and BeneficiaryWithdrawBalance over the node API. I'd be...skeptical about whether these are actions we want to be able to take remotely over an API, but we can descope that discussion.

Additionally, I would move the CLI commands to the following two locations, you can match existing behaviour there:

var actorWithdrawCmd = &cli.Command{
,
var actorWithdrawCmd = &cli.Command{
. The first can be called by anyone who has built lotus-shed, the second requires the miner running in the background.

TL;DR: let's match what we have for the existing withdrawBalance method and take it from there.

@geoff-vball geoff-vball force-pushed the gstuart/beneficiary-withdraw-api branch from f6dcb01 to 477c9cf Compare September 14, 2022 04:19
cmd/lotus-shed/actor.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball changed the title feat: api/cli: gstuart/beneficiary withdraw api feat: api/cli: beneficiary withdraw api and cli Sep 14, 2022
@@ -40,6 +40,7 @@ var actorCmd = &cli.Command{
Subcommands: []*cli.Command{
actorSetAddrsCmd,
actorWithdrawCmd,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to ownerWithdrawCmd, rename the command itself to owner-withdraw, but leave withdraw as an alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored everything. Now the cli commands just need a flag to know to send from beneficiary.

cmd/lotus-miner/actor.go Outdated Show resolved Hide resolved
cmd/lotus-shed/actor.go Outdated Show resolved Hide resolved
node/impl/storminer.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/beneficiary-withdraw-api branch from 6e01663 to 4025a11 Compare September 14, 2022 14:59
cmd/lotus-miner/actor.go Outdated Show resolved Hide resolved
cmd/lotus-shed/actor.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/beneficiary-withdraw-api branch from 8b04641 to 21906b5 Compare September 14, 2022 16:26
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.

Integrate the new WithdrawBalance to support caller as owner or beneficairy
2 participants