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 init-from-state cmd #264

Closed
wants to merge 1 commit into from
Closed

fix init-from-state cmd #264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 22 additions & 12 deletions cmd/bitsongd/cmd/init_from_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const (
FlagIncreaseCoinAmount = "increase-coin-amount"
)

// bitsongd init-from-state v021 export-v021.json v021 --old-moniker Cosmostation --old-account-addr bitsong1wf3q0a3uzechxvf27reuqts8nqm45sn29ykncv --increase-coin-amount 10000000000000000ubtsg --o
// bitsongd init-from-state v021 export-v021.json v021 --old-moniker Cosmostation --old-account-addr bitsong1wf3q0a3uzechxvf27reuqts8nqm45sn29ykncv --increase-coin-amount 10000000000000000 -o
// InitFromStateCmd returns a command that initializes all files needed for Tendermint
// and the respective application.
func InitFromStateCmd(defaultNodeHome string) *cobra.Command {
Expand Down Expand Up @@ -114,7 +114,7 @@ func InitFromStateCmd(defaultNodeHome string) *cobra.Command {
increaseCoinAmount, _ := cmd.Flags().GetInt64(FlagIncreaseCoinAmount)

// attempt to lookup address from Keybase if no address was provided
kb, err := keyring.New(sdk.KeyringServiceName(), "test", clientCtx.HomeDir, bufio.NewReader(cmd.InOrStdin()), clientCtx.Codec)
kb, err := keyring.New(sdk.KeyringServiceName(), "os", clientCtx.HomeDir, bufio.NewReader(cmd.InOrStdin()), clientCtx.Codec)
if err != nil {
return fmt.Errorf("failed to open keyring: %w", err)
}
Expand Down Expand Up @@ -346,18 +346,27 @@ func ConvertStateExport(clientCtx client.Context, params StateExportParams) (*tm
// Impersonate validator
var oldValidator tmtypes.GenesisValidator

// Update tendermint validator data
// Filter the validators to keep only the one matching params.OldMoniker
var updatedValidators []tmtypes.GenesisValidator
for i := range genDoc.Validators {
if genDoc.Validators[i].Name == params.OldMoniker {
oldValidator = genDoc.Validators[i]
// Update the matching validator's data
validator := &genDoc.Validators[i]

// Replace validator data
validator.PubKey = params.TmPubKey
validator.Address = params.TmPubKey.Address()
validator.Power = validator.Power + (params.IncreaseCoinAmount / 1000000)
validator.Power = validator.Power + (params.IncreaseCoinAmount)

// Add the updated validator to the new slice
updatedValidators = append(updatedValidators, *validator)
break // Exit loop after finding and updating the matching validator
}
}

// Replace the original validators slice with the updated slice
genDoc.Validators = updatedValidators
Comment on lines +349 to +368
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve validator update logic to handle edge cases.

The current implementation has several potential issues:

  1. It assumes only one validator matches the moniker, breaking after the first match.
  2. Using a single-element slice in updatedValidators could be problematic if multiple validators share the same moniker.
  3. The power calculation no longer divides by 1,000,000, which might affect compatibility.

Consider this safer implementation:

-	var updatedValidators []tmtypes.GenesisValidator
+	updatedValidators := make([]tmtypes.GenesisValidator, 0)
 	for i := range genDoc.Validators {
 		if genDoc.Validators[i].Name == params.OldMoniker {
 			oldValidator = genDoc.Validators[i]
-			// Update the matching validator's data
 			validator := &genDoc.Validators[i]
 
 			validator.PubKey = params.TmPubKey
 			validator.Address = params.TmPubKey.Address()
-			validator.Power = validator.Power + (params.IncreaseCoinAmount)
+			validator.Power = validator.Power + (params.IncreaseCoinAmount / 1_000_000)
 
-			// Add the updated validator to the new slice
 			updatedValidators = append(updatedValidators, *validator)
-			break // Exit loop after finding and updating the matching validator
+			if len(updatedValidators) > 1 {
+				return nil, fmt.Errorf("multiple validators found with moniker %s", params.OldMoniker)
+			}
 		}
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Filter the validators to keep only the one matching params.OldMoniker
var updatedValidators []tmtypes.GenesisValidator
for i := range genDoc.Validators {
if genDoc.Validators[i].Name == params.OldMoniker {
oldValidator = genDoc.Validators[i]
// Update the matching validator's data
validator := &genDoc.Validators[i]
// Replace validator data
validator.PubKey = params.TmPubKey
validator.Address = params.TmPubKey.Address()
validator.Power = validator.Power + (params.IncreaseCoinAmount / 1000000)
validator.Power = validator.Power + (params.IncreaseCoinAmount)
// Add the updated validator to the new slice
updatedValidators = append(updatedValidators, *validator)
break // Exit loop after finding and updating the matching validator
}
}
// Replace the original validators slice with the updated slice
genDoc.Validators = updatedValidators
// Filter the validators to keep only the one matching params.OldMoniker
updatedValidators := make([]tmtypes.GenesisValidator, 0)
for i := range genDoc.Validators {
if genDoc.Validators[i].Name == params.OldMoniker {
oldValidator = genDoc.Validators[i]
validator := &genDoc.Validators[i]
validator.PubKey = params.TmPubKey
validator.Address = params.TmPubKey.Address()
validator.Power = validator.Power + (params.IncreaseCoinAmount / 1_000_000)
updatedValidators = append(updatedValidators, *validator)
if len(updatedValidators) > 1 {
return nil, fmt.Errorf("multiple validators found with moniker %s", params.OldMoniker)
}
}
}
// Replace the original validators slice with the updated slice
genDoc.Validators = updatedValidators


if oldValidator.Name == "" {
return nil, fmt.Errorf("validator to replace %s not found", params.OldMoniker)
}
Expand Down Expand Up @@ -389,14 +398,15 @@ func ConvertStateExport(clientCtx client.Context, params StateExportParams) (*tm
}
}

// Update total power
for i := range stakingGenState.LastValidatorPowers {
validatorPower := &stakingGenState.LastValidatorPowers[i]
if validatorPower.Address == operatorAddr {
validatorPower.Power = validatorPower.Power + (params.IncreaseCoinAmount / 1000000)
}
// Update LastValidatorPowers
stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{
{
Address: operatorAddr,
Power: updatedValidators[0].Power,
},
}
stakingGenState.LastTotalPower = stakingGenState.LastTotalPower.Add(sdk.NewInt(params.IncreaseCoinAmount / 1000000))

Comment on lines +401 to +408
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential panic in LastValidatorPowers update.

The code assumes updatedValidators has exactly one element and directly accesses index 0, which could panic if the slice is empty.

Apply this safer implementation:

-	stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{
-		{
-			Address: operatorAddr,
-			Power:   updatedValidators[0].Power,
-		},
-	}
+	if len(updatedValidators) == 0 {
+		return nil, fmt.Errorf("no validator found with moniker %s", params.OldMoniker)
+	}
+	stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{{
+		Address: operatorAddr,
+		Power:   updatedValidators[0].Power,
+	}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Update LastValidatorPowers
stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{
{
Address: operatorAddr,
Power: updatedValidators[0].Power,
},
}
stakingGenState.LastTotalPower = stakingGenState.LastTotalPower.Add(sdk.NewInt(params.IncreaseCoinAmount / 1000000))
if len(updatedValidators) == 0 {
return nil, fmt.Errorf("no validator found with moniker %s", params.OldMoniker)
}
stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{{
Address: operatorAddr,
Power: updatedValidators[0].Power,
}}

stakingGenState.LastTotalPower = sdk.NewInt(updatedValidators[0].Power)

// Update self delegation on operator address
for i := range stakingGenState.Delegations {
Expand Down
Loading