-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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:market:add lotus filplus cli for extend claims terms #11545
Conversation
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.
Thanks for the PR! This broadly looks correct, though I have some suggested improvements.
}, | ||
&cli.BoolFlag{ | ||
Name: "really-do-it", | ||
Usage: "pass this flag to really extend sectors, otherwise will only print out json representation of parameters", |
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 looks like we just fail if really-do-it
isn't passed?
ArgsUsage: "providerAddress Optional[...claimId]", | ||
Flags: []cli.Flag{ | ||
&cli.StringFlag{ | ||
Name: "from", |
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.
i'd suggest making this a second mandatory parameter called clientAddress
, and use that as the sender
return IncorrectNumArgs(cctx) | ||
} | ||
|
||
if !cctx.Bool("really-do-it") { |
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.
Instead, I'd suggest running the logic as before, but calling GasEstimateMessageGas
instead of sending the message. This will simulate the message, which is useful to know whether it would pass and what it would cost, without taking any action.
See here for an example of a place we do this.
return err | ||
} | ||
|
||
claimIDsMap := make(map[verifreg.ClaimId]struct{}, len(args)-1) |
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.
The Lotus API already has a method called StateGetClaims
, which returns all the claims for a specific provider. Can we use that, instead of decoding the state here?
@nicelove666 Please note that we have one more PR to implement claim extension. I would request you to please checkout my comment #11466 (review) on the other PR. I would request you and @swift-mx to work together to avoid duplicating efforts and expanding the scope of the command as requested. |
Closing this there has been no input from author. #11711 supersedes this now. |
Related Issues
resolve #10908
Proposed Changes
Add the CLI command for deal renewal
Additional Info
Name: "extend-claims-terms",
Usage: "extend claims terms",
ArgsUsage: "providerAddress Optional[...claimId]",
Flags: []cli.Flag{
&cli.StringFlag{
Name: "from",
Usage: "specify the account to send the message from, it's client address",
Required: true,
},
&cli.Int64Flag{
Name: "extension",
Usage: "try to extend selected sectors by this number of epochs, defaults to 180 days",
Value: 518400,
},
&cli.Int64Flag{
Name: "new-expiration",
Usage: "try to extend selected sectors to this epoch, ignoring extension",
},
&cli.BoolFlag{
Name: "really-do-it",
Usage: "pass this flag to really extend sectors, otherwise will only print out json representation of parameters",
},
},
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps