Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose BLS public keys from platform.getValidatorsAt #1740

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

This is the last piece to be able to implement an adapter for the validators.State interface from the platformvm.Client interface.

Resolves #1634

Split out of #1611

How this works

Changes the platform.getValidatorsAt API return type to include BLS public keys. Note - this is a breaking change.

validators.State interface can be implemented by:

  • GetMinimumHeight -> GetHeight
  • GetCurrentHeight -> GetHeight
  • GetSubnetID -> ValidatedBy
  • GetValidatorSet -> GetValidatorsAt

How this was tested

Calling:

curl --location 'http://localhost:9650/ext/bc/P' \
--header 'Content-Type: application/json' \
--data '{
    "jsonrpc":"2.0",
    "id"     :1,
    "method" :"platform.getValidatorsAt",
    "params" :{
        "height": 109466,
        "subnetID": "11111111111111111111111111111111LpoYY"
    }
}'

against a synced Fuji node.

Previous result
New result

@StephenButtolph StephenButtolph added this to the v1.10.6 milestone Jul 20, 2023
@StephenButtolph StephenButtolph added enhancement New feature or request vm This involves virtual machines labels Jul 20, 2023
Copy link
Contributor

@darioush darioush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a test that un-marshaling the marshaled json returns the original object?
(Also it seems the weight is returned as a string in the new format vs a number in the old format, not sure if this change is intentional)

@StephenButtolph
Copy link
Contributor Author

Also it seems the weight is returned as a string in the new format vs a number in the old format, not sure if this change is intentional

We typically return weights as strings because javascript represents all numbers as floats. Which can cause precision errors with the sizes of the numbers we are returning. So we explicitly return strings so that javascript can parse them into big nums correctly.

The prior implementation was an oversight.

@StephenButtolph
Copy link
Contributor Author

should we add a test that un-marshaling the marshaled json returns the original object?

Added

Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One sanity check question, but LGTM

// GetValidatorsAtReply is the response from GetValidatorsAt
type GetValidatorsAtReply struct {
// TODO should we change this to map[ids.NodeID]*validators.Validator?
// We'd have to add a MarshalJSON method to validators.Validator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add MarshalJSON to validators.Validator like the comment describes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not. I decided to do it this way because it felt weird not to include the NodeID in the struct json definition... But it was duplicate information in the map (which I didn't want to expose over the json rpc)

@StephenButtolph StephenButtolph merged commit 83f5eee into dev Jul 20, 2023
13 checks passed
@StephenButtolph StephenButtolph deleted the expose-public-keys branch July 20, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vm This involves virtual machines
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants