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

Unify interface types and expose connect / disconnect methods #52

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Mar 28, 2023

Based on #13.

So far the interface sometimes took Strings, &strs, or actual Rust
types. Moreover we sometimes took the arguments by reference and
sometimes not. In order to make the interface more uniform, we here take a step towards
the Rust types and taking arguments by reference.

Note that some of the affected parts are due to change in the future,
e.g., once we transition to using upstream's NetAddress for peer
addresses.

Also, by user request, we expose the capability to just connect a peer, which is useful for example when awaiting an incoming channel from an LSP.

@tnull tnull marked this pull request as draft March 28, 2023 15:47
@tnull
Copy link
Collaborator Author

tnull commented Mar 28, 2023

Draft for now as still WIP.

@tnull tnull force-pushed the 2023-03-expose-connect-method branch from 84780ba to 44dfca3 Compare March 30, 2023 11:44
@tnull tnull changed the title Expose connect_peer / disconnect_peer methods Unify interface types and expose connect_peer / disconnect_peer methods Mar 30, 2023
@tnull tnull marked this pull request as ready for review March 30, 2023 11:45
@tnull tnull force-pushed the 2023-03-expose-connect-method branch 2 times, most recently from 63b9f70 to 13f8c55 Compare March 30, 2023 12:18
@tnull tnull changed the title Unify interface types and expose connect_peer / disconnect_peer methods Unify interface types and expose connect / disconnect methods Mar 30, 2023
This was referenced Mar 30, 2023
@tnull tnull added this to the 0.1 milestone Apr 4, 2023
@tnull tnull force-pushed the 2023-03-expose-connect-method branch from 13f8c55 to e3a6a3a Compare April 7, 2023 08:18
@tnull tnull force-pushed the 2023-03-expose-connect-method branch from e3a6a3a to e9f2e53 Compare April 18, 2023 07:37
@tnull
Copy link
Collaborator Author

tnull commented Apr 18, 2023

Rebased on main after #13 has been merged.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-03-expose-connect-method branch from e9f2e53 to 3faadd7 Compare April 21, 2023 09:31
@tnull
Copy link
Collaborator Author

tnull commented Apr 21, 2023

Addressed feedback and rebased on main.

@tnull tnull force-pushed the 2023-03-expose-connect-method branch 2 times, most recently from 9d1fba8 to ef1f8ff Compare April 25, 2023 08:52
@tnull
Copy link
Collaborator Author

tnull commented Apr 25, 2023

Rebased on main once more.

@tnull tnull force-pushed the 2023-03-expose-connect-method branch from ef1f8ff to de7fc2a Compare April 25, 2023 09:05
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM modulo a couple minor comments. Feel free to squash.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
So far the interface sometimes took `String`s, `&str`s, or actual Rust
types. Moreover we sometimes took the arguments by reference and
sometimes not.

In order to make the interface more uniform, we here take a step towards
the Rust types and taking arguments by reference.

Note that some of the affected parts are due to change in the future,
e.g., once we transition to using upstream's `NetAddress` for peer
addresses.
@tnull tnull force-pushed the 2023-03-expose-connect-method branch from de7fc2a to f07ec07 Compare April 25, 2023 15:47
@tnull
Copy link
Collaborator Author

tnull commented Apr 25, 2023

Squashed commits and removed a few more clones:

diff --git a/src/lib.rs b/src/lib.rs
index 1145aa0..8ce9bd6 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -859,6 +859,6 @@ impl Node {
                let peer_info = PeerInfo { pubkey: node_id, address };

-               let con_peer_pubkey = peer_info.pubkey.clone();
-               let con_peer_addr = peer_info.address.clone();
+               let con_peer_pubkey = peer_info.pubkey;
+               let con_peer_addr = peer_info.address;
                let con_success = Arc::new(AtomicBool::new(false));
                let con_success_cloned = Arc::clone(&con_success);
@@ -879,10 +879,10 @@ impl Node {
                }

+               log_info!(self.logger, "Connected to peer {}@{}. ", peer_info.pubkey, peer_info.address,);
+
                if permanently {
-                       self.peer_store.add_peer(peer_info.clone())?;
+                       self.peer_store.add_peer(peer_info)?;
                }

-               log_info!(self.logger, "Connected to peer {}@{}. ", peer_info.pubkey, peer_info.address,);
-
                Ok(())
        }
@@ -939,6 +939,6 @@ impl Node {
                let peer_info = PeerInfo { pubkey: node_id, address };

-               let con_peer_pubkey = peer_info.pubkey.clone();
-               let con_peer_addr = peer_info.address.clone();
+               let con_peer_pubkey = peer_info.pubkey;
+               let con_peer_addr = peer_info.address;
                let con_success = Arc::new(AtomicBool::new(false));
                let con_success_cloned = Arc::clone(&con_success);
@@ -983,5 +983,4 @@ impl Node {
                ) {
                        Ok(_) => {
-                               self.peer_store.add_peer(peer_info.clone())?;
                                log_info!(
                                        self.logger,
@@ -989,4 +988,5 @@ impl Node {
                                        peer_info.pubkey
                                );
+                               self.peer_store.add_peer(peer_info)?;
                                Ok(())
                        }

@tnull tnull merged commit b51f9e8 into lightningdevkit:main Apr 25, 2023
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.

2 participants