Add "proposed" optional flag to getValidatorsAt
#3531
Conversation
| @@ -102,6 +102,7 @@ var _ = e2e.DescribePChain("[Validator Sets]", func() { | |||
| tc.DefaultContext(), | |||
| constants.PrimaryNetworkID, | |||
| height, | |||
| false, | |||
There was a problem hiding this comment.
(No action required) When using a literal value that doesn't provide context as to what it represents, maybe add a comment (e.g. false, // isProposed)?
The pivot to this approach was to re-use the already existing field and make it more easily cacheable on the backend and keep the number of externally exposed RPC methods limited if they are accomplishing similar things. Given the pivot in strategy we opted for a new PR.
I'm open to either solution. The reasoning behind accepting multi-type field was since it's already an established pattern in the industry, see eth_getBlockByNumber which accepts |
| @@ -1689,7 +1689,7 @@ func (s *Service) GetTimestamp(_ *http.Request, _ *struct{}, reply *GetTimestamp | |||
|
|
|||
| // GetValidatorsAtArgs is the response from GetValidatorsAt | |||
| type GetValidatorsAtArgs struct { | |||
| Height avajson.Uint64 `json:"height"` | |||
| Height avajson.Height `json:"height"` | |||
There was a problem hiding this comment.
I guess this is to prevent breaking change for that args. However I wonder if a separate bool flag or even using negative values would make this more straightforward.
There was a problem hiding this comment.
Yeah indeed, that + custom parsing is to allow either json field. The idea was that eth supports this kind of thing already in eth_getBlockByNumber
There was a problem hiding this comment.
But happy to hear the votes either way!
There was a problem hiding this comment.
It's probably better to have backwards compatibility, so I'm fine with this or using negative values. Should we also add latest here?
There was a problem hiding this comment.
a new field would be optional regardless so I don't think it would impact backwards compatibility since the go client is already not backwards compatible in this implementation.
Re: latest I do think it makes sense to add but would prefer to do it as a follow up to get this change in before etna rollout. I do think it's slightly odd that platform.getValidatorsAt({height: "latest"}) and platform.getCurrentValidators{} would then return different types but I guess asking for them in the shorter return format that getValidatorsAt uses is a valid usecase
| type Height struct { | ||
| Numeric Uint64 | ||
| IsProposed bool | ||
| } |
There was a problem hiding this comment.
Similarly to Cey's comments, I wonder if rather than using a struct for this if it would be nicer to have special values for uint64.
Specifically - type Height uint64
- When json marshaling -
Height == MaxUint64gets mapped toproposed - When json unmarshaling -
proposedgets mapped toMaxUint64 - When json unmarshaling -
MaxUint64is considered invalid
That way in the client we could just pass in api.ProposedHeight rather than adding a new bool arg.
(Also I think this should probably live in the api package rather than the json package, this feels more API specific than general to json helpers)
Why this should be merged
I'm not convinced that it should but opening this up for discussion.
Adding this interface was discussed together with adding
platform.GetProposedHeightinterface in #3530 This is a convenience method to allow for passing "proposed" instead of a numeric height literal toGetValidatorsAt.The idea is to make it safer to publicly expose
GetValidatorsAtinterface. This by itself doesn't accomplish it but a different proxy service could use this same interface and callGetProposedHeightand return the cached value if this was called with the same resolved height and subnet.How this works
Makes the GetValidatorsAt accept either numeric height or a "proposed" string literal that first calls
getMinimumHeightand uses that instead of the numerical value. The service side is backward compatible but the client interface has changed.I'm also not sure if this solution is cleaner than just adding a new boolean flag field to the request. I don't think that the
platformVMservice uses this kind of type overloading anywhere else.How this was tested
Added a unit test.
Need to be documented in RELEASES.md?
Added a note to the
RELEASES.mdbut this also introduces a breaking change to the client and not sure what's the best way to handle communication around that