Validator Cliff Updates #1565
Validator Cliff Updates #1565
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1565 +/- ##
===========================================
- Coverage 64.09% 64.06% -0.03%
===========================================
Files 122 122
Lines 6670 6674 +4
===========================================
+ Hits 4275 4276 +1
- Misses 2148 2150 +2
- Partials 247 248 +1 |
x/stake/keeper/validator.go
Outdated
| @@ -303,6 +303,8 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |||
| // TODO benchmark if we should read the current power and not write if it's the same | |||
| if bondedValidatorsCount == int(maxValidators) { // is cliff validator | |||
| k.setCliffValidator(ctx, validator, k.GetPool(ctx)) | |||
| } else { | |||
| k.clearCliffValidator(ctx) | |||
There was a problem hiding this comment.
Presumably we only need to clear the cliff validator if it was previously set (we had 100 validators) and it should be set no longer (we now have 99)?
There was a problem hiding this comment.
correct - I'll add this check in here
x/stake/keeper/validator_test.go
Outdated
| @@ -634,6 +634,7 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { | |||
| require.Equal(t, validators[4].ABCIValidator(), updates[0]) | |||
| } | |||
|
|
|||
| //TODO why is this called NotValidatorCliff? | |||
There was a problem hiding this comment.
No idea - per git blame, you wrote it. 😉
Looks like this tests the cliff validator logic (in the case when we do have a cliff validator) - maybe TestGetTendermintUpdatesWithCliffValidator?
There was a problem hiding this comment.
haha - I wrote that TODO at some unruly hour knowing I might forget about it... thanks! - yeah cool I like your suggested name
x/stake/keeper/validator.go
Outdated
| } | ||
|
|
||
| bondedValidatorsCount++ |
There was a problem hiding this comment.
Hmm, what if the validator is revoked - we should make sure it's unbonded, right? (not sure the logic on line 409 is right either)
Seems like we might try to bond a revoked validator and then panic
There was a problem hiding this comment.
OH THANKS - this is actually the error that I caught (aka the code change your seeing actually doesn't affect the functionality at all, surprise that the linter didn't catch this previously honestly) it just wasn't updated for the FULL case... updating now
There was a problem hiding this comment.
in other words the fact that line was in an else at all is quite misleading
Updates to validator cliff logic:
supersedes #1530
Closes #1519
Fixes previously unknown issues around ordering of revoked validator set in power-rank
CHANGELOG