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

feat(x/staking)!: Add metadata field to validator info #21315

Merged
merged 47 commits into from
Oct 1, 2024

Conversation

ziscky
Copy link
Contributor

@ziscky ziscky commented Aug 15, 2024

Description

Closes: #9988


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Added support for additional metadata in validator descriptions, enhancing the information available for each validator.
    • Introduced new functions for generating random valid URIs, improving simulation capabilities.
  • Enhancements

    • Improved validation processes to ensure data integrity with new metadata fields in validator descriptions.
    • Enriched the validator creation process by integrating metadata generation into various workflows.
  • Bug Fixes

    • Resolved issues with block commitment behavior, ensuring alignment with expected operational functionality.

Copy link
Contributor

coderabbitai bot commented Aug 15, 2024

Walkthrough

Walkthrough

The changes introduced in the codebase enhance the staking module by adding a metadata parameter to validator descriptions. This allows for the inclusion of additional information, such as profile picture links, directly addressing the need for better management of validator profile images and reducing reliance on external services.

Changes

Files Change Summary
x/staking/..., simapp/simd/cmd/testnet.go, simapp/v2/simdv2/cmd/testnet.go, testutil/network/network.go Added metadata field to validator descriptions, updated initialization functions to handle new metadata input, and modified related structures accordingly.
tests/integration/... Updated test files to include metadata in validator descriptions, enhancing the testing framework to support the new metadata structure.
x/staking/types/..., tests/integration/slashing/slashing_test.go, tests/integration/gov/common_test.go Updated Description struct and methods to include metadata, added validation for metadata, and introduced a function for generating random URIs.
CHANGELOG.md Documented the enhancements related to metadata handling in the staking module and the corresponding changes across various components.
x/staking/simulation/rand_util.go, x/staking/simulation/rand_util_test.go Introduced utility functions for generating random URIs and corresponding unit tests to validate their functionality.

Assessment against linked issues

