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

Rename SocketAddress from NetAddress #2549

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

yanganto
Copy link
Contributor

@yanganto yanganto commented Sep 4, 2023

Fix #2358

Please change your imported crate from NetAddress to SocketAddress as the following.

- use lightning::ln::msgs::NetAddress;
+ use lightning::ln::msgs::SocketAddress;

Also, modify the type NetAddress::IPv4 to SocketAddress::TcpIpV4 and NetAddress::IPv6 to SocketAddress::TcpIpV6.

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2023

Codecov Report

Patch coverage: 93.50% and project coverage change: -0.05% ⚠️

Comparison is base (44b9c54) 90.63% compared to head (d34fcb8) 90.58%.
Report is 19 commits behind head on main.

❗ Current head d34fcb8 differs from pull request most recent head c2afb4a. Consider uploading reports for the commit c2afb4a to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2549      +/-   ##
==========================================
- Coverage   90.63%   90.58%   -0.05%     
==========================================
  Files         110      110              
  Lines       58199    57526     -673     
  Branches    58199    57526     -673     
==========================================
- Hits        52747    52111     -636     
+ Misses       5452     5415      -37     
Files Changed Coverage Δ
lightning-net-tokio/src/lib.rs 75.55% <66.66%> (ø)
lightning/src/routing/gossip.rs 89.94% <66.66%> (ø)
lightning/src/ln/msgs.rs 85.82% <86.36%> (-0.39%) ⬇️
lightning/src/ln/peer_handler.rs 61.45% <100.00%> (ø)

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@yanganto
Copy link
Contributor Author

yanganto commented Sep 4, 2023

It seems that the failing test cases did not relate to this PR. If something I need to change on this PR, please kindly let me know. 🙏

@optout21
Copy link
Contributor

optout21 commented Sep 4, 2023

Could you add to the PR description how users of LDK need to adapt their code? Just unroll the renames that has to be done (incl. in use statements). Otherwise LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
@yanganto yanganto force-pushed the socket-addr branch 2 times, most recently from b1449d5 to d34fcb8 Compare September 5, 2023 02:21
@TheBlueMatt
Copy link
Collaborator

This LGTM, but I kinda want to land #2134, which is getting quite close, first if that's okay.

@yanganto
Copy link
Contributor Author

yanganto commented Sep 6, 2023

This LGTM, but I kinda want to land #2134, which is getting quite close, first if that's okay.

Sure, and sorry about this. I did not check if there was a PR work on NetAddress before.

@TheBlueMatt
Copy link
Collaborator

All good, better than not opening it at all :)

Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

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

LGTM,
verified that all instanced were renamed.

@tnull
Copy link
Contributor

tnull commented Sep 7, 2023

This needs a rebase now.

@tnull
Copy link
Contributor

tnull commented Sep 8, 2023

Could also consider renaming variables/methods that reflected the NetAddress type before, e.g., their_net_address in peer_handler::Peer and fn test_net_address_from_str in msgs.rs.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this!

@@ -0,0 +1 @@
* The `NetAddress` has been moved to `SocketAddress`. The fieds `IPv4` and `IPv6` are also rename to `TcpIpV4` and `TcpIpV6` (#2358).
Copy link
Contributor

Choose a reason for hiding this comment

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

PR number is off

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can fix it in post :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2348 is the issue. I thought it was much clearer the reason we doing this, and also linked to the PR if the user wants to see the code diff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, we normally link the PRs so that they go directly to the code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I will make it right in future PR

@TheBlueMatt TheBlueMatt merged commit 81f4151 into lightningdevkit:main Sep 8, 2023
11 of 14 checks passed
@yanganto yanganto deleted the socket-addr branch September 9, 2023 02:32
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.

Modify NetAddress Enum naming
7 participants