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

LND: Simple channel closing implementation #37

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Bobface
Copy link

@Bobface Bobface commented Sep 30, 2020

Adds a CloseChannel method to LndClient which delegates to the existing LndSwaggerClient.CloseChannelAsync method.

Adds a `CloseChannel` method to `LndClient` which delegates to the existing
`LndSwaggerClient.CloseChannelAsync` method.
@knocte
Copy link

knocte commented Oct 1, 2020

@NicolasDorier can you review? thanks!

@NicolasDorier
Copy link
Member

can you ad a test

@knocte
Copy link

knocte commented Oct 2, 2020

@NicolasDorier AFAIU there are no tests for opening channels (grep OpenChannel -ri tests/ returns no results) yet even, can you add those first? Or are we missing something?

@NicolasDorier
Copy link
Member

NicolasDorier commented Oct 5, 2020

Do not test channel open, test channel close. Look the tests, I setup channel already, just close one of those after.

Those LN implementation always break their shit at every update. Without test, this PR will work for 2 months and we would not know it.

@knocte
Copy link

knocte commented Oct 5, 2020

Look the tests, I setup channel already,

Thanks for the info. Thing is, I looked at the tests and didn't find the channel setup anywhere, can you clarify exactly where's this please?

@knocte
Copy link

knocte commented Oct 5, 2020

https://github.com/btcpayserver/BTCPayServer.Lightning/blob/master/src/BTCPayServer.Lightning.All/ConnectChannels.cs

Hehe, that file is under the namespace BTCPayServer.Lightning.Tests, however it's inside the src folder instead of the tests folder (and uses "CreateChannel" instead of "OpenChannel"), that's why I was not seeing it :)

I'll have a look, thanks!

knocte added 3 commits October 5, 2020 17:41
In fact its namespace was BTCPayServer.Lightning.Tests
so it was simply misplaced.
Because this way it matches better with the LN API,
and is easier to find.
@knocte
Copy link

knocte commented Oct 5, 2020

@NicolasDorier why is CI not running for the PR?

@NicolasDorier
Copy link
Member

Somehow was disabled. Can you try to re-commit?

Not sure if we should add this as the Dispose() method
of the test class actually:
https://stackoverflow.com/a/33516224/544947
@knocte knocte force-pushed the lnd-close-channel branch from c348dfd to aea9e9d Compare October 6, 2020 16:40
@NicolasDorier
Copy link
Member

does not pass

@knocte
Copy link

knocte commented Oct 8, 2020

Yep I know, not sure why, I'll investigate later.

@NicolasDorier
Copy link
Member

@knocte ^

knocte pushed a commit to nblockchain/geewallet that referenced this pull request Oct 19, 2020
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 15, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 15, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 16, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 16, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 16, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 16, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte added a commit to nblockchain/geewallet that referenced this pull request Feb 16, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte pushed a commit to nblockchain/geewallet that referenced this pull request Feb 17, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
knocte added a commit to nblockchain/geewallet that referenced this pull request Feb 17, 2021
BTCPayServer.Lightning does currently not support closing channels with LND.
There is an open PR which implements this feature:
btcpayserver/BTCPayServer.Lightning#37

Until this PR is merged we use the compiled DLLs with this feature.
Afterwards this commit can be dropped and the Nuget package versions
can be updated to the latest version which has the PR merged.
@dennisreimann
Copy link
Member

This one needs rebasing :)

@knocte
Copy link

knocte commented Sep 26, 2022

@aarani do we need this?

@dennisreimann
Copy link
Member

I think it would make sense to coordinate it with #84 or integrate the updates there.

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.

4 participants