Always store the reward recipient #657
Conversation
richardpringle
requested review from
iansuvak,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
richardpringle
force-pushed
the
staking-rewards-to-any
branch
from
2c594c7 to
fab95d9
Compare
| @@ -289,11 +289,7 @@ abstract contract PoSValidatorManager is | |||
| revert UnauthorizedOwner(_msgSender()); | |||
| } | |||
|
|
|||
| if (rewardRecipient == _msgSender()) { | |||
| delete $._rewardRecipients[validationID]; | |||
There was a problem hiding this comment.
I think delete's would still make sense. Looks like the gas cost of no-op delete is 100 (since delete just sets it to zero value) and gas cast of delete when a value was stored is 5000 but refunds up to 4900 (the gas cap being half of the total gas cast of the transaction)
There was a problem hiding this comment.
So I added the deletes back (this isn't actually the place where we wanted to do that btw), but I'm not quite sure I understand the point of deleting the value to begin with if there's a cost to deletion in both cases. If we were refunded more it would make sense, but once the validation is complete, is there any use for that field of the mapping? We won't use that validationID again, will we?
Very possible I'm missing something here.
There was a problem hiding this comment.
Yeah sorry I just wanted to make a general point about deletes and put it in the wrong place.
Also looks like I was wrong about the gas costs above as well as the refund cap. Refund cap is 20% of the overall tx gas cost and the refund is indeed higher than the cost in order to incentivize cleaning up after yourself. Playing around with https://www.evm.codes/#55 simulator it looks like in the actual delete case the cost would be: 2900 and refund 4800. The "warm" case should apply here since we check for the value first.
There was a problem hiding this comment.
Ha! To be honest, I figured that was the case which is why I added them back, but then I reread your comment which gave me pause.
Definitely makes sense to delete when done here. Thanks for the comments!
There was a problem hiding this comment.
Well... Not "here", but you know what I mean
richardpringle
force-pushed
the
staking-rewards-to-any
branch
from
fab95d9 to
e5e25b2
Compare
| if (rewardRecipient == address(0)) { | ||
| rewardRecipient = owner; | ||
| } else { | ||
| delete $._rewardRecipients[validationID]; |
| @@ -895,8 +892,6 @@ abstract contract PoSValidatorManager is | |||
|
|
|||
| if (rewardRecipient == address(0)) { | |||
| rewardRecipient = delegator.owner; | |||
| } else { | |||
| delete $._delegatorRewardRecipients[delegationID]; | |||
There was a problem hiding this comment.
Same comment about the delete here
richardpringle
force-pushed
the
staking-rewards-to-any
branch
from
ce3b7f8 to
bdbb295
Compare
Why this should be merged
Addresses comments from @geoff-vball and @bernard-avalabs about simplifying the logic instead of handling the nil-address as a special case.
I did this a little defensively though. It shouldn't be possible to have a nil value as the reward recipient, but if someone makes some other changes to the contract(s), I defaulted to the owner.
I also stopped deleting the state, not sure if this makes a huge difference.
How this works
If the caller sets the reward-recipient to the nil-address, we default to the owner. The behaviour is the same, but we now store the reward recipient.
How this was tested
I used the same set of tests that existed before.
How is this documented
I updated the inline docs which did not include the behaviour for a 0-address argument.