Skip to content
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

Fix: Bug: Eclair - Error in GUI when open channel #804 #828

Merged
merged 4 commits into from
Feb 3, 2024

Conversation

kelvinator07
Copy link
Contributor

@kelvinator07 kelvinator07 commented Jan 16, 2024

This PR Closes #804

Some ChannelData properties for the Eclair node was updated for v0.9.0 when calling its API and this caused a bug when running that version.

This PR updates the ChannelData properties to be compatible with v0.9.0, v0.8.0 and v0.7.0.

@jamaljsr
Copy link
Owner

Thanks for tackling this issue. I have a bit of feedback after testing this locally.

  1. This is unrelated to the code and more about housekeeping. It would be great if you could write more in the PR description explaining the rationale behind the changes you've made. What was the root of the problem? Did something change in the daemons or dependencies that caused this issue. This would just be helpful for reviewers, as well as our future selves to understand why specific code was changed.
  2. It looks like this works with the latest version of Eclair, but adding older versions to the network is now broken with similar errors. I guess they changed the structure of the API responses with the latest version. It would be great if our code could support both the old and new structures. If it's too much of a pain, then the only other option would be to drop support for the older versions. I prefer to keep the last 2 major versions around for testing, so that would be a last resort.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (110bc10) 100.00% compared to head (3a03019) 100.00%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #828   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          139       139           
  Lines         4389      4401   +12     
  Branches       855       858    +3     
=========================================
+ Hits          4389      4401   +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kelvinator07
Copy link
Contributor Author

Thanks for tackling this issue. I have a bit of feedback after testing this locally.

  1. This is unrelated to the code and more about housekeeping. It would be great if you could write more in the PR description explaining the rationale behind the changes you've made. What was the root of the problem? Did something change in the daemons or dependencies that caused this issue. This would just be helpful for reviewers, as well as our future selves to understand why specific code was changed.
  2. It looks like this works with the latest version of Eclair, but adding older versions to the network is now broken with similar errors. I guess they changed the structure of the API responses with the latest version. It would be great if our code could support both the old and new structures. If it's too much of a pain, then the only other option would be to drop support for the older versions. I prefer to keep the last 2 major versions around for testing, so that would be a last resort.

Thanks for the feedback. I've added a description to the PR.
The API doc doesn't have the sample responses for older versions. Had to run each versions while logging its responses to get that data. I've updated the PR to be compatible with the last 3 versions. Do take a look and let me know what you think. Thanks

@jamaljsr
Copy link
Owner

Thanks for the updates. I will test it out this wknd.

Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 🚀

Thanks for the changes. This looks great! I tested all 3 versions and was able to open channels and make direct payments.

There seems to be an issue with routing payments through Eclair v8.0+ nodes, but that is out of scope for this PR.

@jamaljsr jamaljsr merged commit 48ba1b2 into jamaljsr:master Feb 3, 2024
5 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.

Bug: Eclair - Error in GUI when open channel
3 participants