R4R: Change /staking/validators endpoint to return all validators candidates #4067
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tes as stated in API documentation.
Codecov Report
@@ Coverage Diff @@
## develop #4067 +/- ##
==========================================
- Coverage 60.01% 60% -0.01%
==========================================
Files 212 212
Lines 15107 15106 -1
==========================================
- Hits 9066 9065 -1
Misses 5420 5420
Partials 621 621 |
1 similar comment
Codecov Report
@@ Coverage Diff @@
## develop #4067 +/- ##
==========================================
- Coverage 60.01% 60% -0.01%
==========================================
Files 212 212
Lines 15107 15106 -1
==========================================
- Hits 9066 9065 -1
Misses 5420 5420
Partials 621 621 |
alexanderbez
suggested changes
Apr 8, 2019
There was a problem hiding this comment.
Thanks @Vuksan! Left a question for @rigelrozanski to get clarification. Also, this needs a pending change log entry.
|
|
||
| res, errRes := codec.MarshalJSONIndent(cdc, validators) | ||
| if err != nil { | ||
| if errRes != nil { |
There was a problem hiding this comment.
Good catch. I recall fixing the same issue elsewhere in staking. Perhaps we should do a quick pass through and make sure there aren't other bugs like this 👍
| @@ -129,11 +129,10 @@ func NewQueryRedelegationParams(delegatorAddr sdk.AccAddress, srcValidatorAddr s | |||
| } | |||
|
|
|||
| func queryValidators(ctx sdk.Context, cdc *codec.Codec, k keep.Keeper) (res []byte, err sdk.Error) { | |||
| stakingParams := k.GetParams(ctx) | |||
| validators := k.GetValidators(ctx, stakingParams.MaxValidators) | |||
| validators := k.GetAllValidators(ctx) | |||
There was a problem hiding this comment.
If the query's documentation states all validators instead of all bonded validators, then this change make sense. Is this the expectation @rigelrozanski?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API documentation states "Get all validator candidates".
It was returning up to
maxValidatorsvalidator candidates, not all of them.Also, error below never got returned, because of typo: condition was checking err instead of errRes.
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work. - I searched through issues, but couldn't find any related to this.
Wrote tests
Updated relevant documentation (
docs/) - API documentation already states that all validator candidates should be returned.Added a relevant changelog entry:
sdkch add [section] [stanza] [message]rereviewed
Files changedin the github PR explorerFor Admin Use: