feat : close channel command #198

Harshit933 · 2024-03-19T16:26:54Z

Adds the ability to close channels from the lampo node.

Changes done :

- Adds close_channel function inside lampod/src/ln/channel_manager.rs
- Adds request/response inside model/close_channel.rs
- Adds `close` command inside main.rs

Some minor changes :

- Adds `channel_id` inside channels response.

Fixes #195

Adds the ability to close channels from the lampo node. Changes done : Adds close_channel function inside lampod/src/ln/channel_manager.rs Adds request/response inside model/close_channel.rs Adds `close` command inside main.rs Some minor changes : Adds `channel_id` inside channels response.

vincenzopalazzo

Good PR thanks for doing it.

I left some comments in here

lampo-common/src/model/close_channel.rs

lampod/src/actions/handler.rs

lampod/src/ln/channel_manager.rs

lampod/src/ln/events.rs

tests/tests/src/lampo_cln_tests.rs

vincenzopalazzo

need more works, sorry

lampo-common/src/model/close_channel.rs

lampod/src/actions/handler.rs

lampod/src/jsonrpc/channels.rs

lampod/src/ln/channel_manager.rs

lampod/src/ln/events.rs

tests/tests/src/lampo_cln_tests.rs

When there is only one channel open with the given `node_id` we can just skip the `channel_id` from the `close` request.

vincenzopalazzo

We are close, some rust style suggestion, and we need to make the test better

lampo-common/src/model/close_channel.rs

lampod/src/jsonrpc/channels.rs

vincenzopalazzo · 2024-03-21T12:00:00Z

tests/tests/src/lampo_cln_tests.rs

+    let result: Result<response::CloseChannel, _> = lampo.call(
+        "close",
+        request::CloseChannel {
+            node_id: info_cln.id.to_string(),
+            channel_id: Some(channels.channels.first().unwrap().channel_id.to_string()),
+        },
+    );
+    assert!(result.is_err(), "{:?}", result);


nit: this is a poor test, 99% of the code is copied by other test, and your code is not checking nothing import.

You are missing:

checking that CloseChannel contains the information that you need

add another test where the channel_id is not specified

The test where channel_id is not specified is already added.

// Closing the second channel - at this point there is only 1 channel with the peer let result: Result<response::CloseChannel, _> = lampo.call( "close", request::CloseChannel { node_id: info_cln.id.to_string(), channel_id: None, }, ); assert!(result.is_ok(), "{:?}", result); assert_eq!( result.unwrap().peer_id, info_cln.id.to_string() );

So I do not understand the test, why you are calling 3 time close channel?

For clarity every time that you close a channel you make a new test, and add the reason inside the name. E.g: test_lampo_to_cln_close_channel_without_channel_id_success and so one all the combination that you added

There are four tests in a same one. I'll describe each one of them -
Prior to the test we open two channels.

1st test - We call the close with a None channel_id, this adds the purpose that when there are two channels open with the same peer, we have to provide respective channel_id.

2nd test - Closing the first channel - This test demonstrate that we can close the channel with given channel_id

3rd test - Now we have only one channel left - We can close this channel without providing channel_id.

4th test - Now we do not have any channel left - Now closing the channel would give an error.

I avoided making 4 sets of different tests as it would add much more boilerplate code. Let me know if I should make different test cases for each.

Let me know if I should make different test cases for each.

Yes please :)

vincenzopalazzo

Adding a new set of comments

vincenzopalazzo · 2024-03-22T08:46:03Z

tests/tests/src/lampo_cln_tests.rs

+    let result: Result<response::CloseChannel, _> = lampo.call(
+        "close",
+        request::CloseChannel {
+            node_id: info_cln.id.to_string(),
+            channel_id: Some(channels.channels.first().unwrap().channel_id.to_string()),
+        },
+    );
+    assert!(result.is_err(), "{:?}", result);


So I do not understand the test, why you are calling 3 time close channel?

For clarity every time that you close a channel you make a new test, and add the reason inside the name. E.g: test_lampo_to_cln_close_channel_without_channel_id_success and so one all the combination that you added

vincenzopalazzo · 2024-03-22T15:12:32Z

CI is reporting a failure

test lampo_cln_tests::test_lampo_to_cln_close_channel_without_channel_id_success ... ok

failures:

---- lampo_cln_tests::test_close_channel_without_opening_a_channel_success stdout ----
thread 'lampo_cln_tests::test_close_channel_without_opening_a_channel_success' panicked at tests/tests/src/lampo_cln_tests.rs:870:5:
callback got a timeout
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: tests::lampo_cln_tests::test_close_channel_without_opening_a_channel_success
             at ./src/lampo_cln_tests.rs:870:5
   3: tests::lampo_cln_tests::test_close_channel_without_opening_a_channel_success::{{closure}}
             at ./src/lampo_cln_tests.rs:857:58
   4: core::ops::function::FnOnce::call_once
             at /build/rustc-1.75.0-src/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    lampo_cln_tests::test_close_channel_without_opening_a_channel_success

lampod/src/jsonrpc/channels.rs

This commits includes some refactoring of the `close` channel tests and adds a `channel_id` test.

vincenzopalazzo

LGTM

There are some comments that can be addressed in other PRs

vincenzopalazzo · 2024-03-22T21:05:46Z

tests/tests/src/lampo_cln_tests.rs

@@ -539,3 +540,373 @@ fn be_able_to_kesend_payments() {
    assert!(result.is_ok(), "{:?}", result);
    async_run!(cln.stop()).unwrap();
 }
+
+#[test]
+fn test_closing_two_channels_without_channelid_success() {


Suggested change

fn test_closing_two_channels_without_channelid_success() {

fn test_closing_two_channels_without_channelid_fails() {

It is returning an error right?

vincenzopalazzo · 2024-03-22T21:08:23Z

tests/tests/src/lampo_cln_tests.rs

+#[test]
+fn channel_id_tests() {
+    let node_id = "039c108cc6777e7d5066dfa33c611c32e6baa1c49de6d546b5b76686486d0360ac".to_string();
+
+    // This is a correct channel_hex of 32 bytes
+    let channel_hex =
+        Some("0a44677526ac8c607616bd91258d7e5df1d86fae9c32e23aa18703a650944c64".to_string());
+    let req = CloseChannel {
+        node_id: node_id.clone(),
+        channel_id: channel_hex,
+    };
+    let channel_bytes = [
+        10, 68, 103, 117, 38, 172, 140, 96, 118, 22, 189, 145, 37, 141, 126, 93, 241, 216, 111,
+        174, 156, 50, 226, 58, 161, 135, 3, 166, 80, 148, 76, 100,
+    ];
+    let channel_id_bytes = req.channel_id();
+    assert_eq!(channel_bytes, channel_id_bytes.unwrap().0);
+}


This can be added inside lampo-common/src/model/close_channel.rs

inside the tests there are functional tests, and this is a unit test

Moving `channel_id` test from `lampo_cln_test` to `close_channel.rs` and rename some test names. Follow up to vincenzopalazzo#198

Moving `channel_id` test from `lampo_cln_test` to `close_channel.rs` and rename some test names. Follow up to #198

Harshit933 changed the title ~~Feat/close~~ feat : close channel command Mar 19, 2024

vincenzopalazzo requested changes Mar 19, 2024

View reviewed changes

test : adds integration test for close_channel

c012ed7

Harshit933 force-pushed the feat/close branch from 130b713 to fdc3963 Compare March 20, 2024 15:52

vincenzopalazzo requested changes Mar 20, 2024

View reviewed changes

vincenzopalazzo referenced this pull request Mar 20, 2024

fix : make the channel_id inside close optional

fdc3963

When there is only one channel open with the given `node_id` we can just skip the `channel_id` from the `close` request.

Harshit933 force-pushed the feat/close branch from fdc3963 to 4d3c1ef Compare March 21, 2024 10:52

fix : make the channel_id inside close optional

05574d9

When there is only one channel open with the given `node_id` we can just skip the `channel_id` from the `close` request.

Harshit933 force-pushed the feat/close branch from 4d3c1ef to 05574d9 Compare March 21, 2024 11:00

vincenzopalazzo requested changes Mar 21, 2024

View reviewed changes

vincenzopalazzo requested changes Mar 22, 2024

View reviewed changes

Harshit933 force-pushed the feat/close branch from 428d45b to 6f77491 Compare March 22, 2024 17:21

vincenzopalazzo requested changes Mar 22, 2024

View reviewed changes

fix : refactor close command tests

ecdc494

This commits includes some refactoring of the `close` channel tests and adds a `channel_id` test.

Harshit933 force-pushed the feat/close branch from 6f77491 to ecdc494 Compare March 22, 2024 18:06

vincenzopalazzo requested changes Mar 22, 2024

View reviewed changes

vincenzopalazzo merged commit 9ba4c71 into vincenzopalazzo:main Mar 22, 2024
4 of 5 checks passed

Harshit933 added a commit to Harshit933/lampo.rs that referenced this pull request Mar 23, 2024

refactor : fix some test names and move some test

9df6a18

Moving `channel_id` test from `lampo_cln_test` to `close_channel.rs` and rename some test names. Follow up to vincenzopalazzo#198

Harshit933 added a commit to Harshit933/lampo.rs that referenced this pull request Mar 23, 2024

refactor : fix some test names and move some test

5d6d434

Moving `channel_id` test from `lampo_cln_test` to `close_channel.rs` and rename some test names. Follow up to vincenzopalazzo#198

Harshit933 mentioned this pull request Mar 23, 2024

refactor : fix some test names and move some test #200

Merged

vincenzopalazzo pushed a commit that referenced this pull request Mar 23, 2024

refactor : fix some test names and move some test

5bc56d7

Moving `channel_id` test from `lampo_cln_test` to `close_channel.rs` and rename some test names. Follow up to #198

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat : close channel command #198

feat : close channel command #198

Harshit933 commented Mar 19, 2024 •

edited

Loading

vincenzopalazzo left a comment

vincenzopalazzo left a comment

vincenzopalazzo left a comment

vincenzopalazzo Mar 21, 2024

Harshit933 Mar 22, 2024

vincenzopalazzo Mar 22, 2024

Harshit933 Mar 22, 2024

vincenzopalazzo Mar 22, 2024

vincenzopalazzo left a comment

vincenzopalazzo Mar 22, 2024

vincenzopalazzo commented Mar 22, 2024

vincenzopalazzo left a comment

vincenzopalazzo Mar 22, 2024

vincenzopalazzo Mar 22, 2024

	fn test_closing_two_channels_without_channelid_success() {
	fn test_closing_two_channels_without_channelid_fails() {

feat : close channel command #198

feat : close channel command #198

Conversation

Harshit933 commented Mar 19, 2024 • edited Loading

Changes done :

Some minor changes :

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincenzopalazzo commented Mar 22, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harshit933 commented Mar 19, 2024 •

edited

Loading