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

Clarify that QuoteDetails.amount does not include fees #198

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

diehuxx
Copy link

@diehuxx diehuxx commented Dec 6, 2023

No description provided.

Copy link
Contributor

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

Are the fees included in the amount always? if so this is a good clarification to me as I didn't realise.

@jiyoontbd
Copy link
Contributor

hmm, i don't think we ever fully decided (or just not documented?) whether the amountSubunits always included fees. My assumption til now was that they were separate, which is why we had a separate field named feeSubunits.

cc: @phoebe-lew @mistermoe if they are aware of any docs / decisions made i'm not remembering

@KendallWeihe
Copy link
Contributor

This is like how in the US, the sales tax is not included in the listed price, so then when you go to checkout you pay the amount+some, but then in Europe they mandate that the listed price also includes sales tax and when Europeans come to the US for the first time they're like, "hey what the heck, that's not what you listed as the price"

@diehuxx
Copy link
Author

diehuxx commented Dec 6, 2023

Are the fees included in the amount always?

I'm also not sure if there was ever a decision made -- It's just my opinion that amount should include fees.

My thinking is that

  1. it's better to choose whether includes or excludes rather than being ambiguous
  2. it's more convenient to just grab the price from the JSON than to get both price + fees and do girlmath to get the total

If I'm jumping the gun and we should kick this decision down the road further, I can remove that change from this PR.

@diehuxx
Copy link
Author

diehuxx commented Dec 7, 2023

Notes from our sync today:

  • Payin amount and payout amounts are customer-centric fields. Payin amount should always be the amount that Alice is going to pay. Payout amount should always be the amount that Alice is going to receive. This means that payin amount includes fees, and payout amount excludes fees.
  • Are subunits strings actually the best way to represent currency amounts? Ideally, subunits allow us to use integer amounts. But if an exchange involves millisatoshi amounts, for example, we will need to use fractional amounts of subunits, which defeats the purpose. Major unit amount strings may be easier.
  • If we use major unit amount strings, what is the decimal separator symbol? May currencies, including USD and BTC, use a period ., e.g. $2.50. In some locales, it is common to use a comma as a decimal separator, e.g. 2,50. We're split on whether . should always be the decimal separator or we should use , for currencies like EUR.

@andresuribe87
Copy link
Contributor

Are subunits strings actually the best way to represent currency amounts? Ideally, subunits allow us to use integer amounts. But if an exchange involves millisatoshi amounts, for example, we will need to use fractional amounts of subunits, which defeats the purpose. Major unit amount strings may be easier.

I strongly suggest that we refrain from modelling any numbers as integers nor floats. When dealing with finances, it is extremely important to not loose any precision, nor have overflow problems.

Showing localized currency number to users should be done, IMHO, at the presentation layer. That's higher than the protocol layer.

@mistermoe
Copy link
Member

mistermoe commented Dec 8, 2023

Example from our earlier sync for what @diehuxx posted

image

the above can be read as:

  • Alice will pay in 10 USD. 3 of which is considered fees
  • the PFI will pay out 0.00000275 BTC (aka alice will receive 0.00000275 BTC). 0.00000002 was taken for fees.

@mistermoe
Copy link
Member

If we use major unit amount strings, what is the decimal separator symbol? May currencies, including USD and BTC, use a period ., e.g. $2.50. In some locales, it is common to use a comma as a decimal separator, e.g. 2,50. We're split on whether . should always be the decimal separator or we should use , for currencies like EUR.

was previously leaning towards using what makes the most sense for the currency, however now leaning towards snapping to ..

examples that have standardized around using .

  1. Programming Languages: Most programming languages, such as Python, Java, C++, and JavaScript, use a period as the decimal separator. This is the standard practice for representing deciminal numbers in code.

  2. JSON (JavaScript Object Notation): numbers are represented using a period as the decimal separator. This follows the conventions of JavaScript.

  3. XML (eXtensible Markup Language): also typically uses a period as the decimal separator in its syntax.

  4. CSV (Comma-Separated Values): typically rely on using a . given that the commas need to be escaped

  5. IEEE 754 (Floating Point Arithmetic) This is a standard for floating-point arithmetic used in computers and programming languages. It defines formats and methods for representing and manipulating floating-point numbers in binary.

  6. SQL Databases: SQL databases, such as MySQL, PostgreSQL, and SQL Server, typically use a period as the decimal separator for storing and querying floating-point numbers.

  7. International System of Units (SI): basis for international scientific measurements, uses a period as the decimal separator in English-speaking countries.

