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

Feature: Keysend custom tlv #411

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Evanfeenstra
Copy link

Adds send_with_custom_tlv to SpontaneousPayment, so you can send a keysend with custom data inside.

Please LMK if there is more bindings stuff or testing that should be added here.

Thanks!

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this! Already looks pretty good to me, just a few comments.

I think it would make sense to also add custom TLV support of the receiver's end while we're at it, which would also allow adding end-to-end test coverage in unit tests.

src/payment/spontaneous.rs Outdated Show resolved Hide resolved
src/payment/spontaneous.rs Outdated Show resolved Hide resolved
src/payment/spontaneous.rs Outdated Show resolved Hide resolved
src/payment/spontaneous.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull 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 adding the receive part also. I don't think we need / should add the TlvEntries wrapper type, but apart from that it mostly looks good.

Btw, please try to clean up the the commit history so that it reflects a number of logical changes to the codebase. Fixups should be 'sorted in' beneath such feature commits so they can be squashed into the respective commits before merging.

src/error.rs Outdated Show resolved Hide resolved
bindings/ldk_node.udl Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
@Evanfeenstra
Copy link
Author

@tnull after those last updates, the changes are small enough that I just squashed to a single commit. This is much cleaner now :)

@Evanfeenstra Evanfeenstra force-pushed the keysend-custom-tlv branch 2 times, most recently from da78120 to 848e4de Compare December 2, 2024 20:06
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

@tnull after those last updates, the changes are small enough that I just squashed to a single commit. This is much cleaner now :)

Thanks, but note that it's usually preferable to break up a PR into multiple commits and to squash fixups in rounds or just before merging as it allows reviewers to follow what comments/requested changes were already addressed.

src/event.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
bindings/ldk_node.udl Outdated Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, please squash the fixup commit.

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.

3 participants