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: add claim extension #711

Merged
merged 4 commits into from
Mar 28, 2024
Merged

feat: add claim extension #711

merged 4 commits into from
Mar 28, 2024

Conversation

LexLuthr
Copy link
Collaborator

@LexLuthr LexLuthr commented Mar 12, 2024

Depends on filecoin-project/lotus#11711

  • When we say NOW to TermMax and if TermMax was already 5 years then nothing changes. So does the deal expired after the original TermStart+TermMax or does it stay beyond that and expired at NOW+TermMax? Need clarification from Actor team.

@LexLuthr LexLuthr marked this pull request as ready for review March 18, 2024 10:04
@LexLuthr LexLuthr requested review from TippyFlitsUK and rjan90 March 18, 2024 10:04
@beck-8
Copy link

beck-8 commented Mar 18, 2024

I think it needs to be clarified here, the period of the sector or some people will misunderstand?
I think it's important to clarify here that sectors are limited to only a 5-year term.so not 9 years 10x power
image

@beck-8
Copy link

beck-8 commented Mar 18, 2024

image
The commands for 1 and 2 are exactly the same? Is there a problem here?

I think in the second case, you need to add something like --really-do-it, because some people may not want to use datacap, but inadvertently use it.

@LexLuthr
Copy link
Collaborator Author

@beck-8 The command structure does not change because you are using a different client address. The description is different and so is the --client value.
As for the --really-do-it we already have checks in code to confirm the use of Datacap.

@LexLuthr
Copy link
Collaborator Author

I think it needs to be clarified here, the period of the sector or some people will misunderstand? I think it's important to clarify here that sectors are limited to only a 5-year term.so not 9 years 10x power image

@beck-8 Can you please recheck that line now and confirm it is not confusing anymore.

@beck-8
Copy link

beck-8 commented Mar 18, 2024

I think it needs to be clarified here, the period of the sector or some people will misunderstand? I think it's important to clarify here that sectors are limited to only a 5-year term.so not 9 years 10x power image

@beck-8 Can you please recheck that line now and confirm it is not confusing anymore.

i think no problem

@beck-8
Copy link

beck-8 commented Mar 18, 2024

@beck-8 The command structure does not change because you are using a different client address. The description is different and so is the --client value.
As for the --really-do-it we already have checks in code to confirm the use of Datacap.

Let me re-describe the problem just now: I'll illustrate it with examples.

Client1-sp1-claim1

Client2-sp1-claim2

Now I want to use client2 to renew claim2 for sp1 (no datacap consumption), assuming that sp1 only has claim1 claim2, so I will use the -all parameter.

Will this cause client2 to renew the claim1 of SP1 together (consuming datacap)

I didn't look at your PR by myself in this place. I just simply saw the docs, so I have this question.

@LexLuthr
Copy link
Collaborator Author

@beck-8 The command structure does not change because you are using a different client address. The description is different and so is the --client value.
As for the --really-do-it we already have checks in code to confirm the use of Datacap.

Let me re-describe the problem just now: I'll illustrate it with examples.

Client1-sp1-claim1

Client2-sp1-claim2

Now I want to use client2 to renew claim2 for sp1 (no datacap consumption), assuming that sp1 only has claim1 claim2, so I will use the -all parameter.

Will this cause client2 to renew the claim1 of SP1 together (consuming datacap)

The command give you a warning that it required Datacap to extend all claims. The Datacap will be the for claim1 only in this case. Then it will ask you to proceed or not. If you Choose NO, it will simply extend claim2 only.

I didn't look at your PR by myself in this place. I just simply saw the docs, so I have this question.

@beck-8
Copy link

beck-8 commented Mar 18, 2024

I understand, thank you very much

Copy link
Collaborator

@rjan90 rjan90 left a comment

Choose a reason for hiding this comment

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

lgtm. Just some smaller nits

content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@TippyFlitsUK TippyFlitsUK left a comment

Choose a reason for hiding this comment

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

Looking good!! Couple of minor suggestions.

content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
content/en/lotus/manage/claim-extension.md Outdated Show resolved Hide resolved
@LexLuthr LexLuthr merged commit 0d563e6 into main Mar 28, 2024
2 checks passed
@LexLuthr LexLuthr deleted the feat/claim-extension branch March 28, 2024 11:18
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.

4 participants