Add miner ChangeOwnerAddress method #1206
Conversation
actors/builtin/miner/miner_actor.go
Outdated
| // Proposes or confirms a change of owner address. | ||
| // If invoked by the current owner, proposes a new owner address for confirmation. If the proposed address is the | ||
| // current owner address, revokes any existing proposal. | ||
| // If invoked by and re-proposing the previously proposed address, changes the current owner address to be |
There was a problem hiding this comment.
At first I thought this sentence was ungrammatical, but now I see it's a sort of fascinating mixture where "previously proposed address" is the object of "invoked by" and both the object and, by implication, subject of "re-proposing". Fascination aside, it is a difficult sentence to parse. I would just drop "and re-proposing" or start with "If invoked by previously proposed address with the same proposal, ...".
| var st State | ||
| rt.StateTransaction(&st, func() { | ||
| info := getMinerInfo(rt, &st) | ||
| if rt.Caller() == info.Owner || info.PendingOwnerAddress == nil { |
There was a problem hiding this comment.
This looks correct. I had to make the table below to convince myself all the cases were covered.
POA==nil && Owner && NA==Owner => POA=nil
POA==nil && Owner && NA!=Owner => POA=NA
POA==nil && !Owner => Not Owner Error
POA!=nil && Owner => POA=NA
POA!=nil && !Owner && !POA => Not POA Error
POA!=nil && !Owner && POA && NA!=POA => Error
POA!=nil && !Owner && POA && NA==POA => Owner=POA, POA=nil
An alternate construction:
if Owner:
validate caller is owner
info.PendingOwnerAddress = newAddress
else:
validate PendingOwnerAddress is not nil # if needed
validate caller is PendingOwnerAddress
validate newAddress == PendingOwnerAddress
info.Owner = PendingOwnerAddress
if PendingOwnerAddress == info.Owner:
info.PendingOwnerAddress == nil
This separates the authorization logic from the logic that determines whether this has a future change or not and seems a little more clear to me, but it's up to you.
There was a problem hiding this comment.
I mostly achieved your suggestion. One constraint is my goal to fail via ValidateImmediateCallerIs (hence SysErrForbidden) in all paths except confirming the wrong address. So if info.PendingOwnerAddress == nil, via ValidateImmediateCallerIs(info.Owner).
|
|
||
| info := actor.getInfo(rt) | ||
| assert.Equal(t, actor.owner, info.Owner) | ||
| assert.Nil(t, info.PendingOwnerAddress) |
There was a problem hiding this comment.
An ExpectAbort when the proposed address attempts to confirm it would add confidence and clarity here.
anorth
force-pushed
the
anorth/changeowner
branch
from
f109d2f
to
004c618
Compare
Codecov Report
@@ Coverage Diff @@
## master #1206 +/- ##
========================================
- Coverage 71.3% 71.1% -0.3%
========================================
Files 65 65
Lines 6552 6697 +145
========================================
+ Hits 4674 4763 +89
- Misses 1135 1166 +31
- Partials 743 768 +25 |
|
Doesn't this prevent multisig actors to be the owner address? |
A miner operator may wish to change the owner address associated with a miner, e.g. due to compromise or for key rotation.
This is a one-way operation that cannot be undone if the operator sets an address they do not actually control. To reduce risk of error, the method confirms effective ownership of the new address with a two-step process:
The operator (or someone) must have access to the new address's private key in order to send the second message.
Closes #1176
FYI @magik6k @arajasek