[staking] make transfering ownership back valid #4319
Conversation
envestcc
requested review from
CoderZhi,
dustinxie,
Liuhaai and
a team
as code owners
| id, err := p.generateCandidateID(owner, blkCtx.BlockHeight, csm) | ||
| if err != nil { | ||
| // cannot collide with existing identifier | ||
| if csm.GetByIdentifier(owner) != nil { |
There was a problem hiding this comment.
This is to ensure that the owner is not used as an ID for any existed candidate.
| failureStatus: iotextypes.ReceiptStatus_ErrCandidateAlreadyExist, | ||
| } | ||
| } | ||
| candID = id | ||
| if csm.GetByIdentifier(candID) != nil { |
There was a problem hiding this comment.
The reason for deletion here is that there is no need to check for ID conflicts anymore, as this is already ensured in generateCandidateID.
| failureStatus: iotextypes.ReceiptStatus_ErrCandidateAlreadyExist, | ||
| } | ||
| } | ||
| candID = id | ||
| if csm.GetByIdentifier(candID) != nil { | ||
| id, err := p.generateCandidateID(owner, blkCtx.BlockHeight, csm) |
There was a problem hiding this comment.
by moving the conflict check ahead, generateCandidateID will always return the original owner address, the for loop will never enter. this would cause 1 address can register only once
as discussed last evening, should remove the 3 lines
if isValidID(owner) {
return owner, nil
}
and let for loop to generate a random identifier. Then, the same address (after transfer ownership) can register again
There was a problem hiding this comment.
Yes, the first point is correct that only owner address will be used as identifier.
You might miss some part of the discussion. After the hard fork, preventing the same address to generate a new candidate is on purpose, to prevent confusion: CandidateByOwner and CandidateByIdentifier returns two different candidates given the same address. The reason I didn't ask @envestcc to delete the logic in generateCandidateID in this PR is that it really doesn't matter to keep it.
Description
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: