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

Add channel_keys_id as param in get_destination_script to support gen… #2744

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

rmalonson
Copy link
Contributor

…erating a different destination script for each channel.

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fa0d015) 88.54% compared to head (f53e911) 88.52%.

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

Files Patch % Lines
lightning/src/ln/channel.rs 66.66% 1 Missing ⚠️
lightning/src/util/test_utils.rs 50.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2744      +/-   ##
==========================================
- Coverage   88.54%   88.52%   -0.03%     
==========================================
  Files         113      113              
  Lines       89355    89330      -25     
  Branches    89355    89330      -25     
==========================================
- Hits        79119    79075      -44     
- Misses       7864     7883      +19     
  Partials     2372     2372              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wpaulino
Copy link
Contributor

LGTM, please amend the commit message to follow our guidelines https://cbea.ms/git-commit/.

@rmalonson
Copy link
Contributor Author

LGTM, please amend the commit message to follow our guidelines https://cbea.ms/git-commit/.

Done

wpaulino
wpaulino previously approved these changes Nov 22, 2023
@benthecarman
Copy link
Contributor

benthecarman commented Nov 23, 2023

Is there a way to reference a key id to a channel? Would be nice so we can see a transaction to a given address and know which channel it was for

@wpaulino
Copy link
Contributor

You should be able to map the user_channel_id to channel_keys_id via SignerProvider::generate_channel_keys_id. Does that work for you?

@benthecarman
Copy link
Contributor

Yeah that'd work

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Awesome work on the new code—it's looking really clean and clear!

I was curious if there were any chats or discussions leading up to this change. If so, could you point me in that direction?

Also, thinking about the impact of this change, I think it might be a good idea to update the comments for the get_destination_script, indicating why are we (now) passing the channel_keys_id as an argument.

@wpaulino
Copy link
Contributor

Needs a rebase

This enables implementers to generate a different destination script for each channel.
@rmalonson
Copy link
Contributor Author

Needs a rebase

Done

@rmalonson
Copy link
Contributor Author

Awesome work on the new code—it's looking really clean and clear!

I was curious if there were any chats or discussions leading up to this change. If so, could you point me in that direction?

Also, thinking about the impact of this change, I think it might be a good idea to update the comments for the get_destination_script, indicating why are we (now) passing the channel_keys_id as an argument.

Thanks! Yes, this was briefly discussed in the Lightspark discord channel here. And I've updated the description for get_destination_script to include the reasoning for channel_keys_id.

@wpaulino wpaulino merged commit 146a291 into lightningdevkit:main Nov 27, 2023
15 checks passed
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.

None yet

5 participants