fix: peer connect error handling and open channel button validation#3769
fix: peer connect error handling and open channel button validation#3769myxmaster wants to merge 2 commits intoZeusLN:masterfrom
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 addresses critical bugs related to peer connection and channel opening workflows. It refines the logic for connecting to peers, preventing an erroneous channel opening attempt when only a public key is provided. Additionally, it significantly improves the user experience by ensuring that the 'Open Channel' and 'Connect Peer' buttons accurately reflect the validity of user inputs, preventing submission with incomplete or invalid data. Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request addresses two bugs: an error when connecting to a peer with only a public key, and incorrect validation logic for the 'Open Channel' button. The changes correctly fix these issues by adjusting the logic in connectPeer, adding proper state management for form inputs, and improving button validation. I've provided one comment highlighting a UX inconsistency related to host validation, emphasizing the importance of a smoother user experience as per our guidelines. Overall, the changes are solid and effectively resolve the reported problems.
8eb0818 to
231ec59
Compare
| false, | ||
| connectPeerOnly | ||
| ); | ||
| ).catch(() => {}); |
There was a problem hiding this comment.
should we not display an error here if an error is thrown?
There was a problem hiding this comment.
This catch is just a minimal fix to suppress the unhandled promise rejection. I think a clean solution belongs in a separate PR (see this issue as ToDo).
In short:
There is a deeper inconsistency, which needs to be fixed. connectPeer communicates errors through two channels simultaneously: MobX observables (for the UI) and reject() (for programmatic use), but openChannel only uses observables. No caller actually uses the rejection. So the clean solution would be to align both methods: always
resolve(true/false), never reject(), and let callers react to the boolean return value.
231ec59 to
800cdae
Compare
|
(rebased) |
800cdae to
402b267
Compare
|
(rebased) |
402b267 to
ff8a051
Compare
ff8a051 to
a5d3870
Compare
|
(rebased) |
|
|
||
| const loading = connectingToPeer || openingChannel; | ||
|
|
||
| const isNodeHostValid = |
There was a problem hiding this comment.
@myxmaster, ValidationUtils.validateNodeHost returns false for empty strings, but isNodeHostValid ends up treating them as valid in isInvalidPeer. This logic seems incorrect. Could you please take another look?
There was a problem hiding this comment.
validateNodeHost deliberately returns false for empty strings because an empty string is not a valid host address format. However, providing a host is optional in this context here (LND and CLN both support connecting to a peer by pubkey only).
For this reason, isNodeHostValid is computed as host === '' || ValidationUtils.validateNodeHost(host) -> empty is valid in this view (gossip lookup will be attempted).
Description
This fixes #3768.
Bug 1: Cryptic proto error when connecting peer by pubkey only
When connecting a peer without a host:port in "Connect Peer only" mode, an early-return code path in
connectPeer()incorrectly setchannelRequest, triggering a MobX reaction that calledopenChannel()instead ofconnectPeer(). This caused LND to receive an open-channel request with an emptylocal_funding_amount(uint64 in protobuf), producing the cryptic errorproto: invalid value for uint64 type: "".The early-return (intended for the "open channel, peer already known" path) now only applies when
connectPeerOnlyis false. The actualconnectPeerbackend call proceeds and LND handles gossip lookup, returning an error if the peer is not reachable.-> I noticed that LND does not return a nice error message, but only "EOF", but this is something that has to be fixed on LND side. Until then I don't think we need a temporary workaround on Zeus side (meaning: display a user friendly error msg), because it is an edge case anyway; usually people will provide host:port.
Also fixed a resulting unhandled promise rejection in
OpenChannel.tsxwhereconnectPeer()was called without.catch().Bug 2: "Open Channel" / "Connect Peer" button active although required fields are empty
Several validation state issues caused the Open Channel / Connect Peer button to remain active when required inputs were empty:
isNodePubkeyValidtofalseSwitching back to "Olympus by ZEUS" did not restoreisNodePubkeyValid/isNodeHostValidtotrueisNodePubkeyValidto true;isNodeHostValidwas refactored out of state entirely and is now derived from host in render, consistent with howisInvalidFeeRateis handledAmountInputinitializes by callingonAmountChangewithsatAmount = '0'(string), so!satAmountnever triggered -> fixed by checking!Number(satAmount)satAmountin parent state, becauseAmountInput.componentDidUpdatedoes not callonAmountChangein its else-branch -> fixed by explicitly resettingsatAmount: '0'in the toggle handlerThis pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: