Skip to content

refactor: use a more straightforward return value #23718

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

Merged
merged 1 commit into from
Mar 3, 2025
Merged

refactor: use a more straightforward return value #23718

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion x/auth/migrations/legacytx/stdsign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (ss StdSignature) MarshalYAML() (interface{}, error) {
return nil, err
}

return string(bz), err
return string(bz), nil
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reconsider error handling approach.

Suppressing the YAML marshaling error by returning nil could hide potential issues from callers. Consider propagating the original error to allow proper error handling upstream.

Apply this diff to restore proper error handling:

-	return string(bz), nil
+	return string(bz), err
📝 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
return string(bz), nil
return string(bz), err

Copy link
Contributor

Choose a reason for hiding this comment

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

check again, rabbit!

Copy link
Contributor

Choose a reason for hiding this comment

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

@technicallyty You're right! I apologize for my previous comment. The current implementation is correct because:

  1. The error from yaml.Marshal is already properly handled in the if err != nil check
  2. If execution reaches the final return statement, the marshaling was successful
  3. Therefore, returning nil as the error is appropriate here

I'm retracting my previous suggestion. Thank you for the correction!

(｀･ω･´)ゞ

✏️ Learnings added 
Learnt from: technicallyty
PR: cosmos/cosmos-sdk#23718
File: x/auth/migrations/legacytx/stdsign.go:132-132
Timestamp: 2025-02-19T17:26:40.793Z
Learning: In the `MarshalYAML` method of `StdSignature`, returning `nil` as error in the final return statement is correct when the marshaling operation has already been validated by an earlier error check.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}

func (ss StdSignature) UnpackInterfaces(unpacker gogoprotoany.AnyUnpacker) error {
Expand Down
2 changes: 1 addition & 1 deletion x/staking/keeper/delegation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (k Keeper) GetDelegatorUnbonding(ctx context.Context, delegator sdk.AccAddr
if err != nil {
return unbonding, err
}
return unbonding, err
return unbonding, nil
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restore error propagation.

Suppressing the error from the Walk operation by returning nil could hide potential issues from callers. Consider propagating the original error to allow proper error handling upstream.

Apply this diff to restore proper error handling:

-	return unbonding, nil
+	return unbonding, err
📝 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
return unbonding, nil
return unbonding, err

Copy link
Contributor

Choose a reason for hiding this comment

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

thats wrong rabbit. the error is handled above!

Copy link
Contributor

Choose a reason for hiding this comment

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

@technicallyty You're absolutely right, I apologize for the confusion. The error is properly handled in the check above, and returning nil as the error is correct in this case since we've already verified no error occurred during the Walk operation.

✏️ Learnings added 
Learnt from: technicallyty
PR: cosmos/cosmos-sdk#23718
File: x/staking/keeper/delegation.go:212-212
Timestamp: 2025-02-19T01:32:16.298Z
Learning: In Go functions, when an error is explicitly checked and handled in an if statement that returns early, it's correct to return nil as the error value after the check since we've confirmed no error occurred in the operation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

}

// GetDelegatorBonded returns the total amount a delegator has bonded.
Expand Down
Loading