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

lnrpc+lncli: add send support for custom data records #3900

Merged
merged 1 commit into from Jan 14, 2020

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Jan 10, 2020

With the introduction of custom record sending and receiving, it became possible to attach arbitrary data to a payment. One obvious use case is attaching a human-readable message to a payment. Especially in the case of spontaneous key send payments, this can give the receiver some context on the payment.

For example: tipping. Usually people sending a tip would want to include some information on who they are or what the tip is for. For Lightning this may be even more desired than for other payment methods, because payments are anonymous by default.

Attaching and retrieving the message was already possible before, but this PR makes it easier to do so by introducing an additional lncli flag --data to attach one or more custom records:

lncli sendpayment -d 0274e7fb33eafd74fe1acb6db7680bb4aa78e9c839a6e954e38abfad680f645ef7 -a 100 --key_send --data 323442=00,3234556=ffff080812

To specify a string value, the standard command line tool xxd can be used (the example record id here is the 3-byte ascii string 'tip' converted to an integer):

--data 7629168=$(echo -n "Thank you!" | xxd -pu -c 10000)

(The -c parameter is to prevent xxd from inserting line breaks)

Note: The available onion blob space of 1300 bytes is used for routing info and custom records. The bigger the size of the custom records, the fewer bytes remain for routing info and the shorter the maximum route length will be.

@joostjager joostjager requested review from Roasbeef and cfromknecht Jan 10, 2020
@joostjager joostjager changed the title lnrpc+lncli: add support for sender message lnrpc+lncli: add support for (key) sender message Jan 10, 2020
@joostjager joostjager force-pushed the joostjager:payment-msg branch from daa1d4f to 6e9aefc Jan 10, 2020
Copy link
Member

Roasbeef left a comment

I thought this was intended to allow users to set arbitrary kv tlv pairs on the command line rather than carve out a new record for messages.

Copy link
Collaborator

cfromknecht left a comment

What is probably the most important accomplishment of this PR however is that it makes an attempt to standardize an unofficial field for payment messages.

I'm not convinced this should happen within lnd until there is some actual standardization, especially a field that knowingly collides with whatsat.

@joostjager joostjager force-pushed the joostjager:payment-msg branch from 6e9aefc to 6964a69 Jan 13, 2020
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Jan 13, 2020

Message record id updated to a different random number.

My thought was that if we don't pick a field and provide a few hooks to make it easy to use (string flag on lncli and string type invoice htlc field), payment messages are less likely to be used. The payment message functionality can serve as a demonstrator of everything else that is possible using custom records. it seemed nice to me to get people thinking about that.

But as @Roasbeef suggests, we can also generalize this to just a new lncli flag. The question though is what a good format is. Any thoughts on that?

lncli sendpayment --data=34378273:"hello world!",124343:0feab32827cba23

It gets complicated with the spaces and special chars. Also not sure what different shells do there. In my bash, at least the data argument comes through as one block, but the quotes have disappeared.

@joostjager joostjager requested review from Roasbeef and cfromknecht Jan 13, 2020
@joostjager joostjager force-pushed the joostjager:payment-msg branch from 6964a69 to af02a77 Jan 14, 2020
@joostjager joostjager changed the title lnrpc+lncli: add support for (key) sender message lnrpc+lncli: add send support for custom data records Jan 14, 2020
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Jan 14, 2020

Removed hard-coded record and replaced by generic --data flag.

cmd/lncli/commands.go Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:payment-msg branch from af02a77 to d978b55 Jan 14, 2020
@joostjager joostjager requested a review from cfromknecht Jan 14, 2020
Copy link
Collaborator

cfromknecht left a comment

LGTM 🏈

@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 14, 2020
@joostjager joostjager modified the milestones: 0.10.0, 0.9.0 Jan 14, 2020
Copy link
Member

Roasbeef left a comment

LGTM 🎄

Very simple yet powerful PR!

@Roasbeef Roasbeef merged commit 3799f16 into lightningnetwork:master Jan 14, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 62.954%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.