refactor: use errors.New to replace fmt.Errorf with no parameters #20608
Conversation
github-actions
bot
added
C:x/staking
C:CLI
C:x/distribution
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.Newenhances clarity and consistency in error handling.x/evidence/types/genesis.go (1)
48-48: The change fromfmt.Errorftoerrors.Newin theValidatefunction enhances clarity and consistency in error handling.store/v2/root/factory.go (1)
96-96: Usingerrors.Newfor the unsupported feature inCreateRootStorealigns with the PR's goal of standardizing error handling.x/genutil/utils.go (1)
56-56: Replacingfmt.Errorfwitherrors.Newfor the invalid mnemonic error inInitializeNodeValidatorFilesFromMnemonicenhances error message clarity.client/rpc/tx.go (2)
191-191: Usingsdkerrors.ErrLogic.Wrapffor the timeout error inWaitTxCmdprovides a more specific and informative error message.
218-218: The use oferrors.Newfor the missing txhash error inparseHashFromInputsimplifies 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/errorsis correctly added to replacefmt.Errorfwitherrors.Newfor error handling.
196-196: Correct replacement offmt.Errorfwitherrors.Newfor the error message when Prometheus metrics are not enabled.
222-222: Proper use oferrors.Newin place offmt.Errorffor 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.Errorfwitherrors.Newfor the error message when the spend limit is not positive.
205-205: Proper use oferrors.Newin place offmt.Errorffor 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 theerrorspackage is correctly added to replacefmt.Errorfwitherrors.Newfor error handling.
168-168: Correct replacement offmt.Errorfwitherrors.Newfor the error message when creating a validator set fails.x/protocolpool/keeper/keeper.go (2)
434-434: Correctly replacedfmt.Errorfwitherrors.Newfor 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.Newin place offmt.Errorffor 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.Errorfwitherrors.Newfor 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.Errorfwitherrors.Newfor 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.Newfor 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.
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.
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.
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.
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.Errorfwitherrors.Newfor consistency.In line with the PR objective to replace
fmt.Errorfwitherrors.Newwhen 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.
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.
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.Newapproved.The change from
fmt.Errorftoerrors.Newfor the "iavl v2 not supported" error aligns with the PR's objective of standardizing error handling. This is a good use oferrors.Newsince there are no formatting directives needed.x/protocolpool/keeper/keeper.go (12)
210-210: Refactor toerrors.Newapproved.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.Newapproved.The duplication of the error message "total funds percentage cannot exceed 100" has been addressed by using
errors.Newconsistently. This maintains the consistency and clarity of error messages across different methods.
393-393: Refactor toerrors.Newapproved.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.Newapproved.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.Newapproved.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.Newapproved.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.Newapproved.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.Newapproved.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.Newapproved.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.Newapproved.The change to use
errors.Newfor the error "percentage cannot be negative" is consistent with the PR's goals and improves the error handling standardization.
498-498: Refactor toerrors.Newapproved.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.Newapproved.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.
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.
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.
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.Newinstead offmt.Errorfin multiple files for more descriptive messages.