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 optional memo to TAP addresses #757

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Jan 5, 2024

This PR adds optional memos to TAP addresses. Memos are encoded in the address similiar to BOLT 11 Descriptions.

I've tested it locally with a 0.3 Tap client. The implementation seems to be both backward and forward compatible.

Things I'm unsure about:

  • Maximum length of description
  • Where to add itests that make sense

Creation of address:
image

Decoding on old version:
image

Decoding of TAP addr without memo(addr created on tap version 0.3)
image

Fixes #751

This commit adds memo fields to the addrs table. The memo fields are
used to store the memo for the address.
This commit adds the memo field to the NewAddrRequest and Addr messages.
This commit adds the memo field to the TAP address constructors.
This commit adds address encoding and decoding to the TAP address.
This commit adds the ability to add a memo to the address related
cli commands.
@guggero
Copy link
Member

guggero commented Jan 8, 2024

Thanks for the PR! I think having a memo is definitely useful on the receiving end to know what purpose an address was created for.
But I'm not sure whether it should be included in the address itself? Unfortunately the term is quite overloaded.
We use --memo for lncli addinvoice where the text does go into the invoice and can be seen by the sender.
But we also use --memo for lncli openchannel where it's purely a note/info for the node operator.

I would personally have expected the memo on a TAP address to follow the second pattern, where the memo is only for the creator of the address, not also the recipient.
If the intention is for this to be used like the memo in an invoice, we probably should make it more clear that it will become part of the address and anyone can decode it (so don't put privacy sensitive information in it). Also we would need to update the BIP and add the memo as a field there.

@sputn1ck
Copy link
Member Author

sputn1ck commented Jan 8, 2024

I would personally have expected the memo on a TAP address to follow the second pattern, where the memo is only for the creator of the address, not also the recipient.

That was how I initially started building it out. But after some thought addresses being already tied with amounts and assests make them feel a lot like lightning invoices already. Plus with address reuse being generally discouraged (Is this the case for TAP addresses as well?) to me giving the memo to the sender of funds also made sense.

I guess as the future of taproot-asset payments is probably tied to lightning channels, we will have invoices with memos and using addresses for payments seems unlikely. So happy to change it to the local memo only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

[feature]: memo or description like field for generating TAP address
3 participants