Improve lncli closechannel error messages for mempool fee and channel state (#9155) #10288
Conversation
… state - Enhance user-facing error when mempool fee is too low, suggesting bumpfee usage. - Add clear error when channel is already closing, guiding users to pendingchannels and bumpfee.
Summary of ChangesHello @Suvrat1629, 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 significantly enhances the user experience for Highlights
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error messages in lncli closechannel to be more actionable. The changes are good, but I have a couple of suggestions. In lnwallet/btcwallet/btcwallet.go, the new error message is too specific and assumes a cooperative close, which might not always be the case. I've suggested a more generic message. In rpcserver.go, there are some indentation issues and long error strings that could be improved for readability, following the repository's style guide.
| errors.Is(err, chain.ErrMinRelayFeeNotMet): | ||
|
|
||
| return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee, err.Error()) | ||
| return fmt.Errorf("%w: %v. The cooperative close transaction was created but not accepted into the mempool due to low fees. You may need to use 'lncli wallet bumpfee' to increase the fee and get the transaction confirmed", lnwallet.ErrMempoolFee, err.Error()) |
There was a problem hiding this comment.
The new error message is specific to cooperative close transactions, but mapRpcclientError is a generic function that can be called for other transaction types as well. This could lead to confusing error messages for users. Consider making the message more generic.
Additionally, the error string is very long and should be split across multiple lines to improve readability, as per the style guide.1
return fmt.Errorf("%w: %v. The transaction was not accepted into the "+
"mempool due to low fees. If this was a cooperative channel close, "+
"you may need to use 'lncli wallet bumpfee' to get the transaction "+
"confirmed", lnwallet.ErrMempoolFee, err.Error())Style Guide References
Footnotes
| rpcsLog.Debugf("Trying to non-force close "+ | ||
| "offline channel with chan_point=%v", | ||
| chanPoint) | ||
|
|
||
| return fmt.Errorf("unable to gracefully close "+ | ||
| "channel while peer is offline (try "+ | ||
| "force closing it instead): %v", err) | ||
| // Check if the channel is already closed or has a pending close tx. | ||
| if channel.HasChanStatus(channeldb.ChanStatusCoopBroadcasted) { | ||
| return fmt.Errorf("channel is already in the process of closing. Check 'lncli pendingchannels' for status. If the close transaction is not confirmed, you may need to use 'lncli wallet bumpfee'") | ||
| } | ||
|
|
||
| return fmt.Errorf("unable to gracefully close channel while peer is offline (try force closing it instead): %v", err) |
There was a problem hiding this comment.
This new block of code has some indentation issues. The rpcsLog.Debugf call is not indented correctly, and the following if statement is over-indented. Additionally, the error messages are quite long. According to the style guide, long strings should be split to improve readability.1
rpcsLog.Debugf("Trying to non-force close "+
"offline channel with chan_point=%v",
chanPoint)
// Check if the channel is already closed or has a pending close tx.
if channel.HasChanStatus(channeldb.ChanStatusCoopBroadcasted) {
return fmt.Errorf("channel is already in the process of closing. " +
"Check 'lncli pendingchannels' for status. If the close " +
"transaction is not confirmed, you may need to use " +
"'lncli wallet bumpfee'")
}
return fmt.Errorf("unable to gracefully close channel while peer "+
"is offline (try force closing it instead): %v", err)Style Guide References
Footnotes
1 participant
Change Description
This PR addresses issue #9155 by making error messages for lncli closechannel more actionable and less confusing:
All error strings follow Go style guidelines.
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.