Skip to content

Conversation

@hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Dec 11, 2025

fixes issues found with staticcheck

@hieblmi hieblmi self-assigned this Dec 11, 2025
@gemini-code-assist
Copy link

Summary of Changes

Hello @hieblmi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request systematically addresses and resolves various issues flagged by the staticcheck linter across multiple packages. The changes primarily focus on modernizing Go code by updating deprecated functions, ensuring consistent error handling patterns, aligning with Go's naming conventions, and refactoring function signatures for improved clarity and adherence to best practices. Additionally, an obsolete file has been removed to streamline the codebase.

Highlights

  • gRPC Client Modernization: Updated gRPC client creation calls from grpc.Dial to the newer grpc.NewClient function in assets/client.go and swap_server_client.go.
  • File I/O Updates: Replaced deprecated io/ioutil functions like ioutil.ReadFile and ioutil.TempDir with their modern equivalents from the os package, such as os.ReadFile and os.TempDir().
  • Error Handling Consistency: Corrected the argument order for errors.Is calls to conform to staticcheck recommendations, changing errors.Is(context.Canceled, err) to errors.Is(err, context.Canceled).
  • Go Naming Conventions: Renamed an error variable from testPublishError to errTestPublish to align with standard Go error naming conventions (ST1012).
  • Insecure gRPC Transport: Updated the method for specifying insecure gRPC transport from grpc.WithInsecure() to grpc.WithTransportCredentials(insecure.NewCredentials()).
  • Function Signature Refactoring: Adjusted the return order of the startNewLoopIn function in loopin_test.go to place the error as the last return value, following common Go practices.
  • File Removal: Removed the uncharge_state.go file, indicating it is no longer needed.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses several issues found by the staticcheck tool, including usage of deprecated functions and incorrect error handling patterns. The changes are mostly correct and improve code quality. However, I've found a critical issue where ioutil.TempDir is incorrectly replaced with os.TempDir. This could lead to the deletion of the system's temporary directory during tests, which is a significant bug. I've provided suggestions to fix this using os.MkdirTemp instead.

@hieblmi hieblmi force-pushed the minor-fixes branch 5 times, most recently from 184a6c6 to f1dd6fb Compare December 17, 2025 13:36
@hieblmi hieblmi requested review from bhandras and starius December 17, 2025 13:52
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Nice, thanks! 🎉

case insec:
opts = append(opts, grpc.WithTransportCredentials(
insecure.NewCredentials(),
),
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀
Few nits

// proxyAddr indicates that a SOCKS proxy found at the address should be used to
// establish the connection.
func getSwapServerConn(address, proxyAddress string, insecure bool,
func getSwapServerConn(address, proxyAddress string, insec bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/insec/skipCertCheck/

ctx.errChan <- err
}()
return cfg, err, inSwap
return cfg, inSwap, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an empty line before return

loopd/daemon.go Outdated

err := depositManager.Run(d.mainCtx, initChan)
if err != nil && !errors.Is(context.Canceled, err) {
if err != nil && !errors.Is(err, context.Canceled) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch! Can you add tests capturing errors.Is regressions in the file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added tests here: hieblmi#5


// Dial the gRPC server.
conn, err := grpc.Dial(config.Host, opts...)
conn, err := grpc.NewClient(config.Host, opts...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some grpc.Dial calls still live in looprpc/client.pb.gw.go
We can bump protobuf deps and run make rpc to fix that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated the protobuf dependency, but a single occurrence remains, due to backward-compat issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I managed to get rid of it here: hieblmi#4

@hieblmi hieblmi force-pushed the minor-fixes branch 2 times, most recently from 43c8aa7 to 1754b52 Compare December 19, 2025 08:26
- Update assets/client.go to use grpc.NewClient
- Update swap_server_client.go to use grpc.NewClient
- Replace grpc.WithInsecure() with insecure.NewCredentials()
- Change return signature of startNewLoopIn from (*swapConfig, error, *loopInSwap) to (*swapConfig, *loopInSwap, error)
- Fixes ST1008: error should be returned as the last argument
- Rename testPublishError to errTestPublish (ST1012)
- Add explicit type to all constants in const groups (SA9004)
- Fixes staticcheck style warnings
@hieblmi
Copy link
Collaborator Author

hieblmi commented Dec 19, 2025

I added commit 5ec5875 to remove the grpc.DialContext and tested this locally. But I would appreciate a confirmation @starius.

@hieblmi hieblmi requested a review from starius December 19, 2025 09:39
Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

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

LGTM!
I added regression tests for the errors.Is bug: hieblmi#5

@hieblmi hieblmi merged commit 1bdaff8 into lightninglabs:master Dec 20, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants