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

Andrew7234/add validator names #769

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Andrew7234
Copy link
Collaborator

Per request from explorer folks, we now include the validator human-readable name in /*delegations* endpoints when available. Note that the e2e test updates do not reflect this change because the metadata_registry analyzer isn't enabled for e2e_regression tests. However, I did enable it manually and found a couple outdated tests which are also fixed in this PR.

Screenshot 2024-10-17 at 5 17 09 PM

@Andrew7234 Andrew7234 force-pushed the andrew7234/add-validator-names branch from 4dcdc44 to 16805da Compare October 17, 2024 22:20
@@ -1511,6 +1511,9 @@ components:
type: string
description: The delegatee (validator) address.
example: *staking_address_1
validator_name:
Copy link
Member

@ptrus ptrus Oct 22, 2024

Choose a reason for hiding this comment

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

The API's approach to handling validator metadata has become somewhat inconsistent.

Currently, we return validator_name for these endpoints, EntityInfo for block proposers and signers, and ValidatorMedia for validator-specific endpoints. Although these represent the same underlying data, the choice of response fields across different endpoints feels somewhat arbitrary.

To improve this, I suggest either:

  1. Unify the responses so all relevant endpoints return the same metadata structure, or
  2. (my preference, but apparently not the forntend's): Keep metadata to be returned only by the validator endpoint. The frontend can cache and reuse this information across different parts of the app - whenever dealing with consensus addresses.

Main Reasoning for Option 2:

  • Imagine a design choice is make to also display validator logo next to the name when showing delegations - this would mean updating these endpoints by also including validator_logo
  • If a delegator is a validator, the frontend would likely also want to show the validator name. With the current approach, we'd need to introduce additional fields like delegator_name to the delegation endpoint
  • Consider other endpoints, like consensus/account/{address}. When querying account information for a validator, the frontend would want to show the validator's name. Following the current setup, this would mean adding ValidatorMedia to the account endpoint response.
  • Similarly, if a consensus transaction was submitted by an address that belongs to a validator (or if the recipient is a validator), the frontend would likely want to display the validator's name. This would mean adding the validator metadata to yet more endpoints (consensus/transactions), resulting in more redundancy.

Copy link
Contributor

@buberdds buberdds Oct 22, 2024

Choose a reason for hiding this comment

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

(my preference, but apparently not the forntend's): Keep metadata to be returned only by the validator endpoint.

We have discussed this in https://app.clickup.com/t/869683ayv and I was under the impression that we agreed to handle this on the frontend (PR was created last week oasisprotocol/explorer#1576), since we need the names to be displayed in multiple views.

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