fix(x/staking): make metadata nullable #22556
Conversation
julienrbrt
requested review from
facundomedica,
testinginprod,
JulianToledano,
ziscky and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
x/staking/client/cli/utils.go (1)
137-147: Consider a more concise implementationThe function could be simplified while maintaining the same functionality.
func buildMetadata(profilePicUri string, socialHandlesUris []string) (*types.Metadata, error) { - metadata := types.Metadata{ - ProfilePicUri: profilePicUri, - SocialHandleUris: socialHandlesUris, - } - - if err := metadata.Validate(); err != nil { - return nil, err - } - - return &metadata, nil + metadata := &types.Metadata{ + ProfilePicUri: profilePicUri, + SocialHandleUris: socialHandlesUris, + } + if err := metadata.Validate(); err != nil { + return nil, err + } + return metadata, nil }x/staking/CHANGELOG.md (1)
37-40: Enhance clarity of the metadata changes in the changelog entryThe changelog entry should be more explicit about the nullable aspect of metadata, which is the key change in PR #22556. Consider revising the entry as follows:
-* [#21315](https://github.com/cosmos/cosmos-sdk/pull/21315), [#22556](https://github.com/cosmos/cosmos-sdk/pull/22556) Create metadata type and add metadata field in validator details proto - * Add parsing of `metadata-profile-pic-uri` in `create-validator` JSON. - * Add cli flag: `metadata-profile-pic-uri` to `edit-validator` cmd. +* [#21315](https://github.com/cosmos/cosmos-sdk/pull/21315), [#22556](https://github.com/cosmos/cosmos-sdk/pull/22556) Create metadata type and make it optional in validator details proto + * Make metadata field nullable to avoid migration requirements (PR #22556) + * Add parsing of `metadata-profile-pic-uri` in `create-validator` JSON + * Add cli flag: `metadata-profile-pic-uri` to `edit-validator` cmdx/staking/client/cli/tx.go (1)
404-404: Consider optimizing metadata handling in configurationWhile the change correctly passes metadata as a pointer, the
TxCreateValidatorConfigstruct still stores metadata as a value type. Consider updating the config struct to store metadata as a pointer for consistency and to avoid unnecessary copying.type TxCreateValidatorConfig struct { // ... other fields ... - Metadata types.Metadata + Metadata *types.Metadata }x/staking/simulation/msg_factory.go (1)
146-149: Improve code formatting while maintaining functionalityWhile the metadata pointer conversion is correct, the code readability could be improved by breaking down the long line.
Consider restructuring for better readability:
- d := types.NewDescription(r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), &types.Metadata{ - ProfilePicUri: RandURIOfHostLength(r.Rand, 10), - SocialHandleUris: RandSocialHandleURIs(r.Rand, 2, 10), - }) + metadata := &types.Metadata{ + ProfilePicUri: RandURIOfHostLength(r.Rand, 10), + SocialHandleUris: RandSocialHandleURIs(r.Rand, 2, 10), + } + d := types.NewDescription( + r.StringN(10), + r.StringN(10), + r.StringN(10), + r.StringN(10), + r.StringN(10), + metadata, + )x/staking/types/validator.go (1)
Line range hint
192-199: Critical: Add nil checks to prevent panic.The
Descriptionmethods accessMetadatafields directly which could cause nil pointer dereference. Add nil checks in the following methods:
IsEmpty()UpdateDescription()Validate()Apply this diff to fix the issue:
func (d Description) IsEmpty() bool { - return d.Moniker == "" && d.Details == "" && d.Identity == "" && d.Website == "" && d.SecurityContact == "" && - d.Metadata.ProfilePicUri == "" && len(d.Metadata.SocialHandleUris) == 0 + isEmpty := d.Moniker == "" && d.Details == "" && d.Identity == "" && d.Website == "" && d.SecurityContact == "" + if d.Metadata == nil { + return isEmpty + } + return isEmpty && d.Metadata.ProfilePicUri == "" && len(d.Metadata.SocialHandleUris) == 0 } func (d Description) Validate() (Description, error) { - if err := d.Metadata.Validate(); err != nil { - return d, err + if d.Metadata != nil { + if err := d.Metadata.Validate(); err != nil { + return d, err + } } return d.EnsureLength() } func (d Description) UpdateDescription(d2 Description) (Description, error) { // ... existing code ... - if d2.Metadata.ProfilePicUri == DoNotModifyDesc { - d2.Metadata.ProfilePicUri = d.Metadata.ProfilePicUri + if d2.Metadata != nil && d.Metadata != nil { + if d2.Metadata.ProfilePicUri == DoNotModifyDesc { + d2.Metadata.ProfilePicUri = d.Metadata.ProfilePicUri + } } return NewDescription( d2.Moniker, d2.Identity, d2.Website, d2.SecurityContact, d2.Details, d.Metadata, ).Validate() }Also applies to: 266-267, 271-272
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/staking/types/staking.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
api/cosmos/staking/v1beta1/staking.pulsar.go(1 hunks)x/staking/CHANGELOG.md(1 hunks)x/staking/client/cli/tx.go(2 hunks)x/staking/client/cli/utils.go(2 hunks)x/staking/proto/cosmos/staking/v1beta1/staking.proto(1 hunks)x/staking/simulation/msg_factory.go(2 hunks)x/staking/types/validator.go(1 hunks)
🔥 Files not summarized due to errors (1)
- api/cosmos/staking/v1beta1/staking.pulsar.go: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
📓 Path-based instructions (6)
api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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/client/cli/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/types/validator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
x/staking/client/cli/utils.go (2)
27-27: LGTM: Metadata field correctly changed to pointer type
The change to make Metadata a pointer type (*types.Metadata) aligns with the PR objective of making metadata nullable to avoid validator migrations.
137-147: LGTM: Function correctly updated to return pointer type
The function properly handles metadata validation and correctly returns a pointer type, maintaining consistency with the struct changes.
x/staking/client/cli/tx.go (2)
136-139: LGTM: Correct pointer initialization for nullable metadata
The change correctly initializes metadata as a pointer type, which aligns with making the field nullable as intended.
136-139: Verify metadata handling in dependent code
The changes to make metadata nullable could affect code that assumes metadata is always present. Let's verify the handling in related code.
Also applies to: 404-404
✅ Verification successful
Let me search for more specific information about metadata handling.
Let me search for the actual metadata validation logic to understand how null values are handled.
Based on the gathered information, I can now provide a final response:
Metadata changes are safely handled in the codebase
The changes to make metadata nullable are properly supported by the codebase because:
- The
Description.Validate()method safely handles metadata validation by checkingMetadata.Validate() - The
Metadata.Validate()method only performs validation when fields are non-empty - Proper nil checks exist in protobuf generated code with automatic initialization
- No direct metadata field access without proper null safety checks was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct metadata field access that might need null checks
rg -A 2 "\.Metadata\." --type go
# Search for NewDescription usage to ensure consistent pointer handling
ast-grep --pattern 'NewDescription($$$, $m)'
Length of output: 24645
Script:
#!/bin/bash
# Search for metadata validation and handling
rg -A 3 "Validate\(\)" x/staking/types/metadata.go || true
rg -A 3 "NewMetadata" x/staking/types/ || true
# Search for null/nil checks on metadata
rg -A 2 "if\s+.*\.Metadata\s*==\s*nil" --type go || true
rg -A 2 "if\s+.*\.Metadata\s*!=\s*nil" --type go || true
# Search for metadata initialization patterns
ast-grep --pattern 'Metadata: &types.Metadata{$$$}'
Length of output: 6849
Script:
#!/bin/bash
# Search for metadata validation in staking types
rg -A 5 "func.*Validate" x/staking/types/ || true
# Search for metadata field usage in validator operations
rg -A 3 "Description.*Metadata" x/staking/types/ || true
# Look for any metadata-related error definitions
rg "ErrInvalid.*Metadata" x/staking/types/ || true
Length of output: 4811
x/staking/simulation/msg_factory.go (1)
Line range hint 49-53: LGTM: Metadata pointer conversion looks good
The change correctly implements the metadata as a pointer type, aligning with the PR's objective to make metadata nullable.
x/staking/types/validator.go (2)
Line range hint 192-199: LGTM on the signature change.
The change to make metadata parameter a pointer type aligns with the PR objective to make metadata nullable.
Line range hint 192-199: Verify the impact of this change.
Since this is a breaking change in the function signature, we need to verify all callers of NewDescription have been updated.
✅ Verification successful
Let me gather more context about the usage of NewDescription to ensure we have a complete picture.
All callers of NewDescription have been updated to handle the metadata parameter correctly
After analyzing all usages of NewDescription across the codebase, I can confirm that all callers are passing the metadata parameter correctly. Every call site either:
- Passes a pointer to a
Metadatastruct directly - Uses
&types.Metadata{}for an empty metadata - Uses
&stakingtypes.Metadata{}for an empty metadata
The function signature change has been properly propagated throughout the codebase with no inconsistencies found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of NewDescription to verify they handle the pointer type correctly
ast-grep --pattern 'NewDescription($$$)'
Length of output: 437
Script:
#!/bin/bash
# Find test files that might use NewDescription
rg "NewDescription" -l --type go
# Show the actual calls with context
rg "NewDescription\(" -A 3 -B 3
Length of output: 13690
x/staking/proto/cosmos/staking/v1beta1/staking.proto (1)
82-82: LGTM! The metadata field changes align with PR objectives.
The modification to make the metadata field nullable and remove dont_omitempty is a good solution to avoid requiring validator migrations. This change maintains backward compatibility while allowing the field to be optional.
Let's verify the impact of this change on the validator descriptions across the codebase:
✅ Verification successful
Let me analyze the usage of metadata field across the codebase to verify if making it nullable has any impact.
Let me check how the metadata field is used in validator updates and message handling to ensure the nullable change is safe.
Based on the analysis of the codebase, I can now provide a final response about the metadata field change.
Making metadata nullable is safe and aligns with the PR objectives
The change to make the metadata field nullable in the Description message is safe because:
- The field is already handled as optional in validator updates and message processing
- The Description struct's validation logic includes metadata validation
- The change maintains backward compatibility since:
- Existing validators without metadata will now be properly represented
- The amino.dont_omitempty=false ensures proper serialization behavior
- No breaking changes in how metadata is processed in validator operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for validator description usage patterns to ensure compatibility
# Search for validator description instantiation patterns
rg -A 5 "NewDescription|Description\{"
# Search for metadata field access patterns
rg -A 3 "Description.*[.]metadata|[.]Metadata"
# Search for any direct metadata field assignments
ast-grep --pattern 'metadata = $_'
Length of output: 248199
Script:
#!/bin/bash
# Check for direct metadata field access and assignments
rg -A 3 "Description.*metadata" x/staking/
Length of output: 891
Script:
#!/bin/bash
# Check validator message handling and metadata updates
rg -A 5 "MsgEditValidator" x/staking/
rg -A 5 "UpdateValidator" x/staking/
Length of output: 48736
api/cosmos/staking/v1beta1/staking.pulsar.go (1)
16178-16178: LGTM! The metadata field is correctly marked as nullable.
The protobuf definition correctly implements the nullable metadata field with the proto3 optional marker, which aligns with the PR objective of making the metadata field nullable to avoid migration requirements.
Let's verify the protobuf compilation:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
x/staking/client/cli/utils.go (1)
137-147: LGTM! Consider adding function documentation.The function implementation correctly handles pointer return and validation. Consider adding a godoc comment to document the function's purpose and return values.
Add documentation:
+// buildMetadata creates and validates a new Metadata instance from the provided URIs. +// Returns nil and error if validation fails. func buildMetadata(profilePicUri string, socialHandlesUris []string) (*types.Metadata, error) {x/staking/CHANGELOG.md (1)
37-39: Enhance changelog entry to better reflect the PR's primary objectiveWhile the current entry correctly documents the feature addition, it should also explicitly mention that the metadata field is now nullable, which is the primary objective of PR #22556. This change is significant as it avoids the need for validator migrations.
Consider updating the entry to:
- * [#21315](https://github.com/cosmos/cosmos-sdk/pull/21315), [#22556](https://github.com/cosmos/cosmos-sdk/pull/22556) Create metadata type and add metadata field in validator details proto + * [#21315](https://github.com/cosmos/cosmos-sdk/pull/21315), [#22556](https://github.com/cosmos/cosmos-sdk/pull/22556) Create metadata type and add nullable metadata field in validator details proto to avoid validator migrationsx/staking/simulation/msg_factory.go (1)
146-149: Consider improving readability while maintaining functionalityWhile the metadata pointer implementation is correct, consider splitting the description creation for better readability:
- d := types.NewDescription(r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), &types.Metadata{ - ProfilePicUri: RandURIOfHostLength(r.Rand, 10), - SocialHandleUris: RandSocialHandleURIs(r.Rand, 2, 10), - }) + metadata := &types.Metadata{ + ProfilePicUri: RandURIOfHostLength(r.Rand, 10), + SocialHandleUris: RandSocialHandleURIs(r.Rand, 2, 10), + } + d := types.NewDescription( + r.StringN(10), + r.StringN(10), + r.StringN(10), + r.StringN(10), + r.StringN(10), + metadata, + )x/staking/types/validator.go (2)
Line range hint
237-237: Critical: Add nil check for Metadata to prevent panic.Direct access to
d2.Metadata.ProfilePicUricould cause a nil pointer dereference ifd2.Metadatais nil, which is now possible with the nullable metadata change.Add a nil check before accessing the metadata:
-if d2.Metadata.ProfilePicUri == DoNotModifyDesc { +if d2.Metadata != nil && d2.Metadata.ProfilePicUri == DoNotModifyDesc {
Breaking change: NewDescription callers need to be updated to use pointer type for metadata
The verification reveals that all current callers of
NewDescriptionare passing non-pointerMetadata{}values, which is incompatible with the new pointer parameter type*Metadata. This needs to be fixed in:
x/staking/client/cli/tx.gox/staking/simulation/msg_factory.gox/genutil/types/genesis_state_test.gotests/integration/staking/keeper/*tests/integration/slashing/slashing_test.gotests/integration/gov/common_test.gotestutil/network/network.gosimapp/v2/simdv2/cmd/testnet.gosimapp/simd/cmd/testnet.go🔗 Analysis chain
Line range hint
192-200: LGTM: Metadata parameter correctly changed to pointer type.The change aligns with the PR objective of making metadata nullable to avoid validator migration.
Let's verify all callers of
NewDescriptionhave been updated to handle the pointer type:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to NewDescription to ensure they handle the pointer type correctly ast-grep --pattern 'NewDescription($$$, metadata)'Length of output: 52
Script:
#!/bin/bash # Search for all calls to NewDescription with any pattern to analyze the usage rg "NewDescription\(" -A 2 # Also search for test files that might use NewDescription rg "NewDescription\(" --type go --glob "*_test.go" -A 2Length of output: 10383
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/staking/types/staking.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
api/cosmos/staking/v1beta1/staking.pulsar.go(1 hunks)x/staking/CHANGELOG.md(1 hunks)x/staking/client/cli/tx.go(2 hunks)x/staking/client/cli/utils.go(2 hunks)x/staking/proto/cosmos/staking/v1beta1/staking.proto(1 hunks)x/staking/simulation/msg_factory.go(2 hunks)x/staking/types/validator.go(1 hunks)
🔥 Files not summarized due to errors (1)
- api/cosmos/staking/v1beta1/staking.pulsar.go: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
📓 Path-based instructions (6)
api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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/client/cli/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/types/validator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
x/staking/client/cli/utils.go (1)
27-27: LGTM! Metadata field change aligns with nullable requirement.
The change from types.Metadata to *types.Metadata correctly implements the nullable metadata requirement.
Let's verify consistent pointer usage across the codebase:
✅ Verification successful
Let me search for any remaining non-pointer Metadata fields that might need to be updated for consistency.
Let me check the specific Metadata field declarations in these files to verify if they need to be updated.
The Metadata field change is consistent with the codebase
The change from types.Metadata to *types.Metadata in x/staking/client/cli/utils.go is correct and consistent with the rest of the codebase. The search results show that:
- The base
Metadatatype inx/staking/types/staking.pb.gois already defined as a pointer in the Description struct - The proto definition in
staking.protohas the field marked as nullable - Only one remaining non-pointer usage was found in
tx.gowhich appears to be a temporary struct for CLI command handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining non-pointer Metadata field declarations
# that might need to be updated for consistency
# Search for Metadata field declarations in struct definitions
ast-grep --pattern 'type $_ struct {
$$$
Metadata $_ $$$
$$$
}'
Length of output: 75857
Script:
#!/bin/bash
# Search for Metadata field declarations in validator-related files
rg -l "Metadata\s+[^*]" x/staking/
Length of output: 176
Script:
#!/bin/bash
# Show the context around Metadata fields in these files
rg -B2 -A2 "Metadata\s+[^*]" x/staking/
Length of output: 3823
x/staking/client/cli/tx.go (2)
404-404: LGTM! Correct pointer usage in NewDescription call.
The change maintains consistency with the nullable metadata pattern.
136-139: LGTM! Metadata initialization as pointer type is correct.
The change aligns with making metadata nullable and maintains proper initialization of the struct fields.
Let's verify consistent pointer usage across the codebase:
✅ Verification successful
Let me gather more information to verify the metadata pointer usage specifically in the staking module.
Let me gather more specific information about metadata usage in the staking module.
Metadata pointer type implementation is consistent
The verification confirms that:
- The
Metadatafield inDescriptionstruct is correctly defined as a pointer type (*Metadata) - The
NewDescriptionfunction signature has been updated to accept metadata as a pointer - The initialization in the CLI matches the expected pointer usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining non-pointer metadata usage
# Expect: All metadata declarations should use pointer type
# Search for Metadata struct usage
ast-grep --pattern 'Metadata {$$$}'
# Search for NewDescription calls
ast-grep --pattern 'NewDescription($$$)'
Length of output: 2160
Script:
#!/bin/bash
# Check metadata field declarations and usages in staking types
ast-grep --pattern 'type $_ struct {
$$$
Metadata $_
$$$
}'
# Check metadata assignments in staking module
rg "Metadata =" x/staking/ -A 2
# Check for any direct metadata instantiation
rg "Metadata{" x/staking/
Length of output: 79797
Script:
#!/bin/bash
# Check for metadata field declarations in staking types
rg "type.*Metadata.*struct" x/staking/
# Check metadata field usage in Description struct
rg "type.*Description.*struct" x/staking/ -A 15
# Check metadata assignments in validator operations
rg "NewDescription.*Metadata" x/staking/
Length of output: 2106
x/staking/simulation/msg_factory.go (1)
Line range hint 49-53: LGTM: Metadata pointer implementation is correct
The change correctly implements the metadata as a pointer type, aligning with the PR objective to make metadata nullable. The random data generation for URIs is appropriate for simulation purposes.
x/staking/proto/cosmos/staking/v1beta1/staking.proto (1)
82-82: LGTM! The change aligns with the PR objectives.
Making the metadata field nullable and omitempty is the right approach as it:
- Avoids requiring migrations for validators
- Makes sense for optional validator information
- Maintains backwards compatibility
Let's verify the impact on API clients:
✅ Verification successful
Making metadata nullable is safe and properly handled in the codebase
The code inspection shows that metadata handling is properly implemented:
- The
NewDescriptionfunction accepts a nullable metadata parameter - Metadata validation is performed only when metadata is non-nil
- The code has proper nil checks before accessing metadata fields (e.g.,
d.Metadata.ProfilePicUri) - The DoNotModifyDesc pattern preserves existing metadata values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usages of validator metadata in the codebase
# to ensure clients can handle nil metadata
# Look for metadata field access in Go files
rg -t go "\.Metadata\b"
# Look for metadata field access in gRPC/REST handlers
rg -t go "Description.*Metadata" "x/staking/keeper" "x/staking/client"
Length of output: 74016
Script:
#!/bin/bash
# Let's search for validator metadata handling in staking module
# to verify if the code properly handles nil metadata
# Look for metadata field access in validator-related code
rg -t go "Description.*Metadata|\.Metadata\." x/staking/types/validator.go x/staking/keeper/
# Look for metadata validation code
rg -t go "Validate.*Metadata" x/staking/types/
# Look for metadata field in gRPC/REST handlers
rg -t go "Description.*Metadata" x/staking/client/
Length of output: 683
api/cosmos/staking/v1beta1/staking.pulsar.go (1)
16178-16178: Skip review of generated protobuf code
This is an auto-generated file from protobuf definitions. The changes reflect the metadata field being made nullable, which aligns with the PR objectives.
Let's verify the corresponding .proto file to ensure the nullable option is correctly set:
#!/bin/bash
# Description: Verify the proto definition for the metadata field
# Expected: Should find the metadata field definition with nullable option
# Search for the proto file and metadata field definition
fd -e proto | xargs rg -l "cosmos.staking.v1beta1" | xargs rg -A 5 "metadata.*Metadata"
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/genutil/types/genesis_state_test.go (1)
69-69: Consider adding test case with nil metadataWhile the change correctly updates the metadata parameter to use a pointer type, consider adding a test case where metadata is nil to ensure the nullable behavior works as expected.
Example addition:
func TestValidateGenesisNilMetadata(t *testing.T) { desc := stakingtypes.NewDescription("testname", "", "", "", "", nil) // ... rest of the test setup require.NoError(t, err) // should succeed with nil metadata }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
testutil/network/network.go(1 hunks)x/genutil/gentx_test.go(1 hunks)x/genutil/types/genesis_state_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
testutil/network/network.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/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"
🔇 Additional comments (3)
x/genutil/types/genesis_state_test.go (1)
42-42: LGTM! Verify test coverage for nullable metadata
The change correctly updates the metadata parameter to use a pointer type, aligning with the PR's objective of making metadata nullable.
Let's verify the test coverage for both nil and non-nil metadata cases:
x/genutil/gentx_test.go (2)
39-39: LGTM: Metadata pointer change aligns with PR objectives
The modification to use a pointer for metadata in NewDescription correctly implements the nullable metadata requirement.
Line range hint 286-295: LGTM: Well-structured test implementations
The new GenesisState1 and GenesisState2 types provide clear test cases for both error and success scenarios in genesis transaction handling.
| @@ -493,7 +493,7 @@ func New(l Logger, baseDir string, cfg Config) (NetworkI, error) { | |||
| sdk.ValAddress(addr).String(), | |||
| valPubKeys[i], | |||
| sdk.NewCoin(cfg.BondDenom, cfg.BondedTokens), | |||
| stakingtypes.NewDescription(nodeDirName, "", "", "", "", stakingtypes.Metadata{}), | |||
| stakingtypes.NewDescription(nodeDirName, "", "", "", "", &stakingtypes.Metadata{}), | |||
There was a problem hiding this comment.
💡 Codebase verification
Several test files need to be updated to use pointer metadata
The NewDescription function in x/staking/types/validator.go expects a pointer *Metadata, but some test files are still passing value types. The following locations need to be updated to use &stakingtypes.Metadata{}:
tests/integration/slashing/slashing_test.go:stakingtypes.NewDescription("foo_moniker", "", "", "", "", stakingtypes.Metadata{})tests/integration/staking/keeper/msg_server_test.go:types.NewDescription("Validator", "", "", "", "", types.Metadata{})tests/integration/staking/keeper/genesis_test.go: Multiple occurrences oftypes.NewDescription(..., types.Metadata{})tests/integration/gov/common_test.go:stakingtypes.NewDescription("T", "E", "S", "T", "Z", stakingtypes.Metadata{})simapp/simd/cmd/testnet.go:stakingtypes.NewDescription(nodeDirName, "", "", "", "", stakingtypes.Metadata{})
🔗 Analysis chain
LGTM! Change aligns with making metadata nullable.
The modification to use a pointer for metadata aligns with the PR objective of making the field nullable, which helps avoid the need for validator migrations.
Let's verify that this change doesn't break existing tests:
The search results show that there are several test files and simulation code using NewDescription with metadata. I notice that some files are already using a pointer (&stakingtypes.Metadata{}), while others are using value types (stakingtypes.Metadata{}). Let's verify if the NewDescription function signature has been updated to expect a pointer consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test cases that might be affected by the metadata change
rg -l "NewDescription.*Metadata" --type go | while read -r file; do
echo "=== $file ==="
rg -A 5 "NewDescription.*Metadata" "$file"
done
Length of output: 6401
Script:
#!/bin/bash
# Check the NewDescription function signature in the staking types
ast-grep --pattern 'func NewDescription($$$) Description'
Length of output: 708
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/integration/gov/common_test.go (1)
37-37: LGTM! The pointer change aligns with making metadata nullable.The change from value to pointer type for
Metadatacorrectly implements the PR objective of making metadata nullable.Consider adding test cases that explicitly use
nilmetadata to ensure the nullable functionality is properly tested. For example:var ( valTokens = sdk.TokensFromConsensusPower(42, sdk.DefaultPowerReduction) TestProposal = v1beta1.NewTextProposal("Test", "description") TestDescription = stakingtypes.NewDescription("T", "E", "S", "T", "Z", &stakingtypes.Metadata{}) + TestDescriptionNil = stakingtypes.NewDescription("T", "E", "S", "T", "Z", nil) TestCommissionRates = stakingtypes.NewCommissionRates(math.LegacyZeroDec(), math.LegacyZeroDec(), math.LegacyZeroDec()) )tests/integration/slashing/slashing_test.go (1)
86-86: Consider adding test cases for metadata nullability.While the current test covers basic validator operations, consider adding test cases that explicitly verify:
- Creating a validator with nil metadata
- Creating a validator with non-nil metadata
- Updating a validator's metadata from nil to non-nil and vice versa
This would ensure the nullable behavior is thoroughly tested.
Example test cases:
// Test with nil metadata description := stakingtypes.NewDescription("foo_moniker", "", "", "", "", nil) // Test with non-nil metadata containing data description := stakingtypes.NewDescription("foo_moniker", "", "", "", "", &stakingtypes.Metadata{ Details: "test details", })tests/integration/staking/keeper/genesis_test.go (1)
Line range hint
1-240: Test coverage assessment for genesis initialization.The test suite comprehensively covers:
- Basic genesis initialization
- Pool balance validation
- Large validator set handling
However, given that this PR makes metadata nullable, consider adding the following test scenarios:
- Genesis initialization with nil metadata
- Export/Import genesis with mixed metadata states
- Validation of metadata state transitions
Would you like me to help implement these additional test cases?
tests/integration/staking/keeper/msg_server_test.go (1)
47-47: LGTM! Consider adding test cases for nil metadata.The change to use a pointer for metadata aligns with making the field nullable. However, since this is a breaking change, consider adding test cases that verify the behavior with
nilmetadata to ensure complete coverage.Add a test case variation like:
validator, err := types.NewValidator(valAddr.String(), PKs[0], types.NewDescription("Validator", "", "", "", "", &types.Metadata{})) +// Add test case with nil metadata +validatorNilMeta, err := types.NewValidator(valAddr.String(), PKs[0], types.NewDescription("Validator", "", "", "", "", nil))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
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(3 hunks)tests/integration/staking/keeper/genesis_test.go(4 hunks)tests/integration/staking/keeper/msg_server_test.go(1 hunks)x/staking/types/validator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/staking/types/validator.go
🧰 Additional context used
📓 Path-based instructions (7)
simapp/simd/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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"
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"
tests/integration/staking/keeper/genesis_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/staking/keeper/msg_server_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 (9)
tests/integration/gov/common_test.go (1)
37-37: Verify test coverage for nullable metadata functionality.
Let's ensure there's adequate test coverage for handling nil metadata across the test suite.
✅ Verification successful
Adequate test coverage exists for nullable metadata functionality
Based on the search results, there is sufficient test coverage for handling metadata across the codebase:
- The nullable metadata functionality is well tested in:
tests/integration/staking/keeper/genesis_test.go: Tests both nil and non-nil metadata casestests/integration/staking/keeper/msg_server_test.go: Tests validator creation with metadatatests/integration/gov/common_test.go: Tests description with metadatatests/integration/slashing/slashing_test.go: Tests validator description with metadata
The test coverage appears comprehensive, testing both the happy path (with metadata) and edge cases (nil metadata) across different modules and integration scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for nil metadata scenarios
# Search for test cases involving metadata in staking tests
echo "Searching for metadata-related test cases..."
rg -l "NewDescription.*Metadata" "tests/**/*_test.go"
# Search for nil metadata test cases
echo "Searching for nil metadata test cases..."
rg "NewDescription.*nil" "tests/**/*_test.go"
Length of output: 411
Script:
#!/bin/bash
# Let's try a different approach using fd to find test files first
echo "Finding test files..."
fd -e go -e GO ".*_test\.go$"
# Then search for metadata-related test cases
echo "Searching for metadata-related test cases..."
rg "NewDescription.*Metadata" $(fd -e go -e GO ".*_test\.go$")
# Search for nil metadata test cases
echo "Searching for nil metadata test cases..."
rg "NewDescription.*nil" $(fd -e go -e GO ".*_test\.go$")
# Let's also check for any other metadata-related tests
echo "Searching for other metadata-related tests..."
rg "Metadata" $(fd -e go -e GO ".*_test\.go$")
Length of output: 163915
tests/integration/slashing/slashing_test.go (1)
86-86: LGTM: Pointer-based metadata change aligns with PR objectives.
The change to use &stakingtypes.Metadata{} correctly implements the pointer-based approach for nullable metadata.
tests/integration/staking/keeper/genesis_test.go (1)
44-44: LGTM! Consistent metadata pointer usage across validator descriptions.
The changes correctly implement nullable metadata by consistently using &types.Metadata{} across all validator descriptions in the test cases. This aligns with the PR objective of making metadata nullable.
Also applies to: 70-70, 78-78
simapp/v2/simdv2/cmd/testnet.go (1)
298-298: LGTM: Metadata pointer change aligns with PR objectives
The change from stakingtypes.Metadata{} to &stakingtypes.Metadata{} correctly implements the nullable metadata requirement for validators.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Metadata pointer change is consistently implemented across the codebase
Based on the search results, all instances of NewDescription in the codebase consistently use &stakingtypes.Metadata{} as the metadata parameter, aligning with the PR's objective of making metadata nullable. The change in simapp/v2/simdv2/cmd/testnet.go is part of this consistent implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining non-pointer Metadata usage in NewDescription calls
# Expected: No instances of direct Metadata struct usage in NewDescription
# Search for NewDescription calls with non-pointer Metadata
ast-grep --pattern 'NewDescription($$$, $$$, $$$, $$$, $$$, Metadata{$$$})'
# Search for NewDescription calls to ensure they use pointer Metadata
ast-grep --pattern 'NewDescription($$$, $$$, $$$, $$$, $$$, &Metadata{$$$})'
Length of output: 156
Script:
#!/bin/bash
# Let's try a different approach to find NewDescription usage patterns
# Search for NewDescription function calls with broader context
rg "NewDescription\(" -A 2 -B 2
# Also search for any Metadata-related patterns in the codebase
rg "Metadata\{" -A 1 -B 1
# Search for validator-related test files that might use NewDescription
fd "validator.*_test\.go$" --exec rg "NewDescription" {} -A 2 -B 2
Length of output: 49379
simapp/simd/cmd/testnet.go (2)
Line range hint 12-12: LGTM: New logging flag and metadata pointer changes
The changes look good:
- The new
flagEnableLoggingis well-implemented with a clear description and sensible default. - The metadata pointer change aligns with the PR objective to make metadata nullable.
Also applies to: 67-67, 356-356
356-356: Verify consistent metadata pointer usage across the codebase
Let's ensure all calls to NewDescription have been updated to use pointer syntax for metadata.
✅ Verification successful
All NewDescription calls consistently use pointer syntax for metadata
Based on the search results, all instances of NewDescription across the codebase have been updated to use pointer syntax (&stakingtypes.Metadata{}) for the metadata parameter, including test files, simulation code, and CLI commands. The changes are consistent with the breaking change introduced.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to NewDescription to verify consistent pointer usage
ast-grep --pattern 'stakingtypes.NewDescription($$$)'
# Also search for direct string matches as fallback
rg "NewDescription\(" -A 1
Length of output: 4901
tests/integration/staking/keeper/deterministic_test.go (3)
219-221: LGTM! The metadata generator correctly implements nullable metadata.
The function now returns a pointer to Metadata which aligns with the PR objective of making metadata nullable.
334-334: LGTM! Consistent usage of pointer for empty metadata.
The static validator correctly initializes metadata as a pointer, maintaining consistency with the nullable metadata changes.
370-370: LGTM! Consistent usage of pointer for empty metadata.
The second static validator correctly initializes metadata as a pointer, maintaining consistency with the nullable metadata changes.
| @@ -195,7 +195,7 @@ func TestInitGenesisLargeValidatorSet(t *testing.T) { | |||
| validators[i], err = types.NewValidator( | |||
| sdk.ValAddress(addrs[i]).String(), | |||
| PKs[i], | |||
| types.NewDescription(fmt.Sprintf("#%d", i), "", "", "", "", types.Metadata{}), | |||
| types.NewDescription(fmt.Sprintf("#%d", i), "", "", "", "", &types.Metadata{}), | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for nil metadata scenarios in validator initialization
Based on the verification results, the current test suite lacks explicit test cases for validators with nil metadata or mixed metadata scenarios. The existing tests only cover cases where metadata is always initialized with an empty struct (&types.Metadata{}). To ensure robust handling of metadata:
- Add test cases in
tests/integration/staking/keeper/genesis_test.gofor:- Validators with explicitly nil metadata in their descriptions
- A mix of validators where some have metadata and others don't
- Edge cases around metadata handling during genesis initialization
🔗 Analysis chain
Verify test coverage for large validator set initialization.
The test appropriately verifies genesis initialization with a large validator set (200 validators) and maintains proper test coverage for the metadata changes. However, consider adding test cases that explicitly verify:
- Validators with nil metadata
- Mixed scenarios with some validators having metadata and others without
Let's verify if there are any existing test cases for nil metadata:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases involving nil metadata in validator descriptions
rg -A 5 "NewDescription.*nil.*Metadata" "**/*_test.go"
Length of output: 109
Script:
#!/bin/bash
# Search for test cases involving metadata in validator descriptions
rg -A 5 "NewDescription.*Metadata" tests/
Length of output: 4427
Script:
#!/bin/bash
# Search for test cases involving validator descriptions in test files
fd -e go -e test.go . tests/ --exec ast-grep --pattern 'types.NewDescription($$$)'
Length of output: 84
Script:
#!/bin/bash
# Look for test files containing validator-related tests
fd "validator.*test.go" tests/
Length of output: 140
Script:
#!/bin/bash
# Check validator test files for different metadata test scenarios
rg -B 5 -A 5 "NewDescription" tests/integration/staking/keeper/validator_test.go
Length of output: 80
Script:
#!/bin/bash
# Look for nil metadata cases in validator tests
rg "Description.*nil" tests/integration/staking/keeper/validator_test.go
Length of output: 74
Script:
#!/bin/bash
# Check for metadata-related test assertions
rg "Description.*assert|require" tests/integration/staking/keeper/validator_test.go
Length of output: 147
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
x/staking/keeper/keeper_test.go (6)
Line range hint
353-353: Use appropriate time location intime.DateWhen creating a
time.Timevalue, using an emptytime.Locationvia&time.Location{}may lead to unexpected behavior. It's recommended to usetime.UTCortime.Localfor clarity and correctness.Apply this diff to fix the issue:
- date := time.Date(2023, 8, 21, 14, 33, 1, 0, &time.Location{}) + date := time.Date(2023, 8, 21, 14, 33, 1, 0, time.UTC)
Line range hint
361-362: Avoid variable shadowing oferrinside closureIn the closure passed to
testutil.DiffCollectionsMigration, the variableerris declared using:=, which shadows the outererrvariable. This can lead to subtle bugs and should be avoided. Use=instead of:=to assign to the existingerrvariable.Apply this diff to fix the issue:
- err := s.stakingKeeper.SetUBDQueueTimeSlice(s.ctx, date, nil) + err = s.stakingKeeper.SetUBDQueueTimeSlice(s.ctx, date, nil)
Line range hint
396-397: Avoid variable shadowing oferrinside closureWithin the closure in
TestUnbondingDelegationsMigrationToColls, declaringerrwith:=shadows the outererrvariable. It's best to reuse the existingerrvariable to prevent potential issues.Apply this diff to fix the issue:
- err := s.stakingKeeper.SetUnbondingDelegation(s.ctx, ubd) + err = s.stakingKeeper.SetUnbondingDelegation(s.ctx, ubd)
Line range hint
506-507: Avoid variable shadowing oferrinside closureIn
TestValidatorsMigrationToColls, theerrvariable inside the closure shadows the outererrvariable. This practice can lead to confusion and errors. Replace:=with=to assign to the existing variable.Apply this diff to fix the issue:
- err := s.stakingKeeper.SetValidator(s.ctx, val) + err = s.stakingKeeper.SetValidator(s.ctx, val)
Line range hint
538-539: Avoid variable shadowing oferrinside closureIn the closure within
TestValidatorQueueMigrationToColls, the declaration oferrwith:=creates a new variable, shadowing the outererr. To maintain clarity, use=to assign to the existingerrvariable.Apply this diff to fix the issue:
- err := s.stakingKeeper.SetUnbondingValidatorsQueue(s.ctx, endTime, endHeight, addrs) + err = s.stakingKeeper.SetUnbondingValidatorsQueue(s.ctx, endTime, endHeight, addrs)
Line range hint
572-573: Avoid variable shadowing oferrinside closureIn
TestRedelegationQueueMigrationToColls, theerrvariable is redeclared inside the closure, shadowing the outer scope'serr. It's recommended to use=to assign to the existing variable.Apply this diff to fix the issue:
- err := s.stakingKeeper.SetRedelegationQueueTimeSlice(s.ctx, date, dvvTriplets.Triplets) + err = s.stakingKeeper.SetRedelegationQueueTimeSlice(s.ctx, date, dvvTriplets.Triplets)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/staking/keeper/keeper_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/staking/keeper/keeper_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"
Description
Follow-up of #21315
This proto breaking changes (field addition): #21315 causing issues in our system tests in v0.52.
With the goal of one api module, and always use the latest: https://github.com/cosmos/cosmos-sdk/pull/21315/files#diff-9f270bde8d871462bf19903205a718302aba27ac311f1e3f7cf25af595f544d4R81-R82: https://github.com/cosmos/cosmos-sdk/actions/runs/11914721341/job/33203345359
Additionally, if we don't make it nullable, I think we'll need a migration for validators. Is that correct @aaronc?
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
API Breaking Changes