Joon/732 stake keeper #745
Conversation
Codecov Report
@@ Coverage Diff @@
## rigel/tick-tests #745 +/- ##
===================================================
Coverage ? 60.52%
===================================================
Files ? 62
Lines ? 3298
Branches ? 0
===================================================
Hits ? 1996
Misses ? 1157
Partials ? 145 |
x/stake/keeper.go
Outdated
| @@ -141,7 +152,7 @@ func (k Keeper) GetValidators(ctx sdk.Context) (validators []Validator) { | |||
|
|
|||
| // add the actual validator power sorted store | |||
| maxVal := k.GetParams(ctx).MaxValidators | |||
| iterator := store.ReverseIterator(subspace(ValidatorsKey)) //smallest to largest | |||
| iterator := store.ReverseIterator(subspace(ValidatorsKey)) // largest to smallest | |||
x/stake/keeper_test.go
Outdated
| } | ||
| } | ||
|
|
||
| genValidators := func(candidates []Candidate) []Validator { |
There was a problem hiding this comment.
Let's not use this at all the point of these tests was not to create the group of validators arbitrarily from the candidate list but to GET the validators from using the keeper functionality! this doesn't really provide us much value to just generate the validator set from this function
x/stake/keeper_test.go
Outdated
| return validators | ||
| } | ||
|
|
||
| candidates := []Candidate{candidatesIn[1], candidatesIn[4]} |
There was a problem hiding this comment.
okay so I added some function named candidatesIN but this is really confusing generating the candidate group this way - ideally we don't simple "compile" the list as we've done here but retrieve the list from the keeper using "keeper.GetCandidates"
x/stake/keeper_test.go
Outdated
| // test validator added at the beginning | ||
| // test validator added in the middle | ||
| candidates = append([]Candidate{candidatesIn[0]}, candidates...) |
There was a problem hiding this comment.
As per my previous comment we should not be adding to a list, but retrieving from the state
x/stake/keeper_test.go
Outdated
| keeper.setCandidate(ctx, candidates[3]) | ||
| keeper.setCandidate(ctx, candidates[4]) | ||
| acc = keeper.getAccUpdateValidators(ctx) | ||
| validatorsEqual(t, validators, acc) |
There was a problem hiding this comment.
Sorry I broke this test >:) - lol but yeah again we need to reconfigure what we are even testing here!
There was a problem hiding this comment.
Left a few comments... TestGetAccUpdateValidators isn't really actually testing what I had intended each of those comments to test... like we should not be compiling our own lists we should be retrieving lists... like "add a validator to the middle" in this sense means set a candidate to the state using the keeper which is somewhere in the middle for voting power compared to the others!
-> also we need to really check these 4 core cases as I described in person, I don't think we've explicitly and clearly done this
- new validator moves into the validator set, it is accumulated as so
- existing validator already in the validator set changes its voting power
- existing validator moves out of the validator set (aka acc will be {0, addr}
- a candidate which was never in the validator set changes its power but remains still not in the validator set (should not enter the acc changes)
|
Shouldn't the insertion tests be placed in TestGetValidators? Adding candidates that have medium voting power doesn't need to be in TestGetAccUpdateValidators since it has more relationship with Validators store and AccUpdateValidators depends only on the candidates' address. @rigelrozanski Also, I think the case 1, 2, 3 is implemented? Currently working on 4 (and removing genValidators) |
|
yes 1/2/3 may very well be implemented maybe we just need better organization/comments |
mossid
force-pushed
the
joon/732-stake-keeper
branch
from
17a34bb to
77744cb
Compare
|
Let's squash commits before merge which don't particularly useful information such as like "in progress" and "done" |
in progress in progress
in progress in progress done
rigelrozanski
force-pushed
the
joon/732-stake-keeper
branch
from
7815fdf to
a6d587b
Compare
rigelrozanski
force-pushed
the
rigel/tick-tests
branch
from
f1ebd67 to
7d67d00
Compare
|
Great work Joon! I reorganized the tests a bit made a few fixes to functionality which your tests revealed as I was reworking them a bit.... check out the two final commits I added on (also I squashed a few of your commits) |
Closes: #732