-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor: use errors.New to replace fmt.Errorf with no parameters #20608
Conversation
WalkthroughWalkthroughThis update focuses on standardizing error handling across various files within the project. The main change involves replacing instances of Changes
Sequence Diagram(s)Diagram generation skipped as the changes are straightforward standardizations in error handling across various files. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
Outside diff range and nitpick comments (2)
x/auth/client/cli/query.go (2)
Line range hint
203-203
: Improve error message clarity for missing 'denom' field.The error message for a missing 'denom' field should be clearer to help users understand the expected input structure.
- return errors.New("expected denom field") + return errors.New("expected 'denom' field is missing. Ensure the coin denomination is specified.")
Line range hint
210-210
: Enhance error handling for missing 'amount' field.The error message for a missing 'amount' field should provide more context to help users correct their input.
- return errors.New("expected amount field") + return errors.New("expected 'amount' field is missing. Please specify the amount in the input.")
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (22)
- client/rpc/tx.go (4 hunks)
- client/v2/autocli/flag/coin.go (2 hunks)
- client/v2/autocli/flag/pubkey.go (2 hunks)
- client/v2/autocli/query.go (3 hunks)
- client/v2/offchain/verify.go (1 hunks)
- runtime/v2/manager.go (1 hunks)
- store/v2/root/factory.go (2 hunks)
- telemetry/metrics.go (3 hunks)
- testutil/key.go (2 hunks)
- testutil/sims/app_helpers.go (2 hunks)
- tools/cosmovisor/args.go (1 hunks)
- x/auth/client/cli/query.go (6 hunks)
- x/authz/client/cli/tx.go (2 hunks)
- x/crisis/types/genesis.go (2 hunks)
- x/distribution/types/params.go (2 hunks)
- x/evidence/types/genesis.go (2 hunks)
- x/genutil/types/genesis.go (1 hunks)
- x/genutil/utils.go (2 hunks)
- x/protocolpool/keeper/keeper.go (7 hunks)
- x/staking/keeper/slash.go (2 hunks)
- x/staking/types/authz.go (3 hunks)
- x/upgrade/keeper/migrations.go (2 hunks)
Files not reviewed due to errors (2)
- x/crisis/types/genesis.go (no review received)
- client/v2/autocli/query.go (no review received)
Files skipped from review due to trivial changes (4)
- client/v2/offchain/verify.go
- testutil/key.go
- x/staking/keeper/slash.go
- x/upgrade/keeper/migrations.go
Additional context used
Path-based instructions (18)
x/crisis/types/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/types/params.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/autocli/flag/coin.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/autocli/flag/pubkey.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/evidence/types/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/factory.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/utils.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/rpc/tx.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/client/cli/query.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/types/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/autocli/query.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/staking/types/authz.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.telemetry/metrics.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/client/cli/tx.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/sims/app_helpers.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/manager.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (18)
client/v2/autocli/flag/coin.go (1)
41-41
: The change toerrors.New
enhances clarity and consistency in error handling.x/evidence/types/genesis.go (1)
48-48
: The change fromfmt.Errorf
toerrors.New
in theValidate
function enhances clarity and consistency in error handling.store/v2/root/factory.go (1)
96-96
: Usingerrors.New
for the unsupported feature inCreateRootStore
aligns with the PR's goal of standardizing error handling.x/genutil/utils.go (1)
56-56
: Replacingfmt.Errorf
witherrors.New
for the invalid mnemonic error inInitializeNodeValidatorFilesFromMnemonic
enhances error message clarity.client/rpc/tx.go (2)
191-191
: Usingsdkerrors.ErrLogic.Wrapf
for the timeout error inWaitTxCmd
provides a more specific and informative error message.
218-218
: The use oferrors.New
for the missing txhash error inparseHashFromInput
simplifies and clarifies the error creation.x/auth/client/cli/query.go (1)
139-139
: Handle multiple transactions for a single signature more gracefully.The error handling for multiple transactions matching a single signature should provide more context or a potential resolution path for the user.
- return errors.New("found no txs matching given signatures") + return errors.New("multiple transactions found for the given signature, which is unexpected. Please contact support if this issue persists.")Likely invalid or redundant comment.
telemetry/metrics.go (3)
13-13
: The import ofgithub.com/pkg/errors
is correctly added to replacefmt.Errorf
witherrors.New
for error handling.
196-196
: Correct replacement offmt.Errorf
witherrors.New
for the error message when Prometheus metrics are not enabled.
222-222
: Proper use oferrors.New
in place offmt.Errorf
for the error message when the non in-memory metrics sink does not support the generic format.x/authz/client/cli/tx.go (2)
140-140
: Correctly replacedfmt.Errorf
witherrors.New
for the error message when the spend limit is not positive.
205-205
: Proper use oferrors.New
in place offmt.Errorf
for the error message when the spend limit for staking-related authorizations is not positive.testutil/sims/app_helpers.go (2)
5-5
: The import of theerrors
package is correctly added to replacefmt.Errorf
witherrors.New
for error handling.
168-168
: Correct replacement offmt.Errorf
witherrors.New
for the error message when creating a validator set fails.x/protocolpool/keeper/keeper.go (2)
434-434
: Correctly replacedfmt.Errorf
witherrors.New
for various validation error messages in the budget proposal validation. This change enhances the clarity of the error messages.Also applies to: 447-447, 451-451, 455-455
474-474
: Proper use oferrors.New
in place offmt.Errorf
for various error messages in the continuous fund validation. This change is consistent with the PR's objective to standardize error handling.Also applies to: 477-477, 480-480, 486-486
tools/cosmovisor/args.go (1)
313-313
: Replacefmt.Errorf
witherrors.New
for consistency.- return 0, fmt.Errorf("could not parse '%s' into a duration: %w", input, err) + return 0, errors.New("could not parse input into a duration")Likely invalid or redundant comment.
runtime/v2/manager.go (1)
408-408
: Replacefmt.Errorf
witherrors.New
for consistency.- return nil, fmt.Errorf("validator InitGenesis update is already set by another module") + return nil, errors.New("validator InitGenesis update is already set by another module")Likely invalid or redundant comment.
x/protocolpool/keeper/keeper.go
Outdated
@@ -207,7 +207,7 @@ func (k Keeper) SetToDistribute(ctx context.Context, amount sdk.Coins, addr stri | |||
return false, nil | |||
}) | |||
if totalStreamFundsPercentage.GT(math.NewInt(100)) { | |||
return fmt.Errorf("total funds percentage cannot exceed 100") | |||
return errors.New("total funds percentage cannot exceed 100") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of errors.New
instead of fmt.Errorf
for the error message when the total funds percentage exceeds 100 is correctly implemented. However, this message is duplicated in two different methods (SetToDistribute
and iterateAndUpdateFundsDistribution
). Consider extracting this error generation to a common method or constant to avoid duplication and ensure consistency.
Also applies to: 277-277
x/distribution/types/params.go
Outdated
@@ -28,7 +29,7 @@ func validateCommunityTax(i interface{}) error { | |||
} | |||
|
|||
if v.IsNil() { | |||
return fmt.Errorf("community tax must be not nil") | |||
return errors.New("community tax must be not nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace remaining fmt.Errorf
with errors.New
for consistency.
- return fmt.Errorf("community tax must be positive: %s", v)
+ return errors.New("community tax must be positive: " + v.String())
- return fmt.Errorf("community tax too large: %s", v)
+ return errors.New("community tax too large: " + v.String())
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.
return errors.New("community tax must be not nil") | |
return errors.New("community tax must be not nil") | |
return errors.New("community tax must be positive: " + v.String()) | |
return errors.New("community tax too large: " + v.String()) |
client/v2/autocli/flag/pubkey.go
Outdated
@@ -47,7 +48,7 @@ func (a *pubkeyValue) Set(s string) error { | |||
|
|||
any, err := types.NewAnyWithValue(pk) | |||
if err != nil { | |||
return fmt.Errorf("error converting to any type") | |||
return errors.New("error converting to any type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace fmt.Errorf
with errors.New
for consistency.
- return fmt.Errorf("input isn't a pubkey: %w", err)
+ return errors.New("input isn't a pubkey: " + err.Error())
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.
return errors.New("error converting to any type") | |
return errors.New("error converting to any type") | |
return errors.New("input isn't a pubkey: " + err.Error()) |
x/auth/client/cli/query.go
Outdated
@@ -183,7 +184,7 @@ $ %s query tx --%s=%s <sig1_base64>,<sig2_base64...> | |||
// ParseSigArgs parses comma-separated signatures from the CLI arguments. | |||
func ParseSigArgs(args []string) ([]string, error) { | |||
if len(args) != 1 || args[0] == "" { | |||
return nil, fmt.Errorf("argument should be comma-separated signatures") | |||
return nil, errors.New("argument should be comma-separated signatures") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify expected structure for duration encoding.
The error message when the 'nanos' field is missing should guide the user or developer on what is expected.
- return errors.New("expected nanos field")
+ return errors.New("expected 'nanos' field is missing in the input. Ensure the duration is correctly formatted.")
Committable suggestion was skipped due to low confidence.
x/auth/client/cli/query.go
Outdated
} | ||
if len(txs.Txs) > 1 { | ||
// This case means there's a bug somewhere else in the code as this | ||
// should not happen. | ||
return errors.ErrLogic.Wrapf("found %d txs matching given signatures", len(txs.Txs)) | ||
return sdkerrors.ErrLogic.Wrapf("found %d txs matching given signatures", len(txs.Txs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specific error types for better error handling.
Instead of using a generic error, consider using a specific error type that can be caught and handled differently depending on the context.
- return sdkerrors.ErrLogic.Wrapf("found %d txs matching given signatures", len(txs.Txs))
+ return sdkerrors.ErrMultipleTransactionsFound.Wrapf("found %d txs matching given signatures", len(txs.Txs))
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.
return sdkerrors.ErrLogic.Wrapf("found %d txs matching given signatures", len(txs.Txs)) | |
return sdkerrors.ErrMultipleTransactionsFound.Wrapf("found %d txs matching given signatures", len(txs.Txs)) |
x/auth/client/cli/query.go
Outdated
} | ||
|
||
return clientCtx.PrintProto(txs.Txs[0]) | ||
|
||
case TypeAccSeq: | ||
if args[0] == "" { | ||
return fmt.Errorf("`acc_seq` type takes an argument '<addr>/<seq>'") | ||
return errors.New("`acc_seq` type takes an argument '<addr>/<seq>'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the error message for missing acc_seq
argument.
The error message for a missing acc_seq
argument should be more descriptive to help the user understand what is expected.
- return errors.New("`acc_seq` type takes an argument '<addr>/<seq>'")
+ return errors.New("`acc_seq` type requires a non-empty argument in the format '<addr>/<seq>'")
x/genutil/types/genesis.go
Outdated
@@ -209,7 +209,7 @@ func (cs *ConsensusGenesis) UnmarshalJSON(b []byte) error { | |||
|
|||
func (cs *ConsensusGenesis) ValidateAndComplete() error { | |||
if cs == nil { | |||
return fmt.Errorf("consensus genesis cannot be nil") | |||
return errors.New("consensus genesis cannot be nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure non-nullity of critical configuration components.
The error message when the consensus genesis is null should explicitly state the necessity of this configuration for operation.
- return errors.New("consensus genesis cannot be nil")
+ return errors.New("consensus genesis configuration is required and cannot be nil. Please provide a valid configuration.")
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.
return errors.New("consensus genesis cannot be nil") | |
return errors.New("consensus genesis configuration is required and cannot be nil. Please provide a valid configuration.") |
x/staking/types/authz.go
Outdated
@@ -64,12 +64,12 @@ func (a StakeAuthorization) MsgTypeURL() string { | |||
// is unspecified. | |||
func (a StakeAuthorization) ValidateBasic() error { | |||
if a.MaxTokens != nil && a.MaxTokens.IsNegative() { | |||
return errorsmod.Wrapf(fmt.Errorf("max tokens should be positive"), | |||
return errorsmod.Wrapf(errors.New("max tokens should be positive"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specific error types for domain-specific errors.
Instead of using a generic error, consider using a specific error type that can be caught and handled differently depending on the context.
- return errorsmod.Wrapf(errors.New("max tokens should be positive"),
- "negative coin amount: %v", a.MaxTokens)
+ return sdkerrors.ErrNegativeAmount.Wrapf("negative coin amount: %v", a.MaxTokens)
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.
return errorsmod.Wrapf(errors.New("max tokens should be positive"), | |
return sdkerrors.ErrNegativeAmount.Wrapf("negative coin amount: %v", a.MaxTokens) |
x/staking/types/authz.go
Outdated
"negative coin amount: %v", a.MaxTokens) | ||
} | ||
|
||
if a.AuthorizationType == AuthorizationType_AUTHORIZATION_TYPE_UNSPECIFIED { | ||
return fmt.Errorf("unknown authorization type") | ||
return errors.New("unknown authorization type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the error message for unspecified authorization types.
The error message for an unspecified authorization type should be more descriptive to help the user understand what is expected.
- return errors.New("unknown authorization type")
+ return errors.New("authorization type is unspecified or unknown. Please provide a valid authorization type.")
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.
return errors.New("unknown authorization type") | |
return errors.New("authorization type is unspecified or unknown. Please provide a valid authorization type.") |
x/staking/types/authz.go
Outdated
@@ -220,7 +220,7 @@ | |||
case AuthorizationType_AUTHORIZATION_TYPE_CANCEL_UNBONDING_DELEGATION: | |||
return sdk.MsgTypeURL(&MsgCancelUnbondingDelegation{}), nil | |||
default: | |||
return "", errorsmod.Wrapf(fmt.Errorf("unknown authorization type"), | |||
return "", errorsmod.Wrapf(errors.New("unknown authorization type"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide more detailed error information for unknown authorization types.
The error message when an authorization type is unknown should include suggestions or possible values to guide the user.
- return "", errorsmod.Wrapf(errors.New("unknown authorization type"),
- "cannot normalize authz type with %T", authzType)
+ return "", errorsmod.Wrapf(errors.New("unknown authorization type. Valid types include: delegate, undelegate, redelegate, cancel_unbonding_delegation"),
+ "received authorization type: %T", authzType)
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.
return "", errorsmod.Wrapf(errors.New("unknown authorization type"), | |
return "", errorsmod.Wrapf(errors.New("unknown authorization type. Valid types include: delegate, undelegate, redelegate, cancel_unbonding_delegation"), | |
"received authorization type: %T", authzType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- telemetry/metrics.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- telemetry/metrics.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- testutil/sims/app_helpers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- testutil/sims/app_helpers.go
There was a problem hiding this 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
Files selected for processing (1)
- x/genutil/utils.go (2 hunks)
Additional context used
Path-based instructions (1)
x/genutil/utils.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
Additional comments not posted (1)
x/genutil/utils.go (1)
58-58
: Change aligns with PR objectives to standardize error handling by usingerrors.New
for static error messages.
x/genutil/utils.go
Outdated
@@ -54,7 +55,7 @@ func InitializeNodeValidatorFiles(config *cfg.Config, keyType string) (nodeID st | |||
// If no valid mnemonic is given, a random one will be used instead. | |||
func InitializeNodeValidatorFilesFromMnemonic(config *cfg.Config, mnemonic, keyType string) (nodeID string, valPubKey cryptotypes.PubKey, err error) { | |||
if len(mnemonic) > 0 && !bip39.IsMnemonicValid(mnemonic) { | |||
return "", nil, fmt.Errorf("invalid mnemonic") | |||
return "", nil, errors.New("invalid mnemonic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using fmt.Errorf
consistently for errors that involve variable data or need to wrap another error, to maintain consistency in error handling throughout the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- client/rpc/tx.go (4 hunks)
- x/protocolpool/keeper/keeper.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/rpc/tx.go
- x/protocolpool/keeper/keeper.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- client/rpc/tx.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- client/rpc/tx.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/genutil/utils.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/genutil/utils.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- client/v2/autocli/flag/coin.go (2 hunks)
- client/v2/autocli/flag/pubkey.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/v2/autocli/flag/coin.go
- client/v2/autocli/flag/pubkey.go
this pr has been a battle, i apoligise |
Some checks have not been passed, causing auto merging to fail. Is there a way to avoid this? |
There was a problem hiding this 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
Files selected for processing (1)
- x/staking/keeper/slash.go (2 hunks)
Additional context used
Path-based instructions (1)
x/staking/keeper/slash.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
x/staking/keeper/slash.go (1)
193-193
: Replacefmt.Errorf
witherrors.New
for consistency.In line with the PR objective to replace
fmt.Errorf
witherrors.New
when no formatting parameters are used, this instance should be updated to maintain consistency across the codebase.- return math.NewInt(0), fmt.Errorf("invalid validator status") + return math.NewInt(0), errors.New("invalid validator status")Likely invalid or redundant comment.
x/staking/keeper/slash.go
Outdated
@@ -411,7 +411,7 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida | |||
case dstValidator.IsUnbonded() || dstValidator.IsUnbonding(): | |||
notBondedBurnedAmount = notBondedBurnedAmount.Add(tokensToBurn) | |||
default: | |||
return math.ZeroInt(), fmt.Errorf("unknown validator status") | |||
return math.ZeroInt(), errors.New("unknown validator status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace fmt.Errorf
with errors.New
for consistency.
Similar to previous comments, this line should also be updated to use errors.New
instead of fmt.Errorf
since no formatting parameters are used, aligning with the overall refactoring goal of this PR.
- return math.ZeroInt(), fmt.Errorf("unknown validator status")
+ return math.ZeroInt(), errors.New("unknown validator status")
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- store/v2/root/factory.go (2 hunks)
- x/protocolpool/keeper/keeper.go (7 hunks)
Additional context used
Path-based instructions (2)
store/v2/root/factory.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (13)
store/v2/root/factory.go (1)
96-96
: Refactor toerrors.New
approved.The change from
fmt.Errorf
toerrors.New
for the "iavl v2 not supported" error aligns with the PR's objective of standardizing error handling. This is a good use oferrors.New
since there are no formatting directives needed.x/protocolpool/keeper/keeper.go (12)
210-210
: Refactor toerrors.New
approved.This change correctly implements the PR's objective. The error message "total funds percentage cannot exceed 100" is now consistently generated using
errors.New
, which is suitable as there are no formatting parameters.
295-295
: Refactor toerrors.New
approved.The duplication of the error message "total funds percentage cannot exceed 100" has been addressed by using
errors.New
consistently. This maintains the consistency and clarity of error messages across different methods.
393-393
: Refactor toerrors.New
approved.The error message "distribution has not started yet" is now generated using
errors.New
, aligning with the PR's objective to standardize error handling. This change is appropriate as the message does not require any dynamic values.
411-411
: Refactor toerrors.New
approved.The error "budget period has not passed yet" is now properly handled using
errors.New
. This change improves the consistency and readability of the error handling in the code.
452-452
: Refactor toerrors.New
approved.The error "invalid budget proposal: budget per tranche cannot be zero" is now created using
errors.New
, which is correct for static error messages and aligns with the PR's aim.
465-465
: Refactor toerrors.New
approved.The error "invalid budget proposal: start time cannot be less than the current block time" has been correctly refactored to use
errors.New
, enhancing the consistency of error handling.
469-469
: Refactor toerrors.New
approved.The error "invalid budget proposal: tranches must be greater than zero" is now generated using
errors.New
. This is a good practice for static error messages.
473-473
: Refactor toerrors.New
approved.The error "invalid budget proposal: period length should be greater than zero" is now created using
errors.New
, which is suitable for static error messages and enhances the clarity and maintainability of the code.
492-492
: Refactor toerrors.New
approved.The error "percentage cannot be zero or empty" is appropriately handled using
errors.New
, aligning with the objective to standardize error creation.
495-495
: Refactor toerrors.New
approved.The change to use
errors.New
for the error "percentage cannot be negative" is consistent with the PR's goals and improves the error handling standardization.
498-498
: Refactor toerrors.New
approved.The error "percentage cannot be greater than or equal to one" is now generated using
errors.New
, which is a good practice for static error messages.
504-504
: Refactor toerrors.New
approved.The error "expiry time cannot be less than the current block time" has been correctly refactored to
errors.New
, which is appropriate for static error messages and aligns with the PR's aim.
There was a problem hiding this 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
Files selected for processing (1)
- runtime/v2/manager.go (1 hunks)
Additional context used
Path-based instructions (1)
runtime/v2/manager.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/manager.go
Outdated
@@ -405,7 +405,7 @@ func (m *MM[T]) RunMigrations(ctx context.Context, fromVM appmodulev2.VersionMap | |||
|
|||
// The module manager assumes only one module will update the validator set, and it can't be a new module. | |||
if len(moduleValUpdates) > 0 { | |||
return nil, fmt.Errorf("validator InitGenesis update is already set by another module") | |||
return nil, errors.New("validator InitGenesis update is already set by another module") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider enhancing the error message by wrapping the existing error.
The replacement of fmt.Errorf
with errors.New
is aligned with the PR's objective to simplify error creation. However, consider using fmt.Errorf
with error wrapping to provide more context in case of an error, which can be crucial for debugging.
- return nil, errors.New("validator InitGenesis update is already set by another module")
+ return nil, fmt.Errorf("validator InitGenesis update is already set by another module: %w", someErrorVar)
Please replace someErrorVar
with the actual error variable that you would like to wrap.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/protocolpool/keeper/keeper.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/protocolpool/keeper/keeper.go
@tac0turtle Is this PR out of date and does it need to be resubmitted based on the latest code? |
Summary by CodeRabbit
Bug Fixes
Documentation
errors.New
instead offmt.Errorf
in multiple files for more descriptive messages.