feat(backend): return minSendAmount in quote responses by cozminu · Pull Request #3411 · interledger/rafiki

cozminu · 2025-04-28T12:55:32Z

Changes proposed in this pull request

solves issue Return minSendAmount in quote responses #3353
changes the response to be compliant with the new response format

Context

fixes #3353
fixes #3354

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

netlify · 2025-04-28T12:55:50Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`e710876`
🔍 Latest deploy log	https://app.netlify.com/projects/brilliant-pasca-3e80ec/deploys/6847fe1cdb07d50008e735dc

github-actions · 2025-04-28T13:00:01Z

🚀 Performance Test Results

Test Configuration:

VUs: 4
Duration: 1m0s

Test Metrics:

Requests/s: 44.22
Iterations/s: 14.77
Failed Requests: 0.00% (0 of 2662)

📜 Logs


> performance@1.0.0 run-tests:testenv /home/runner/work/rafiki/rafiki/test/performance
> ./scripts/run-tests.sh -e test "-k" "-q" "--vus" "4" "--duration" "1m"

Cloud Nine GraphQL API is up: http://localhost:3101/graphql
Cloud Nine Wallet Address is up: http://localhost:3100/
Happy Life Bank Address is up: http://localhost:4100/
cloud-nine-wallet-test-backend already set
cloud-nine-wallet-test-auth already set
happy-life-bank-test-backend already set
happy-life-bank-test-auth already set
     data_received..................: 928 kB 15 kB/s
     data_sent......................: 1.9 MB 31 kB/s
     http_req_blocked...............: avg=6.78µs   min=2.07µs   med=4.9µs    max=875.45µs p(90)=6.08µs   p(95)=6.53µs  
     http_req_connecting............: avg=447ns    min=0s       med=0s       max=564.54µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=89.87ms  min=7.78ms   med=74.57ms  max=612.35ms p(90)=155.47ms p(95)=178.48ms
       { expected_response:true }...: avg=89.87ms  min=7.78ms   med=74.57ms  max=612.35ms p(90)=155.47ms p(95)=178.48ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 2662
     http_req_receiving.............: avg=75.22µs  min=28.22µs  med=65.69µs  max=3.42ms   p(90)=95.36µs  p(95)=108.58µs
     http_req_sending...............: avg=32.42µs  min=10.59µs  med=25.08µs  max=2.19ms   p(90)=37.12µs  p(95)=50.15µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=89.76ms  min=7.64ms   med=74.49ms  max=612.26ms p(90)=155.38ms p(95)=178.41ms
     http_reqs......................: 2662   44.21839/s
     iteration_duration.............: avg=270.63ms min=153.83ms med=259.89ms max=1.16s    p(90)=326.95ms p(95)=359.67ms
     iterations.....................: 889    14.767148/s
     vus............................: 4      min=4       max=4 
     vus_max........................: 4      min=4       max=4

mkurapov

Main idea looks good, just a few questions/suggestions

mkurapov · 2025-05-06T18:55:55Z

