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.

Sign up for GitHub

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

Jump to bottom

More expressive invoice descriptions #98

Merged
merged 5 commits into from
Jul 13, 2022
Merged

More expressive invoice descriptions #98

merged 5 commits into from
Jul 13, 2022

Conversation

nepet
Copy link
Contributor

@nepet nepet commented Jul 11, 2022

This pull request adds more descriptive invoice messages that resolve #58

@nepet nepet added this to the v0.3.0 milestone Jul 11, 2022
wtogami
wtogami reviewed Jul 13, 2022
View reviewed changes
@wtogami
Copy link
Contributor

wtogami commented Jul 13, 2022 •
edited
Loading

Tested ... this is a vast improvement over erroneous liquid swap within all CLN invoices prior.

Unfortunately this is bothering me ...

My swap-out is a swap-in from the partner's POV.
My swap-in is a swap-out from the partner's POV.

Invoice labels swap-out or swap-in are generated from the POV ... of the requester? (confused about this)

Invoice label: swap-out lbtc fee fbd2c20f5461a567909d19a0dd742db0ad91efbcbddc4ac0b2b88f07517dbfd8

It bothers me that these invoice labels show swap-out or swap-in to both sides when that isn't what it is from the POV of one side.

      "type": "swap-out",
      "role": "receiver",

Within listswaps and the statistics for each peer in listpeers it seems we get this POV right by differentiating the type and the role?

Since the invoice text is identical to both sides, how about this instead?

Invoice label: peerswap lbtc fee 699350x125x0 fbd2c20f5461a567909d19a0dd742db0ad91efbcbddc4ac0b2b88f07517dbfd8
Invoice label: peerswap lbtc claim 699350x125x0 fbd2c20f5461a567909d19a0dd742db0ad91efbcbddc4ac0b2b88f07517dbfd8

It was peerwap ... the short-id is often more useful at a glance ... and they can lookup the swap id if they want to.

@wtogami
Copy link
Contributor

wtogami commented Jul 13, 2022 •
edited
Loading

Make these changes before merge

  • Both swap-in and swap-out within label become peerswap
  • Add short-id before the swap id within the label.
  • Change the comment to maybe ... Construct memo ?

After this is merged please rebase #83 and #97

nepet added 4 commits July 13, 2022 13:43
@nepet
actions: Add swap info to invoice description.
350e0d6 
We want to have a more expressive description that contains the
chain that is swapped on, the usecase of the invoice as well as
the swap id.

Description format:
peerswap <chain> <invoice_type> <scid> <swap_id>
@nepet
testframework: Add methods to get the last invoice description
03b5b64
@nepet
tests: Add test to check on the claim invoice description
718189e
@nepet
go.mod: Tidy
6e22105
@nepet
Copy link
Contributor Author

nepet commented Jul 13, 2022

Requested changes are done. Self Ack 6e22105

@nepet nepet merged commit 2d008bc into ElementsProject:master Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wtogami wtogami wtogami left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

[CLN] improve invoice memo
2 participants
@nepet @wtogami