Making error message clearer when peer_pubkey used and peer is offline#2139
Making error message clearer when peer_pubkey used and peer is offline#2139sergey3bv wants to merge 2 commits into
Conversation
Summary of ChangesHello, 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 improves the user experience by providing clearer feedback when an RFQ flow fails due to a peer being offline. Instead of a generic 'no asset channels found' error, the system now explicitly states that matching channels exist but are currently inactive. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. 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 enhances the RFQ manager's error reporting by providing specific feedback when matching asset channels exist but are inactive, rather than returning a generic 'not found' error. The changes include logic to fetch and filter all channels for a specific peer and a new test case to validate the error messages. Reviewers identified a potential breaking change in addScidAlias due to its reliance on exact error string matching and recommended style improvements, including adding explanatory comments for code intention and using idiomatic range loops.
| return nil, fmt.Errorf("no active asset "+ | ||
| "channel for %s and peer=%s: "+ | ||
| "matching channel(s) are inactive "+ | ||
| "(peer offline or not usable for "+ | ||
| "RFQ)", &specifier, peerPubKey) | ||
| } |
There was a problem hiding this comment.
The new error message introduced here is much clearer for the user. However, please be aware that addScidAlias (lines 633-635) currently uses fragile string matching on errors returned by FetchChannel. Since this new error message does not match the expected string "no asset channel balance found for", it will cause addScidAlias to skip its fallback logic and return an error instead. You should consider updating addScidAlias to handle this new error case or, ideally, refactor the error handling to avoid string matching.
| if peerPubKey != nil { | ||
| _, ok := balancesMap[*peerPubKey] | ||
| if !ok { | ||
| allChannels, err := m.cfg.ChannelLister.ListChannels( |
There was a problem hiding this comment.
According to the repository style guide (lines 7-8, 51), in-body comments should explain the intention behind a block of code. Adding a brief comment here explaining why we are fetching all channels (to provide a more descriptive error message) would improve maintainability.
// If we couldn't find an active channel for the peer, we'll
// check if there are any inactive channels that match the
// asset specifier. This allows us to provide a more
// descriptive error message to the user.
allChannels, err := m.cfg.ChannelLister.ListChannels(References
- Comments must explain the why behind a certain block of code. In-body comments should explain the intention of the code. (link)
| for i := range allChannels { | ||
| if allChannels[i].PubKeyBytes == *peerPubKey { | ||
| peerChannels = append( | ||
| peerChannels, allChannels[i], | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop can be simplified by using a value range, which is more idiomatic in Go when the index is not required. This improves readability as per the style guide (line 6).
for _, channel := range allChannels {
if channel.PubKeyBytes == *peerPubKey {
peerChannels = append(peerChannels, channel)
}
}References
- Readable code is the most important requirement for any commit created. (link)
sergey3bv
force-pushed
the
refactor/clean-err-message
branch
from
680f3ca to
2358cee
Compare
1 participant
Should close #1577