Objective Addressed Explanation
Make validator info have an explicit link to profile pictures (##9988)

Possibly related PRs

Suggested labels

C:x/accounts, C:x/accounts/multisig, C:collections

Suggested reviewers

  • sontrinh16
  • JulianToledano
  • facundomedica
  • testinginprod
  • akhilkumarpilli
  • tac0turtle

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 25986ca and 8ac40a0.

Files selected for processing (1)
  • x/staking/simulation/rand_util_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/staking/simulation/rand_util_test.go

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@@ -79,6 +78,9 @@ message Description {
string security_contact = 4;
// details define other optional details.
string details = 5;
// metadata is a map of string keys to any values that can be used to store additional information about the
// validator.
map<string, google.protobuf.Any> metadata = 6;
Copy link
Member

Choose a reason for hiding this comment

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

Wallet and explorer complained that proposal metadata were too loose and not typed (even though we had recommended json structure).
Shall we instead be opinionated about which metadata can be set? Using an any will lead to the same complaints I think.

@ziscky ziscky force-pushed the ziscky/9988-staking-validator-metadata branch from 937a95e to 14eaff9 Compare August 22, 2024 10:04
@ziscky ziscky marked this pull request as ready for review August 23, 2024 12:40
@ziscky ziscky requested a review from a team as a code owner August 23, 2024 12:40
@ziscky ziscky requested a review from julienrbrt August 23, 2024 12:40

This comment has been minimized.

@ziscky ziscky requested a review from aaronc August 23, 2024 12:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (2)
x/staking/client/cli/tx.go (1)

76-76: Include metadata in the example JSON.

The example JSON now includes a "metadata" field for profile pictures. Ensure the documentation reflects this change.

testutil/network/network.go (1)

492-492: Ensure all usages of stakingtypes.NewDescription include the metadata parameter.

The following files contain usages of stakingtypes.NewDescription that do not include the new stakingtypes.Metadata{} parameter:

  • tests/integration/slashing/slashing_test.go
  • tests/integration/staking/keeper/deterministic_test.go
  • tests/integration/gov/common_test.go
  • simapp/v2/simdv2/cmd/testnet.go

Please update these instances to include the metadata parameter to align with the PR objectives.

Analysis chain

LGTM! Verify the usage of stakingtypes.NewDescription.

The addition of stakingtypes.Metadata{} to the stakingtypes.NewDescription function call is consistent with the PR objectives of adding metadata for validator info.

Ensure that all usages of stakingtypes.NewDescription across the codebase are updated to include the new metadata parameter.

Run the following script to verify the usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of `stakingtypes.NewDescription` include the new metadata parameter.

# Test: Search for the function usage. Expect: Only occurrences with the new metadata parameter.
rg --type go -A 5 $'stakingtypes.NewDescription'

Length of output: 5767

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e88c138 and 33ca14a.

Files ignored due to path filters (2)
  • x/staking/proto/buf.lock is excluded by !**/*.lock
  • x/staking/types/staking.pb.go is excluded by !**/*.pb.go
Files selected for processing (11)
  • simapp/simd/cmd/testnet.go (1 hunks)
  • testutil/network/network.go (1 hunks)
  • x/genutil/gentx_test.go (1 hunks)
  • x/genutil/types/genesis_state_test.go (2 hunks)
  • x/staking/client/cli/flags.go (1 hunks)
  • x/staking/client/cli/tx.go (8 hunks)
  • x/staking/client/cli/utils.go (3 hunks)
  • x/staking/keeper/msg_server.go (1 hunks)
  • x/staking/proto/cosmos/staking/v1beta1/staking.proto (4 hunks)
  • x/staking/simulation/operations.go (2 hunks)
  • x/staking/types/validator.go (4 hunks)
Additional context used
Path-based instructions (10)
x/staking/client/cli/flags.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/client/cli/utils.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/genutil/types/genesis_state_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

x/genutil/gentx_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

x/staking/client/cli/tx.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/types/validator.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/simd/cmd/testnet.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/keeper/msg_server.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

testutil/network/network.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/simulation/operations.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (23)
x/staking/client/cli/flags.go (1)

24-24: Addition of FlagMetadata is appropriate.

The new constant FlagMetadata is added correctly and adheres to the Uber Golang style guide. It enhances the CLI functionality by allowing metadata specification. Ensure that this flag is utilized in relevant CLI commands.

x/staking/client/cli/utils.go (3)

27-27: Addition of Metadata field in validator struct is correct.

The inclusion of the Metadata field in the validator struct enhances its capability to store additional information. This aligns with the PR objective of managing validator profile images.


41-41: Ensure correct handling of Metadata in JSON parsing.

The addition of the Metadata field in the internalVal struct and its JSON tag is correctly implemented. This ensures that metadata is properly parsed and validated.


100-100: Return of Metadata in parseAndValidateValidatorJSON is appropriate.

The function correctly returns the Metadata field as part of the validator struct. This ensures that metadata is included in the validator information.

x/genutil/types/genesis_state_test.go (2)

42-42: Inclusion of Metadata in TestValidateGenesisMultipleMessages is correct.

The test function now includes the Metadata parameter, ensuring comprehensive validation of the new field. This aligns with the PR objectives.


69-69: Inclusion of Metadata in TestValidateGenesisBadMessage is correct.

The test function now includes the Metadata parameter, ensuring comprehensive validation of the new field. This aligns with the PR objectives.

x/genutil/gentx_test.go (1)

39-39: Ensure test coverage for the new metadata field.

The Description instantiation now includes a stakingtypes.Metadata{} parameter. Verify that test cases validate the handling of metadata.

x/staking/client/cli/tx.go (6)

4-4: Import statement for json is necessary.

The json package is used for unmarshalling metadata strings.


134-141: Validate metadata parsing logic.

The metadata string is parsed from JSON and assigned to the metadata variable. Ensure proper error handling and validation.


197-197: Ensure metadata is correctly included in NewDescription.

The NewDescription function now includes the metadata parameter. Verify that this change is correctly handled in the function logic.


278-278: Add Metadata field to TxCreateValidatorConfig.

The Metadata field is added to the TxCreateValidatorConfig struct. Ensure this field is correctly populated and used in transactions.


318-327: Validate metadata parsing in PrepareConfigForTxCreateValidator.

The metadata string is parsed and assigned to the Metadata field. Ensure proper error handling and validation.


407-407: Include metadata in BuildCreateValidatorMsg.

The BuildCreateValidatorMsg function now includes the Metadata field in the description. Verify that this change is correctly handled in the function logic.

x/staking/types/validator.go (4)

6-6: Import statement for url is necessary.

The url package is used for validating the ProfilePicUri in metadata.


192-199: Include metadata in NewDescription.

The NewDescription function now includes a metadata parameter. Ensure this change is correctly integrated into the function logic.


261-268: Validate metadata in Description.Validate.

The Validate method for Description now calls metadata.Validate(). Ensure this validation logic is robust and correctly implemented.


270-279: Validate ProfilePicUri in Metadata.Validate.

The Validate method for Metadata checks the ProfilePicUri format. Ensure this validation logic is robust and correctly implemented.

x/staking/proto/cosmos/staking/v1beta1/staking.proto (2)

81-83: Addition of metadata field in Description message is appropriate.

The inclusion of the metadata field enhances the Description message by allowing additional validator information, such as profile pictures. The non-nullable constraint ensures that this information is always present.


85-90: Introduction of Metadata message is well-structured.

The Metadata message provides a clear and structured way to include additional information, such as a profile picture URI, enhancing the validator's description.

simapp/simd/cmd/testnet.go (1)

351-351: Inclusion of Metadata in initTestnetFiles enhances validator initialization.

The addition of the Metadata parameter to the stakingtypes.NewDescription function call allows for richer validator descriptions, aligning with the PR's objectives.

x/staking/keeper/msg_server.go (1)

109-109: Switch to Validate() in CreateValidator enhances validation.

Replacing EnsureLength() with Validate() likely provides a more comprehensive validation mechanism, improving the robustness of input validation for validator creation.

x/staking/simulation/operations.go (2)

181-183: LGTM! Metadata parameter added to SimulateMsgCreateValidator.

The addition of the types.Metadata parameter with a ProfilePicUri field enhances the simulation function by allowing it to handle metadata related to a validator's profile picture.


283-285: LGTM! Metadata parameter added to SimulateMsgEditValidator.

The addition of the types.Metadata parameter with a ProfilePicUri field enhances the simulation function by allowing it to handle metadata related to a validator's profile picture.

Comment on lines 232 to 233
d.Metadata, // TODO: how should we check for the DoNotModifyDesc
).Validate()
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider handling DoNotModifyDesc for metadata.

The UpdateDescription function currently does not handle DoNotModifyDesc for metadata. Consider implementing this logic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f77f60b and 0d3db0a.

Files selected for processing (8)
  • CHANGELOG.md (1 hunks)
  • simapp/simd/cmd/testnet.go (1 hunks)
  • simapp/v2/simdv2/cmd/testnet.go (1 hunks)
  • tests/integration/gov/common_test.go (1 hunks)
  • tests/integration/slashing/slashing_test.go (1 hunks)
  • tests/integration/staking/keeper/deterministic_test.go (13 hunks)
  • tests/integration/tx/aminojson/aminojson_test.go (1 hunks)
  • testutil/network/network.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
  • CHANGELOG.md
  • tests/integration/gov/common_test.go
  • tests/integration/slashing/slashing_test.go
  • tests/integration/tx/aminojson/aminojson_test.go
  • testutil/network/network.go
Additional context used
Path-based instructions (3)
simapp/v2/simdv2/cmd/testnet.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/simd/cmd/testnet.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

tests/integration/staking/keeper/deterministic_test.go (3)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (4)
simapp/v2/simdv2/cmd/testnet.go (1)

298-298: Review of Metadata Addition in Validator Description

The addition of the metadata field to the stakingtypes.NewDescription function call is a crucial enhancement. This change aligns with the PR's objectives to allow validators to directly link their profile pictures, improving the management of validator profile images and reducing reliance on external services like Keybase.

However, it's important to ensure that the metadata field is being utilized correctly across the application, especially in areas where validator descriptions are displayed or processed.

Verification successful

Metadata Field Integration Verified

The metadata field in the stakingtypes.NewDescription function is consistently used across the codebase, including in test files. This indicates that the change is well-integrated and potentially covered by tests. Ensure that these tests verify the correct behavior of the metadata field to maintain robustness.

  • Files with Usage:
    • simapp/v2/simdv2/cmd/testnet.go
    • simapp/simd/cmd/testnet.go
    • tests/integration/staking/keeper/deterministic_test.go
    • tests/integration/slashing/slashing_test.go
    • tests/integration/gov/common_test.go
    • x/genutil/gentx_test.go
    • x/genutil/types/genesis_state_test.go
    • testutil/network/network.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new metadata field across the application.

# Test: Search for the usage of `stakingtypes.NewDescription` with the new metadata field.
ast-grep --lang go --pattern $'stakingtypes.NewDescription($_, $_, $_, $_, $_, stakingtypes.Metadata{})'

Length of output: 2385

tests/integration/staking/keeper/deterministic_test.go (3)

212-230: Review of the metadataGenerator and generateUri functions

The metadataGenerator function is well-implemented, using the rapid library to generate random metadata for testing, which includes a profile picture URI and social handle URIs. The generateUri function, used within metadataGenerator, correctly constructs URIs using random strings for the host and path, ensuring they are formatted as HTTPS URLs.

  • Correctness: The implementation correctly adheres to the PR's objective of adding a metadata field.
  • Performance: The use of rapid for generating test data is appropriate and efficient for test environments.
  • Best Practices: The code is clean, well-structured, and follows Go best practices.
  • Readability: The functions are concise and easy to understand.
  • Test Coverage: These functions are crucial for generating test data that includes the new metadata field, ensuring that the staking module's handling of this data is robustly tested.

Overall, the implementation of these functions meets the requirements and contributes effectively to the test suite.


251-251: Ensure Metadata Integration in Validator Creation

The integration of the metadataGenerator within the createValidator function is crucial for testing the new metadata functionality. By drawing metadata directly within the validator's description setup, this ensures that every test validator created has a metadata field populated, which aligns with the PR's objectives.

  • Correctness: Correctly integrates the new metadata field in test validators.
  • Best Practices: Follows good practice by ensuring that new functionality is included in integration tests.
  • Test Coverage: Adequately tests the new functionality by incorporating it into the standard validator creation process used across various tests.

This change is essential for ensuring that the new metadata field behaves as expected under different test scenarios.


327-327: Static Validators Metadata Initialization

The updates to getStaticValidator and getStaticValidator2 functions to include an empty stakingtypes.Metadata{} instance are minimal but necessary. This ensures that even static validators used in tests have the metadata field initialized, preventing any nil pointer exceptions and maintaining consistency with dynamic validators.

  • Correctness: Properly initializes the metadata field to an empty struct, which is safe and prevents potential runtime errors.
  • Consistency: Ensures that all validators, whether dynamically or statically generated, have their metadata fields initialized.
  • Best Practices: The approach of initializing complex structs to their zero values is a standard practice in Go, particularly in testing scenarios.

These changes are small but improve the robustness and maintainability of the test suite by ensuring consistency across different types of validators.

Also applies to: 363-363

simapp/simd/cmd/testnet.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f77f60b and 0d3db0a.

Files selected for processing (8)
  • CHANGELOG.md (1 hunks)
  • simapp/simd/cmd/testnet.go (1 hunks)
  • simapp/v2/simdv2/cmd/testnet.go (1 hunks)
  • tests/integration/gov/common_test.go (1 hunks)
  • tests/integration/slashing/slashing_test.go (1 hunks)
  • tests/integration/staking/keeper/deterministic_test.go (13 hunks)
  • tests/integration/tx/aminojson/aminojson_test.go (1 hunks)
  • testutil/network/network.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • CHANGELOG.md
  • tests/integration/tx/aminojson/aminojson_test.go
  • testutil/network/network.go
Additional context used
Path-based instructions (5)
tests/integration/gov/common_test.go (3)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

tests/integration/slashing/slashing_test.go (3)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

simapp/v2/simdv2/cmd/testnet.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/simd/cmd/testnet.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

tests/integration/staking/keeper/deterministic_test.go (3)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (7)
tests/integration/slashing/slashing_test.go (2)

85-85: Test structure adheres to the Uber Golang style guide.

The test is well-structured and follows best practices for Go testing, including the use of require for assertions and clear separation of setup, execution, and verification phases. This structure helps in maintaining readability and ensuring that each part of the test is focused on a specific aspect of the functionality.


85-85: Verify the correct instantiation of the metadata parameter.

The metadata parameter is instantiated as an empty struct, which is correct for the purpose of this test. However, to fully test the functionality, consider adding a test case where metadata contains actual data (e.g., a valid URL to a profile picture). This would ensure that the system correctly handles and stores this new type of data.

Verification successful

Metadata parameter is tested with actual data elsewhere.

The stakingtypes.Metadata parameter is instantiated with actual data in tests/integration/staking/keeper/deterministic_test.go, which ensures that the handling of non-empty metadata is tested. While the current test in tests/integration/slashing/slashing_test.go correctly uses an empty metadata struct, consider adding a test case with non-empty metadata for completeness and enhanced coverage.

  • tests/integration/staking/keeper/deterministic_test.go: Tests with ProfilePicUri and SocialHandleUris.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the metadata parameter is correctly instantiated and handled.

# Test: Search for tests that instantiate the metadata parameter with actual data.
rg --type go -A 5 $'stakingtypes.Metadata{'

Length of output: 152


Script:

#!/bin/bash
# Description: Verify that the metadata parameter is correctly instantiated and handled.

# Test: Search for tests that instantiate the metadata parameter with actual data.
rg --type go -A 5 $'stakingtypes.Metadata\\{'

Length of output: 5566

simapp/v2/simdv2/cmd/testnet.go (1)

298-298: Review the addition of the metadata field in validator descriptions.

The change at line 298 introduces a new stakingtypes.Metadata{} instance in the stakingtypes.NewDescription function call. This aligns with the PR's objectives to enhance validator descriptions with additional metadata, such as profile pictures. However, it's important to verify whether this metadata is intended to be populated dynamically elsewhere in the code or if further modifications are needed to handle the metadata effectively.

Verification successful

Ensure dynamic population of metadata in validator descriptions.

The stakingtypes.Metadata{} instance in simapp/v2/simdv2/cmd/testnet.go is currently a placeholder. While the metadata structure is capable of holding data such as profile pictures and social handles, as seen in the test file deterministic_test.go, it is important to ensure that this metadata is populated dynamically or in the appropriate contexts within the application. Consider reviewing where and how metadata should be populated to fully utilize its capabilities.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the metadata is populated dynamically elsewhere in the code.

# Test: Search for assignments to `stakingtypes.Metadata` fields. Expect: Assignments that populate the metadata.
ast-grep --lang go --pattern $'stakingtypes.Metadata { $$$ }'

Length of output: 1632

simapp/simd/cmd/testnet.go (1)

356-356: Approve addition of metadata field in validator descriptions.

The addition of the stakingtypes.Metadata{} parameter in the NewDescription function call aligns with the PR's objectives to enhance validator descriptions with profile pictures. This change is crucial for reducing reliance on external services like Keybase and improving the management of validator profile images.

Please ensure that the integration of the new metadata field is thoroughly tested, especially in environments where the validator descriptions are consumed, such as block explorers and wallets.

tests/integration/staking/keeper/deterministic_test.go (3)

212-230: Review of the metadataGenerator and generateUri functions

The metadataGenerator function is well-implemented, using the rapid library to generate random metadata for testing. The function correctly constructs a Metadata object with a profile picture URI and social handle URIs, which aligns with the PR's objectives to handle validator profile pictures more effectively.

The generateUri function also appears correctly implemented. It uses the rapid library to generate random strings for the host and path, and constructs a URL using the net/url package. This ensures that the URIs are valid and well-formed.

Overall, these changes are consistent with the PR's goal of enhancing the metadata handling in validator information. The use of property-based testing with rapid is appropriate for this context, ensuring a robust test suite.


251-251: Integration of metadataGenerator in createValidator function

The integration of metadataGenerator within the createValidator function is crucial for testing the new metadata functionality. By drawing a Metadata instance during validator creation, the test effectively simulates the real-world usage of this new feature.

This change is essential for ensuring that the metadata field is populated correctly and behaves as expected under various conditions. It's a good practice to include such integrations in unit tests to catch potential issues early in the development cycle.


327-327: Static Validator Functions with Default Metadata

The updates to getStaticValidator and getStaticValidator2 functions to include a default stakingtypes.Metadata{} instance are a necessary adjustment to maintain consistency with the new metadata field in the validator struct.

These changes ensure that even static validators used in tests have the metadata field populated, albeit with an empty default value. This is important for ensuring that all parts of the test suite acknowledge and support the new metadata structure, preventing any regressions or overlooked bugs related to uninitialized fields.

Also applies to: 363-363

tests/integration/gov/common_test.go Show resolved Hide resolved
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! only the changelog should be fixed

CHANGELOG.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

All those should be moved under staking/CHANGELOG.md

@julienrbrt
Copy link
Member

You should run make lint-fix as well

@github-actions github-actions bot added C:server/v2 Issues related to server/v2 C:server/v2 api labels Sep 21, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Outside diff range and nitpick comments (2)
x/staking/simulation/rand_util.go (2)

12-24: Add unit tests for the function.

The function does not have any unit tests. Unit tests are essential to ensure the correctness and reliability of the function.

Consider adding unit tests to cover various scenarios, including edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


27-36: Add unit tests for the function.

The function does not have any unit tests. Unit tests are essential to ensure the correctness and reliability of the function.

Consider adding unit tests to cover various scenarios, including edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 05bea00 and 1862040.

Files selected for processing (3)
  • x/staking/simulation/msg_factory.go (2 hunks)
  • x/staking/simulation/rand_util.go (1 hunks)
  • x/staking/simulation/rand_util_test.go (1 hunks)
Additional context used
Path-based instructions (3)
x/staking/simulation/msg_factory.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/simulation/rand_util.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/staking/simulation/rand_util_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (4)
x/staking/simulation/rand_util_test.go (2)

14-39: LGTM!

The test function TestRandURIOfHostLength is well-structured and follows the Golang testing conventions. It uses a table-driven approach to test the RandURIOfHostLength function with different input sizes, including edge cases. The test correctly parses the generated URI and checks the host length using the require package for assertions.


41-60: LGTM!

The test function TestRandSocialHandleURIs is well-structured and follows the Golang testing conventions. It uses a table-driven approach to test the RandSocialHandleURIs function with different numbers of social handles. The test correctly checks the length of the generated social handle URIs using the require package for assertions.

x/staking/simulation/msg_factory.go (2)

42-52: LGTM!

The changes to the MsgCreateValidatorFactory function look good. The types.NewDescription method now includes a types.Metadata parameter, which is populated with a ProfilePicUri and SocialHandleUris. This enhances the validator's description with profile and social media information.


146-149: LGTM!

The changes to the MsgEditValidatorFactory function look good. The types.NewDescription method now includes a types.Metadata parameter, which is populated with a ProfilePicUri and SocialHandleUris. This enhances the edited validator's description with profile and social media information.

if n == 0 {
return ""
}
tld := ".com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider making the TLD configurable.

Using a fixed TLD of ".com" might not be suitable for all use cases. Consider making the TLD configurable by accepting it as a parameter or using a constant that can be easily modified.

@tac0turtle tac0turtle added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 814d4cf Oct 1, 2024
70 of 73 checks passed
@tac0turtle tac0turtle deleted the ziscky/9988-staking-validator-metadata branch October 1, 2024 07:41
alpe added a commit that referenced this pull request Oct 1, 2024
* main:
  docs: amend docs for 52 changes  (#21992)
  test: migrate e2e/authz to system tests (#21819)
  refactor(runtime/v2): use StoreBuilder (#21989)
  feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482)
  docs: add instructions to change DefaultGenesis (#21680)
  feat(x/staking)!: Add metadata field to validator info (#21315)
  chore(x/authz)!: Remove account keeper dependency (#21632)
alpe added a commit that referenced this pull request Oct 1, 2024
* main:
  docs: amend docs for 52 changes  (#21992)
  test: migrate e2e/authz to system tests (#21819)
  refactor(runtime/v2): use StoreBuilder (#21989)
  feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482)
  docs: add instructions to change DefaultGenesis (#21680)
  feat(x/staking)!: Add metadata field to validator info (#21315)
  chore(x/authz)!: Remove account keeper dependency (#21632)
  chore(contributing): delete link (#21990)
  test(gov): Migrate e2e to system test (#21927)
  test: e2e/client to system tests (#21981)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/Staking: make validator info have an explicit link to profile pictures
5 participants