@diehuxx
Copy link
Author

diehuxx commented Dec 8, 2023

Just added one more commit folding PaymentInstruction into QuoteDetails rather than having two sets of payin/payout fields each for QuoteDetails and PaymentInstructions

@diehuxx diehuxx changed the title Clarify fields on QuoteDetails type #201 Clarify fields on QuoteDetails type Dec 8, 2023
@diehuxx diehuxx changed the title #201 Clarify fields on QuoteDetails type Clarify fields on QuoteDetails type Dec 8, 2023
@diehuxx diehuxx force-pushed the quote-details branch 2 times, most recently from 19d22e8 to a2bcfe3 Compare December 8, 2023 21:04
Comment on lines 415 to 416
| `amountSubunits` | string | Y | The amount of currency paid out from or paid in to Alice in the smallest respective unit |
| `feeSubunits` | string | N | The amount paid in fees expressed in the smallest respective unit. This field is strictly informative, i.e. it does not affect the total payin/payout amount stated in the `amountSubunits` field. |
Copy link
Contributor

Choose a reason for hiding this comment

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

it does not affect the total payin/payout amount stated in the amountSubunits field.

Reads as slightly confusing because it does affect the amountSubunits but does not affect the "total". I think it's better to include the nuance here in the amountSubunits row, like you had previously, and then also include This field is strictly informative in the `feeSubunits. So, like this...

Suggested change
| `amountSubunits` | string | Y | The amount of currency paid out from or paid in to Alice in the smallest respective unit |
| `feeSubunits` | string | N | The amount paid in fees expressed in the smallest respective unit. This field is strictly informative, i.e. it does not affect the total payin/payout amount stated in the `amountSubunits` field. |
| `amountSubunits` | string | Y | The amount of payment, `feeSubunits` included, expressed in the smallest respective unit. |
| `feeSubunits` | string | N | The amount of fee payment expressed in the smallest respective unit; this field is strictly informative as `amountSubunits` includes `feeSubunits`. |

(Small side comment, we should try to be consistent in including or excluding periods "." at the end of each description)

Copy link
Author

@diehuxx diehuxx Dec 18, 2023

Choose a reason for hiding this comment

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

There is a proposed distinction that came up at office hours where amountSubunits does not always include feeSubunits. amountSubunits includes feeSubunits on payin. On payout, amountSubunits does NOT include feeSubunits. In both cases, amountSubunits represents exactly what Alice will receive/pay. Alice can be unaware of feeSubunits.

My current opinion is that amountSubunits should never include feeSubunits, but I'll stick with the above if there is consensus.

@mistermoe @jiyoontbd @corcillo @phoebe-lew Am I representing the decision from office hours accurately? Is it still your preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

amountSubunits includes feeSubunits on payin. On payout, amountSubunits does NOT include feeSubunits. In both cases, amountSubunits represents exactly what Alice will receive/pay. Alice can be unaware of feeSubunits.

yes, i think this is accurate with respect to what was discussed during office hour.

My current opinion is that amountSubunits should never include feeSubunits, but I'll stick with the above if there is consensus.

hmm, maybe we are focusing too much at the protocol layer on what is presented visually to alice to make sure she understands exactly how much the PFI is asking, and how much she should receive.

it may be easier to remember / reason about in PFI implementation if we aligned on never or always including feeSubunits in amountSubunits for both payin and payout.

Copy link
Contributor

Choose a reason for hiding this comment

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

it may be easier to remember / reason about in PFI implementation if we aligned on never or always including feeSubunits in amountSubunits for both payin and payout.

I second this, as not being consistent expresses a preference at the protocol level (b/c the inverse could also be true, which would prioritize comprehension from the PFI's-side)

Copy link
Author

Choose a reason for hiding this comment

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

if we aligned on never or always

IMO the amount should never include the fee. This is how I've always seen it structured in fintech in the past.

@diehuxx diehuxx changed the title Clarify fields on QuoteDetails type Clarify that QuoteDetails.amount does not include fees Jan 24, 2024
@diehuxx diehuxx merged commit 5f9ddb7 into main Jan 29, 2024
@diehuxx diehuxx deleted the quote-details branch January 29, 2024 20:06
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.

7 participants