+        code: PaymentMethodHandlerErrorCode.QuoteNonPositiveReceiveAmount,
+        details: {
+          minSendAmount: BigInt(
+            Math.ceil(1 / ilpQuote.highEstimatedExchangeRate.valueOf())


Given that highEstimatedExchangeRate is a Ratio, we could use highEstimatedExchangeRate.b and highEstimatedExchangeRate.a to create a new Pay.PositiveRatio without having to do 1 / x

I used ilpQuote.highEstimatedExchangeRate.b.valueOf() and it works...
I will look further into Ratios

mkurapov · 2025-05-06T18:57:42Z

-      {
+    let quote: PaymentQuote
+    try {
+      quote = await deps.paymentMethodHandlerService.getQuote(paymentMethod, {


Do we need to make changes for the local payment method getQuote?

made changes and wrote test

mkurapov · 2025-05-06T19:04:02Z

-      ctx.throw(err.status, err.message)
+
+      ctx.status = err.status
+      ctx.body = {


@oana-lolea

This is the change for #3354, so @cozminu can close the issue as part of this PR.

added fixes #3354 to description

mkurapov · 2025-05-06T19:05:42Z

+        }
+
+        stopTimer()
+        return new QuoteError(QuoteErrorCode.NonPositiveReceiveAmount, details)


Usually, throwing would be more relevant to Error , but given how we're always returning a value instead of throwing for these service methods, I think it's OK.

Curious to see if anyone else has a strong preference on this.

We also used to throw enums here in quote service until I swapped enums for QuoteError, so yes, I think we should settle this..

I think I like what Cozmin has.

Looking at the 2 factors:

enum vs. error object

throw vs. return

We cant use an enum because it's not rich enough and we have clear, established pattern of returning service errors (which I think is kinda nice). And I think if we threw it here (instead of returning basically everywhere else) it would suggest the nature of this error is somehow different than other service errors. It's not, we just need more details. So I think I like returning this QuoteError.

And frankly I think it's just a better pattern than the enums because errors so that we can include more specific details.

mkurapov · 2025-05-06T19:06:59Z

+
+          details.minSendAmount.value =
+            quoteMinSendAmount +
+            (sendingFee?.calculate(fixedFee + quoteMinSendAmount) ?? 0n)


Since calculate() already adds the fixedFee in the method

Suggested change

(sendingFee?.calculate(fixedFee + quoteMinSendAmount) ?? 0n)

(sendingFee?.calculate(quoteMinSendAmount) ?? 0n)

I just reworked this because it was wrong. I also added tests for future sanity
Given the following:
qMinSendAmount = 100 (the minimum from the ilpQuote, not the final one)
fixedFee = 50
percFee = 0.2 (20% or 2000 basisPoints as we know it)

minSendAmount = qMinSendAmount + fees
fees = minSendAmount * percFee + fixedFee
=>
minSendAmount = qMinSendAmount + minSendAmount * percFee + fixedFee
minSendAmount = 100 + minSendAmount * 0.2 + 50
0.8 * minSendAmount = 150
minSendAmount = 150 / 0.8
minSendAmount = 188

Conclusion: It's more complicated than I originally thought

Co-authored-by: Max Kurapov <max@interledger.org>

BlairCurrey · 2025-05-16T16:37:00Z


  return {
-    amount: BigInt(Math.round(Number(opts.sourceAmount) * scaledExchangeRate)),
+    amount: BigInt(Math.floor(Number(opts.sourceAmount) * scaledExchangeRate)),


My intuition is that floor would make more sense than round. Especially when the opposite direction (convertDestination) does Math.ceil. I'm trying to figure out the original reasoning. @cozminu can you elaborate on your thought process here?

@mkurapov I vaguely recall talking about the round behavior with you when making some related changes here #2857. I see this comment you left when originally adding it:

BigInt divided by BigInt ends up truncating and not rounding the result. I think for currency, rounding up or down to the nearest unit makes more sense (and is standard). We can't really get around using number, since we are dealing with decimals for the exchange rate

#1527 (comment)

If we do floor then we're going to need to update the integration test that started failing here https://github.com/interledger/rafiki/actions/runs/14999236719/job/42141498858

Given a 19:1 conversion ratio for MXN to USD with the same scale of 2:
Wallet A in MXN and Wallet B in USD

If Math.round is used and Wallet A sends 29 MXN, then Wallet B will receive 2 USD and this will result in a 14.5:1 conversion ratio for that trade. This means the ASE implementing Rafiki will support the missing 9MXN from the real conversion.
Sending 29 MXN to 37 MXN will result in a financial loss for the ASE.
Sending 38 MXN will result in a fair trade.
Sending 39 MXN to 47 MXN will result in a financial gain for the ASE.
If there’s no sending fee applied and a user sends minimum required to exploit Math.round (29 MXN to USD) from a wallet to another 1000s of times, it can practically gain money due to favorable conversion rate.

If Math.floor is used, Wallet A needs to send >= 38 and <= 56 MXN to convert in 2 USD, removing the chance of a transaction to be a loss for the ASE.

BlairCurrey · 2025-05-16T16:37:07Z

        exchangeRate | sourceAmount | assetScale | expectedResult                                    | description
        ${1.5}       | ${100n}      | ${9}       | ${{ amount: 150n, scaledExchangeRate: 1.5 }}      | ${'exchange rate above 1'}
-        ${1.1602}    | ${12345n}    | ${2}       | ${{ amount: 14323n, scaledExchangeRate: 1.1602 }} | ${'exchange rate above 1 with rounding up'}
+        ${1.1602}    | ${12345n}    | ${2}       | ${{ amount: 14322n, scaledExchangeRate: 1.1602 }} | ${'exchange rate above 1 with rounding up'}


small thing, but we should probably change the descriptions here if we go with Math.floor. the "with rounding up" and "with rounding down" seem to clarify how the Math.round was behaving. can probably jsut omit that part.

mkurapov · 2025-06-02T13:37:02Z

        extensions: {
-          code: errorToCode[quoteOrError]
+          code: errorToCode[quoteOrError.type]
        }


We should also add the quoteOrError.details information in the extensions object. This way we can have the Admin API be able to read minSendAmount as well

mkurapov · 2025-06-02T17:06:13Z

+  }
+  const fixedFee = sendingFee.fixedFee ?? 0n
+  // if the fee is 0%, the invertedPercentageFee is 1
+  // if the fee is 100%, the invertedPercentageFee is 0, which is not allowed


"Technically", it's possible that the fee could be more than 100%. Instead of having an invertedPercentageFee, I believe we instead can do something like
minSendAmount = quoteMinSendAmount + ceil((quoteMinSendAmount + fixedFee) * feePercentage) + fixedFee

The equation used by me resulted from the way we apply fees in Fee.calculate:
Fee = (principal * feePercentage) + fixedFee

I have tried your equation
minSendAmount = quoteMinSendAmount + ceil((quoteMinSendAmount + fixedFee) * feePercentage) + fixedFee
with the following values:
quoteMinSendAmount = 100
fixedFee = 50
feePercentage = 0.2
results: minSendAmount = 180

When calculating quoteMinSendAmount which has the formula of:
quoteMinSendAmount = minSendAmount - Fee
quoteMinSendAmount = minSendAmount - ((principal * feePercentage) + fixedFee)
100 = 180 - (180 * 0.2 + 50)
100 = 180 - 86 => 100 != 94

Taking more than 100% would work only in the case of fixed receive amount because you can add whatever amount you want to the debit, but not in the case of fixed debit because receive amount can only be lower than debit if it makes sense...

Ah, yes, tripped myself up 😄 the percentage fee of course can't be more than 100%.

Following this now.

x - (percentageFee * x) - fixedFee = quoteMinSendAmount

quoteMinSendAmount = 100
fixedFee = 50
feePercentage = 0.2

x - 0.2x - 50 = 100
0.8x - 50 = 100
0.8x = 150
x = 150 / 0.8
ie (quoteMinSendAmount + fixedFee) / invertedPercentageFee)
x = 187.5

Co-authored-by: Max Kurapov <max@interledger.org>

…into cozmin/raf-998

mkurapov · 2025-06-04T09:54:58Z

-        code: PaymentMethodHandlerErrorCode.QuoteNonPositiveReceiveAmount
+        code: PaymentMethodHandlerErrorCode.QuoteNonPositiveReceiveAmount,
+        details: {
+          minSendAmount: BigInt(ilpQuote.highEstimatedExchangeRate.b.valueOf())


Suggested change

minSendAmount: BigInt(ilpQuote.highEstimatedExchangeRate.b.valueOf())

minSendAmount: BigInt(Math.ceil(ilpQuote.highEstimatedExchangeRate.reciprocal().valueOf()))

oh, right. thank you for spotting! fixed

mkurapov · 2025-06-04T09:56:35Z

+        details: {
+          minSendAmount: BigInt(
+            Math.ceil(ilpQuote.highEstimatedExchangeRate.reciprocal().valueOf())
+          )
+        }


let's also add this to L174 (if (ilpQuote.minDeliveryAmount <= BigInt(0)) condition)

mkurapov · 2025-06-05T10:09:24Z

    // e.g. if maxSourceAmount is 4 and the high estimated exchange rate is 0.2, 4 * 0.2 = 0.8
    // where 0.8 < 1, meaning the payment for this quote won't be able to deliver a single unit of value,
-    // even with the most favourable exchange rate. We throw here since we don't want any payments
+    // even with the most favorable exchange rate. We throw here since we don't want any payments


American English autocorrect? 😄

yes.. sorry about that. Installed the British version to avoid this in the future. Btw, do we adhere to a specific english?

No it's totally fine 😄 as long as its (one of) the correct spellings & the comment is understandable. Don't worry about changing anything, it was just my Canadian coming out :)

mkurapov · 2025-06-05T10:11:05Z

+        code: PaymentMethodHandlerErrorCode.QuoteNonPositiveReceiveAmount,
+        details: {
+          minSendAmount: BigInt(
+            Math.ceil(ilpQuote.highEstimatedExchangeRate.reciprocal().valueOf())
+          )
+        }


Let's add a test for this case as well

cozminu added 4 commits April 11, 2025 18:02

feat(backend): change error response

2ed1096

Merge branch 'main' into cozmin/raf-998

6ddba01

feat(backend): send minSendAmount

fddf4fc

feat(backend): return minSendAmount if debitAmount is too small

ea50542

cozminu requested a review from mkurapov April 28, 2025 12:55

cozminu self-assigned this Apr 28, 2025

github-actions Bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Apr 28, 2025

mkurapov reviewed May 6, 2025

View reviewed changes

cozminu and others added 4 commits May 7, 2025 10:33

Apply commit suggestions

7ef333f

Co-authored-by: Max Kurapov <max@interledger.org>

feat(backend): fix calculation for minSendAmount

02f1f14

feat(backend): improved minSendAmount calculation

295d09c

feat(backend): minSendAmount for local payments

3a051ca

cozminu requested a review from BlairCurrey May 13, 2025 14:28

sidvishnoi mentioned this pull request May 15, 2025

perf(bg/PaymentSession): fewer quote requests using exchange rate interledger/web-monetization-extension#1056

Merged

BlairCurrey reviewed May 16, 2025

View reviewed changes

fix(backend): integration tests

0fa2b9f

mkurapov reviewed Jun 2, 2025

View reviewed changes

cozminu and others added 5 commits June 3, 2025 10:07

fix(backend): minSendAmount using ratio reciprocal

d481f65

Co-authored-by: Max Kurapov <max@interledger.org>

feat(backend): add error details to graphql reponse

0700a91

Merge branch 'cozmin/raf-998' of https://github.com/interledger/rafiki …

74cc170

…into cozmin/raf-998

fix(backend): formatting

ecfa787

fix(backend): minSendAmount as BigInt

77f8ac7

mkurapov reviewed Jun 4, 2025

View reviewed changes

fix(backend): return minSendAmount

54e6703

mkurapov reviewed Jun 5, 2025

View reviewed changes

cozminu added 2 commits June 10, 2025 11:52

feat(backend): add test for minDeliveryAmount

a2a0a57

fix(backend): lint fix

e710876

mkurapov approved these changes Jun 10, 2025

View reviewed changes

cozminu merged commit 7d8079f into main Jun 10, 2025
44 checks passed

cozminu deleted the cozmin/raf-998 branch June 10, 2025 11:44

sidvishnoi mentioned this pull request Jun 23, 2025

Optimise minSendAmount probing with response from OP server interledger/web-monetization-extension#1106

Closed

	(sendingFee?.calculate(fixedFee + quoteMinSendAmount) ?? 0n)
	(sendingFee?.calculate(quoteMinSendAmount) ?? 0n)

	minSendAmount: BigInt(ilpQuote.highEstimatedExchangeRate.b.valueOf())
	minSendAmount: BigInt(Math.ceil(ilpQuote.highEstimatedExchangeRate.reciprocal().valueOf()))

Conversation

cozminu commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Context

Checklist

Uh oh!

netlify Bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Uh oh!

github-actions Bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Performance Test Results

Uh oh!

mkurapov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozminu May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozminu May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BlairCurrey May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozminu May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozminu May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BlairCurrey May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cozminu commented Apr 28, 2025 •

edited

Loading

netlify Bot commented Apr 28, 2025 •

edited

Loading

github-actions Bot commented Apr 28, 2025 •

edited

Loading

mkurapov May 6, 2025 •

edited

Loading

cozminu May 8, 2025 •

edited

Loading

cozminu May 7, 2025 •

edited

Loading

BlairCurrey May 15, 2025 •

edited

Loading

cozminu May 8, 2025 •

edited

Loading

cozminu May 19, 2025 •

edited

Loading

BlairCurrey May 16, 2025 •

edited

Loading