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.

Sign up for GitHub

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

fix(x/staking): make metadata nullable #22556

Merged
merged 9 commits into from
Nov 26, 2024
Merged

fix(x/staking): make metadata nullable #22556

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5cc08df
fix(x/staking): make metadata nullable
julienrbrt Nov 19, 2024
a4a5a6b
cl
julienrbrt Nov 19, 2024
f4d6a37
fix
julienrbrt Nov 20, 2024
cc6c67b
Merge branch 'main' into julien/nullable
julienrbrt Nov 20, 2024
accb7ec
build
julienrbrt Nov 26, 2024
09e8221
fix isEmpty
julienrbrt Nov 26, 2024
826779d
updates
julienrbrt Nov 26, 2024
3313e2b
Merge branch 'main' into julien/nullable
julienrbrt Nov 26, 2024
6edb300
updates
julienrbrt Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/cosmos/staking/v1beta1/staking.pulsar.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion simapp/simd/cmd/testnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ func initTestnetFiles(
valStr,
valPubKeys[i],
sdk.NewCoin(args.bondTokenDenom, valTokens),
stakingtypes.NewDescription(nodeDirName, "", "", "", "", stakingtypes.Metadata{}),
stakingtypes.NewDescription(nodeDirName, "", "", "", "", &stakingtypes.Metadata{}),
stakingtypes.NewCommissionRates(math.LegacyOneDec(), math.LegacyOneDec(), math.LegacyOneDec()),
math.OneInt(),
)
Expand Down
2 changes: 1 addition & 1 deletion simapp/v2/simdv2/cmd/testnet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ func initTestnetFiles[T transaction.Tx](
valStr,
valPubKeys[i],
sdk.NewCoin(args.bondTokenDenom, valTokens),
stakingtypes.NewDescription(nodeDirName, "", "", "", "", stakingtypes.Metadata{}),
stakingtypes.NewDescription(nodeDirName, "", "", "", "", &stakingtypes.Metadata{}),
stakingtypes.NewCommissionRates(math.LegacyOneDec(), math.LegacyOneDec(), math.LegacyOneDec()),
math.OneInt(),
)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/gov/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
var (
valTokens = sdk.TokensFromConsensusPower(42, sdk.DefaultPowerReduction)
TestProposal = v1beta1.NewTextProposal("Test", "description")
TestDescription = stakingtypes.NewDescription("T", "E", "S", "T", "Z", stakingtypes.Metadata{})
TestDescription = stakingtypes.NewDescription("T", "E", "S", "T", "Z", &stakingtypes.Metadata{})
TestCommissionRates = stakingtypes.NewCommissionRates(math.LegacyZeroDec(), math.LegacyZeroDec(), math.LegacyZeroDec())
)

Expand Down
2 changes: 1 addition & 1 deletion tests/integration/slashing/slashing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestSlashingMsgs(t *testing.T) {

require.NoError(t, err)

description := stakingtypes.NewDescription("foo_moniker", "", "", "", "", stakingtypes.Metadata{})
description := stakingtypes.NewDescription("foo_moniker", "", "", "", "", &stakingtypes.Metadata{})
commission := stakingtypes.NewCommissionRates(math.LegacyZeroDec(), math.LegacyZeroDec(), math.LegacyZeroDec())

addrStrVal, err := valaddrCodec.BytesToString(addr1)
Expand Down
10 changes: 5 additions & 5 deletions tests/integration/staking/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,9 @@ func bondTypeGenerator() *rapid.Generator[stakingtypes.BondStatus] {
})
}

