search

lndk-org / lndk

MIT License
76 stars 19 forks source link

Check LND has required version and build tags on start LNDK. #111

Closed kannapoix closed 1 week ago

kannapoix commented 2 months ago

This PR resolves #30 Check version and build tags of connecting LND at the start of LNDK.

We could make the has_version function smaller, but I chose clarity over shotness. The has_version and has_tags functions have an optional requirement argument, so we can test these functions with constants dedicated to the unit testing.

I have found that LND does not strictly follow semantic versioning. LND always release with beta pre-release version. Release candidate versions are tagged with something like beta.rc1. If I understand correctly, 0.18.0-beta < 0.18.0-beta.rc right?

orbitalturtle commented 2 months ago

Thanks for the PR @kannapoix! This is really useful. I'll try to give it a look this week.

kannapoix commented 1 month ago

Thank you for your feedback. I will address the issue. I have been unwell recently and unable to work on it. I hope I can push fixes this weekend.

kannapoix commented 1 month ago

I think we're pretty close. The main thing is make itest fails because the skip_version_check field is added to Cfg, which needs to be updated in src/cli.rs and tests/integration_tests.rs.

Sorry, I overlooked it. I'll add skip version check option to CLI and fix tests. Also rebase to latest master branch.

orbitalturtle commented 1 month ago

@kannapoix Thank you, appreciate it!

kannapoix commented 2 weeks ago

I believe the skip version check option for the CLI is no longer necessary, as #104 has changed the CLI implementation so that it now simply connects to an already running LNDK server.

I can't run integration tests on my ARM Mac, because bitcoind crate brings binary for Intel Mac. So, please let me know if integration tests failed.

mrfelton commented 2 weeks ago

I believe the skip version check option for the CLI is no longer necessary, as https://github.com/lndk-org/lndk/pull/104 has changed the CLI implementation so that it now simply connects to an already running LNDK server.

~I believe it would still be needed to enable support for older patched LND versions, like the one we are running. Without it, it would be impossible to start LNDK.~

EDIT: If you're just talking about the cli, yes that probably makes sense.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (302fc05) to head (21dfc92).

Files Patch % Lines
src/main.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #111 +/- ## ====================================== Coverage 0.00% 0.00% ====================================== Files 1 1 Lines 96 97 +1 ====================================== - Misses 96 97 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kannapoix commented 1 week ago

Commits squashed into 'main changes', 'skip version check option', and 'bump lnd'. Also updated the description of the skip_version_check option. Thanks for the comment! 🙇