-
Notifications
You must be signed in to change notification settings - Fork 122
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
chore(DefaultServiceOptions): minor chainID loading refactoring #2022
Conversation
WalkthroughThe changes introduce a new function, Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2022 +/- ##
==========================================
- Coverage 22.38% 22.37% -0.02%
==========================================
Files 358 358
Lines 16043 16051 +8
Branches 12 12
==========================================
Hits 3591 3591
- Misses 12303 12311 +8
Partials 149 149
|
fp = filepath.Join(homeDir, "config", "genesis.json") | ||
) | ||
|
||
f, err := os.Open(filepath.Clean(fp)) |
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.
Found here the suggestion to fix it up
if err != nil { | ||
return "", | ||
errors.Join( | ||
f.Close(), | ||
fmt.Errorf( | ||
"failed to parse chain-id from genesis file: %w", | ||
err, | ||
), | ||
) | ||
} | ||
return chainID, f.Close() |
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.
here we avoid defer f.Close() (which ignores the error causing the gosec warning) and we explicitly return the (unlikely) f.Close error to the caller.
chainID is either specified by flag or read by genesis file. In handling the genesis file, we currently have a couple of gosec warnings around fliepath sanitization and closing the file, which I believe are easily solvable. This PR does that.
Ignoring errors in closing a file may be dangerous. This is not the case for genesis ( we only read the file, not write it). Still I wanted to introduce the error handling to solicit conversation around how to generally handle it in the overall codebase
Summary by CodeRabbit
DefaultServiceOptions
function by removing inline logic and improving readability.