func metadataGenerator() *rapid.Generator[stakingtypes.Metadata] {
return rapid.Custom(func(t *rapid.T) stakingtypes.Metadata {
return stakingtypes.Metadata{
func metadataGenerator() *rapid.Generator[*stakingtypes.Metadata] {
return rapid.Custom(func(t *rapid.T) *stakingtypes.Metadata {
return &stakingtypes.Metadata{
ProfilePicUri: generateUri(t),
SocialHandleUris: []string{generateUri(t), generateUri(t)},
}
Expand Down Expand Up @@ -331,7 +331,7 @@ func getStaticValidator(t *testing.T, f *deterministicFixture) stakingtypes.Vali
"website",
"securityContact",
"details",
stakingtypes.Metadata{},
&stakingtypes.Metadata{},
),
UnbondingHeight: 10,
UnbondingTime: time.Date(2022, 10, 1, 0, 0, 0, 0, time.UTC),
Expand Down Expand Up @@ -367,7 +367,7 @@ func getStaticValidator2(t *testing.T, f *deterministicFixture) stakingtypes.Val
"website",
"securityContact",
"details",
stakingtypes.Metadata{},
&stakingtypes.Metadata{},
),
UnbondingHeight: 100132,
UnbondingTime: time.Date(2025, 10, 1, 0, 0, 0, 0, time.UTC),
Expand Down
10 changes: 5 additions & 5 deletions tests/integration/staking/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestInitGenesis(t *testing.T) {
Status: types.Bonded,
Tokens: valTokens,
DelegatorShares: math.LegacyNewDecFromInt(valTokens),
Description: types.NewDescription("hoop", "", "", "", "", types.Metadata{}),
Description: types.NewDescription("hoop", "", "", "", "", &types.Metadata{}),
}
assert.NilError(t, f.stakingKeeper.SetValidator(f.sdkCtx, bondedVal))

Expand All @@ -67,15 +67,15 @@ func TestInitGenesis(t *testing.T) {
Status: types.Bonded,
Tokens: valTokens,
DelegatorShares: math.LegacyNewDecFromInt(valTokens),
Description: types.NewDescription("hoop", "", "", "", "", types.Metadata{}),
Description: types.NewDescription("hoop", "", "", "", "", &types.Metadata{}),
}
bondedVal2 := types.Validator{
OperatorAddress: sdk.ValAddress(addrs[2]).String(),
ConsensusPubkey: pk2,
Status: types.Bonded,
Tokens: valTokens,
DelegatorShares: math.LegacyNewDecFromInt(valTokens),
Description: types.NewDescription("bloop", "", "", "", "", types.Metadata{}),
Description: types.NewDescription("bloop", "", "", "", "", &types.Metadata{}),
}

// append new bonded validators to the list
Expand Down Expand Up @@ -148,7 +148,7 @@ func TestInitGenesis_PoolsBalanceMismatch(t *testing.T) {
Jailed: false,
Tokens: math.NewInt(10),
DelegatorShares: math.LegacyNewDecFromInt(math.NewInt(10)),
Description: types.NewDescription("bloop", "", "", "", "", types.Metadata{}),
Description: types.NewDescription("bloop", "", "", "", "", &types.Metadata{}),
}

params := types.Params{
Expand Down Expand Up @@ -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{}),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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.go for:
    • 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:

  1. Validators with nil metadata
  2. 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

)
assert.NilError(t, err)
validators[i].Status = types.Bonded
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/staking/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestCancelUnbondingDelegation(t *testing.T) {
delegatorAddr := addrs[1]

// setup a new validator with bonded status
validator, err := types.NewValidator(valAddr.String(), PKs[0], types.NewDescription("Validator", "", "", "", "", types.Metadata{}))
validator, err := types.NewValidator(valAddr.String(), PKs[0], types.NewDescription("Validator", "", "", "", "", &types.Metadata{}))
validator.Status = types.Bonded
assert.NilError(t, err)
assert.NilError(t, f.stakingKeeper.SetValidator(ctx, validator))
Expand Down
2 changes: 1 addition & 1 deletion testutil/network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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 of types.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

stakingtypes.NewCommissionRates(commission, sdkmath.LegacyOneDec(), sdkmath.LegacyOneDec()),
sdkmath.OneInt(),
)
Expand Down
2 changes: 1 addition & 1 deletion x/genutil/gentx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var (
pk2 = priv2.PubKey()
addr1 = sdk.AccAddress(pk1.Address())
addr2 = sdk.AccAddress(pk2.Address())
desc = stakingtypes.NewDescription("testname", "", "", "", "", stakingtypes.Metadata{})
desc = stakingtypes.NewDescription("testname", "", "", "", "", &stakingtypes.Metadata{})
comm = stakingtypes.CommissionRates{}
)

Expand Down
4 changes: 2 additions & 2 deletions x/genutil/types/genesis_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestNetGenesisState(t *testing.T) {
}

func TestValidateGenesisMultipleMessages(t *testing.T) {
desc := stakingtypes.NewDescription("testname", "", "", "", "", stakingtypes.Metadata{})
desc := stakingtypes.NewDescription("testname", "", "", "", "", &stakingtypes.Metadata{})
comm := stakingtypes.CommissionRates{}
valAc := codectestutil.CodecOptions{}.GetValidatorCodec()

Expand All @@ -66,7 +66,7 @@ func TestValidateGenesisMultipleMessages(t *testing.T) {
}

func TestValidateGenesisBadMessage(t *testing.T) {
desc := stakingtypes.NewDescription("testname", "", "", "", "", stakingtypes.Metadata{})
desc := stakingtypes.NewDescription("testname", "", "", "", "", &stakingtypes.Metadata{})
pk1Addr, err := codectestutil.CodecOptions{}.GetValidatorCodec().BytesToString(pk1.Address())
require.NoError(t, err)
msg1 := stakingtypes.NewMsgEditValidator(pk1Addr, desc, nil, nil)
Expand Down
2 changes: 1 addition & 1 deletion x/staking/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

* [#19537](https://github.com/cosmos/cosmos-sdk/pull/19537) Changing `MinCommissionRate` in `MsgUpdateParams` now updates the minimum commission rate for all validators.
* [#20434](https://github.com/cosmos/cosmos-sdk/pull/20434) Add consensus address to validator query response
* [#21315](https://github.com/cosmos/cosmos-sdk/pull/21315) 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 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.

Expand Down
4 changes: 2 additions & 2 deletions x/staking/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func NewEditValidatorCmd() *cobra.Command {
profilePicUri, _ := cmd.Flags().GetString(FlagMetadataProfilePicUri)
socialHandlesUris, _ := cmd.Flags().GetStringArray(FlagMetadataSocialHandleUris)

metadata := types.Metadata{
metadata := &types.Metadata{
ProfilePicUri: profilePicUri,
SocialHandleUris: socialHandlesUris,
}
Expand Down Expand Up @@ -401,7 +401,7 @@ func BuildCreateValidatorMsg(clientCtx client.Context, config TxCreateValidatorC
config.Website,
config.SecurityContact,
config.Details,
config.Metadata,
&config.Metadata,
)

// get the initial validator commission parameters
Expand Down
16 changes: 10 additions & 6 deletions x/staking/client/cli/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type validator struct {
Website string
Security string
Details string
Metadata types.Metadata
Metadata *types.Metadata
CommissionRates types.CommissionRates
MinSelfDelegation math.Int
}
Expand Down Expand Up @@ -134,11 +134,15 @@ func buildCommissionRates(rateStr, maxRateStr, maxChangeRateStr string) (commiss
return commission, nil
}

func buildMetadata(profilePicUri string, socialHandlesUris []string) (metadata types.Metadata, err error) {
metadata.ProfilePicUri = profilePicUri
metadata.SocialHandleUris = socialHandlesUris
func buildMetadata(profilePicUri string, socialHandlesUris []string) (*types.Metadata, error) {
metadata := types.Metadata{
ProfilePicUri: profilePicUri,
SocialHandleUris: socialHandlesUris,
}

if err := metadata.Validate(); err != nil {
return metadata, err
return nil, err
}
return metadata, nil

return &metadata, nil
}
2 changes: 1 addition & 1 deletion x/staking/proto/cosmos/staking/v1beta1/staking.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ message Description {
// details define other optional details.
string details = 5;
// metadata defines extra information about the validator.
Metadata metadata = 6 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
Metadata metadata = 6 [(gogoproto.nullable) = true, (amino.dont_omitempty) = false];
}

// Metadata defines extra information about the validator.
Expand Down
4 changes: 2 additions & 2 deletions x/staking/simulation/msg_factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func MsgCreateValidatorFactory(k *keeper.Keeper) simsx.SimMsgFactoryFn[*types.Ms
r.StringN(10),
r.StringN(10),
r.StringN(10),
types.Metadata{
&types.Metadata{
ProfilePicUri: RandURIOfHostLength(r.Rand, 10),
SocialHandleUris: RandSocialHandleURIs(r.Rand, 2, 10),
},
Expand Down Expand Up @@ -143,7 +143,7 @@ func MsgEditValidatorFactory(k *keeper.Keeper) simsx.SimMsgFactoryFn[*types.MsgE
}
valOpAddrBz := must(k.ValidatorAddressCodec().StringToBytes(val.GetOperator()))
valOper := testData.GetAccountbyAccAddr(reporter, valOpAddrBz)
d := types.NewDescription(r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), r.StringN(10), types.Metadata{
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),
})
Expand Down
Loading
Loading