Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 proposal for currencies on payRequest #251
base: luds
Are you sure you want to change the base?
Add proposal for currencies on payRequest #251
Changes from 7 commits
56bbdd8
55384ff
ab98b7d
5f0ea3a
7bf8f3a
2d37cec
721e642
1d669fc
8580e3c
b5cf042
6a5b6e0
f70a0d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It might be good to include a longer description about this field. With UMA, a bunch of folks integrating have been a bit confused by the mechanics around decimals/multiplier. Might be good to lay out some examples and show why decimals is important for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @lsunsi I added some examples and notes on small currency units in the uma spec that might be helpful here too - https://github.com/uma-universal-money-address/protocol/blob/main/umad-04-lnurlp-response.md#currency-examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense, I tried complementing the spec with this info and copied your example kind of . What do you think of (6a5b6e0)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, that captures the gist of it! I think the main difference is that UMA has an explicit max of 8 decimals, but keeping it open is fine too. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny enough, in UMA, I chose not to include the target amount, but did include the multiplier and some other stuff:
The fees are useful for a full visual cost breakdown on the sender side. The multiplier and decimals help infer the converted amount from the invoice amount. I do like having the wrapping struct to keep related fields grouped and allow for extension in the future without polluting the top-level structure. Just some food for thought :-).
UMA spec link: https://github.com/uma-universal-money-address/protocol/blob/main/umad-06-payreq-response.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. The reason I chose to keep just the int is to keep the protocol smaller.
The only property in this structure that is not already included is the fee, right?
The reason I didn't included it is that there's multiple ways of taking the fee, right? It could be billed in sats, or in currency (althought it's kind of the same). But the fee could be in the price (spread) or separate. Etc.
What do you think about these possibilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair. I just delimit the fee in mSats always because my assumption is that it's based into the cost of the invoice:
invoice amount = amount * multiplier + exchangeFeesMillisatoshi
. I like having a top-level structure to wrap payment details since I could imagine more fields being added there in the future for scenarios you're describing. With the wrapper, we wouldn't need to pollute the top level more.