Skip to content

Add beacon api postStateValidatorIdentities #7223

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

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open

Conversation

ahshum
Copy link
Contributor

@ahshum ahshum commented Jun 11, 2025

Copy link

github-actions bot commented Jun 11, 2025

Unit Test Results

       15 files    2 645 suites   1h 14m 32s ⏱️
  6 542 tests   6 018 ✔️ 524 💤 0
45 386 runs  44 638 ✔️ 748 💤 0

Results for commit bbecca4.

♻️ This comment has been updated with latest results.

@@ -223,6 +223,11 @@ type
status*: string
validator*: Validator

RestValidatorIdentity* = object
index*: ValidatorIndex
pubkeyData*{.serializedFieldName: "pubkey".}: HashedValidatorPubKey
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't this field just be called pubkey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah both work for me. I think it mainly helps the naming consistency as other types using HashedValidatorPubKey are pubkeyData.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to call it pubkeyData, to keep naming consistency rename it please to pubkey. Even if some types internally using pubkeyData all the REST types using pubkey.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also i have not seen HashedValidatorPubKey serialization/deserialization procedure, so it will not working because its impossible to automatically decode this type, so i think proper type for this object would be ValidatorPubKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does it mean? Serialization is working fine for Validator in the response of other APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

It means that it could provide whatever json-serialization will prefer, which probably could be totally incompatible with what specification means.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to call it pubkeyData, to keep naming consistency rename it please to pubkey. Even if some types internally using pubkeyData all the REST types using pubkey.

80cf236

@cheatfate
Copy link
Contributor

This change require implementation of additional tests in ncli/resttest-rules.json

@tersec
Copy link
Contributor

tersec commented Jun 23, 2025

This change require implementation of additional tests in ncli/resttest-rules.json

3327c4c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants