ACP-236 (5): Add proposal execution for auto-renewed validators#5204
ACP-236 (5): Add proposal execution for auto-renewed validators#5204rrazvan1 wants to merge 5 commits intorazvan/acp-236-s4from
Conversation
rrazvan1
force-pushed
the
razvan/acp-236-s4
branch
from
ba87559 to
7f48920
Compare
rrazvan1
force-pushed
the
razvan/acp-236-s5
branch
from
c2f1ed8 to
09be63e
Compare
| switch { | ||
| case tx == nil: | ||
| return txs.ErrNilTx | ||
| case tx.TxID == ids.Empty: | ||
| return ErrInvalidID |
There was a problem hiding this comment.
Should this live in the new helper verifyRewardTx?
There was a problem hiding this comment.
the proper change would be moving this in RewardValidatorTx.SyntacticVerify(..) func
It cannot be moved in verifyRewardTx, because RewardAutoRenewedValidatorTx has the same checks (+ timestamp), in RewardAutoRenewedValidatorTx.SyntacticVerify(..), so there is an inconsistency of behaviour among rewards transactions.
The reason I chose not to move the RewardValidatorTx checks inside SyntacticVerify(..), is to have mininal changes in legacy code.
| return nil | ||
| } | ||
|
|
||
| func (*proposalTxExecutor) RewardAutoRenewedValidatorTx(*txs.RewardAutoRenewedValidatorTx) error { |
There was a problem hiding this comment.
Can we undo this move? This makes the diff a bit more difficult to review since it makes less clear that we refactored code into a new decreaseAbortStateCurrentSupply func. I think you probably did this to either have all usages of that new helper before its declaration, or to have exported code before unexported code, but I don't think there is a guideline for this in Google's style guide (correct me if wrong) and I generally try to optimize for easier PR reviews in these cases when I'm touching existing code.
There was a problem hiding this comment.
I did not move it 🤔, this is how git shows the diff...
rrazvan1
force-pushed
the
razvan/acp-236-s4
branch
from
374ec5c to
81b4ed8
Compare
rrazvan1
force-pushed
the
razvan/acp-236-s5
branch
from
09be63e to
d8c8257
Compare
joshua-kim
left a comment
joshua-kim
left a comment
There was a problem hiding this comment.
I thought this was in the previous PR batch but I forgot to post these...
There was a problem hiding this comment.
Seems like we can refactor this w/ the new createUTXOsStakeOut function if we make the tx parameter accept the txs.PermissionlessStaker interface now
There was a problem hiding this comment.
check the new changes!
rrazvan1
force-pushed
the
razvan/acp-236-s5
branch
from
d8c8257 to
af2d56e
Compare
rrazvan1
force-pushed
the
razvan/acp-236-s4
branch
from
81b4ed8 to
503d7e2
Compare
rrazvan1
force-pushed
the
razvan/acp-236-s5
branch
from
192a105 to
d6b7393
Compare
rrazvan1
force-pushed
the
razvan/acp-236-s4
branch
from
73132fb to
801ccfd
Compare
3 participants
Why this should be merged
Implements the proposal-side execution that handles staking period expiry and reward/renewal logic for auto-renewed validators.
How this works
ProposalTxExecutorto processRewardAutoRenewedValidatorTx, handling reward calculation, auto-compounding, and re-staking for the next period.How this was tested
Covered by the reward validator tests added in #5205 .
Need to be documented in RELEASES.md?
No