Skip to content

fix: quote the exact sat amount to USD-wallet recipients (1.5.3) - #17

Merged
pretyflaco merged 1 commit into
mainfrom
fix/sats-denominated-usd-wallet-invoices
Sep 14, 2026
Merged

pretyflaco merged 1 commit into
mainfrom
fix/sats-denominated-usd-wallet-invoices

Conversation

@pretyflaco

Copy link
Copy Markdown
Collaborator

The bug

A sats donation to a recipient whose default wallet is USD was converted to USD cents before invoicing, so the donor was asked for more than they entered. Since the currency selector defaults to sats, this applied to every sats donation to a USD-wallet account.

A donor entering 1000 was quoted 1007 sats. Two errors stack:

Whole-cent rounding 1000 sats x 0.077936 c/sat = 77.936c -> 78c. A cent is ~13 sats at current rates, so this alone shifts the total.
Stablesats dealer spread Blink prices those 78c back into sats for the payer at the immediate-buy price: 1007 sats, ~0.7% above mid.

The fix

Blink has a mutation for exactly this case:

lnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipient(
  input: LnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipientInput!): LnInvoicePayload!
# amount: SatAmount!  "Amount in satoshis."
# recipientWalletId   "Wallet ID for a USD wallet which belongs to the account of any user."

Its own schema description: "Returns a lightning invoice denominated in satoshis... When invoice is paid the equivalent value at invoice creation will be credited to a USD wallet. Expires after 'expiresIn' or 5 minutes." The donor pays the sats they entered; the recipient still receives USD. It is public like the other on-behalf-of mutations (the resolver takes no auth context), and keeps the same 5-minute expiry.

handleDonate now decides the invoice denomination as well as the amount, and createInvoice takes an optional 4th argument (amountUnit: 'sats' | 'cents') that selects the mutation:

wallet donor currency mutation amount
BTC any lnInvoiceCreateOnBehalfOfRecipient sats
USD fiat lnUsdInvoiceCreateOnBehalfOfRecipient cents
USD sats lnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipient sats, exact

Omitting amountUnit keeps the previous behaviour, so the existing call shape still works.

The sats path also no longer needs fetchExchangeRate('USD') — one fewer network round-trip, and its "USD exchange rate not available" failure mode disappears.

Unchanged: fiat donations still invoice in cents; BTC-wallet recipients are untouched; the self-custodial (Spark) LNURL path is untouched (Spark is BTC-only and always invoiced sats); memo, public API and DOM are identical. convertToUsdCents() is left intact and still characterized by widget-currency.spec.js — it is simply no longer on the sats invoice path, and a comment at the call site records why.

Verification

Live against api.blink.sv on all three paths (via debug-test.html, GraphQL bodies captured):

path mutation sent amount resulting invoice
USD wallet + 1000 sats ...BtcDenominated... "1000" lnbc10u1... = exactly 1000 sats (was ~1007)
USD wallet + $1 LnUsdInvoiceCreate... "100" cents 1291 sats (unchanged)
BTC wallet + 1234 sats LnInvoiceCreate... "1234", expiresIn: 15 1234 sats (unchanged)

Automated: npm test green - 166 tests, 13 new in tests/widget-invoice-denomination.spec.js covering all four wallet/currency combinations at the handleDonate level (including "no exchange-rate lookup happens for sats -> USD wallet", the regression guard) and all three mutations at the createInvoice level, plus the legacy 3-argument call. npm run lint reports 0 errors (2 pre-existing warnings, untouched code).

Prettier: js/blink-pay-button.js already fails npm run format on main (11 files repo-wide do), so it is deliberately not reformatted here - that would bury a 70-line fix in a 2.8k-line diff. The new spec file is prettier-clean.

Notes for the reviewer

  • Version header and analytics widget_version bumped to 1.5.3 per AGENTS.md.
  • Embeds are not version-pinned, so merging redeploys this to all live sites. The change is confined to which mutation is called and with what unit; the QR/DOM/callback contract is untouched.
  • Tiny sats donations to USD wallets behave no worse than before: previously Math.round(1 * 0.0779) = 0 cents produced an API error, now a 1-sat invoice is attempted directly.
  • Found while building blink-paywall, which had the same bug and the same fix.

A sats donation to a recipient whose default wallet is USD was converted to
USD cents before invoicing, so the donor was asked for MORE than they entered.
Two errors stacked:

  1. Rounding to a whole cent. 1000 sats x 0.077936 c/sat = 77.936c -> 78c.
     A cent is ~13 sats at current rates, so this alone moves the total.
  2. The stablesats dealer spread. Blink prices those 78c back into sats for
     the payer at the immediate-buy price: 1007 sats, ~0.7% above mid.

Donors default to the sats selector, so this applied to every sats donation to
a USD-wallet account.

Blink has a mutation for exactly this case:
lnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipient takes an amount in
satoshis against a USD wallet and credits the recipient its value at creation,
so the donor pays the sats they entered and the recipient still receives USD.
It is public like the other on-behalf-of mutations and carries the same
5-minute expiry (an exchange rate is attached to the amount).

handleDonate now decides the invoice DENOMINATION as well as the amount, and
createInvoice takes an optional 4th argument (amountUnit: 'sats' | 'cents')
that selects the mutation. Omitting it keeps the previous behaviour, so the
existing call shape still works. The sats path also no longer needs
fetchExchangeRate('USD'), removing a network round-trip and its
"USD exchange rate not available" failure mode.

Unchanged: fiat donations still invoice in cents, BTC-wallet recipients are
untouched, the self-custodial (Spark) LNURL path is untouched (Spark is
BTC-only and always invoiced sats), and the memo, public API and DOM are the
same. convertToUsdCents() is left intact and still characterized by
widget-currency.spec.js; it is simply no longer on the sats invoice path, and
a comment at the call site records why.

Verified against the live API on all three paths:
  USD wallet + 1000 sats -> BtcDenominated mutation, amount "1000"
                            -> lnbc10u1... (exactly 1000 sats, was ~1007)
  USD wallet + $1        -> cent mutation, amount "100" -> 1291 sats
  BTC wallet + 1234 sats -> sats mutation, expiresIn 15 -> 1234 sats

@blink-dev-bot blink-dev-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consolidated Multi-Model Review

Findings

No consensus functional findings. The new USD-wallet + sats path selects the live public lnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipient mutation with a SatAmount, preserves the exact entered sat amount, and removes the unnecessary exchange-rate fetch. Fiat-to-USD and all BTC-wallet paths remain intact; the optional fourth createInvoice argument preserves the historical three-argument behavior.

Diverging opinions

  • GPT recommended COMMENT because of low-risk coverage gaps, two whitespace-only lines, and the lack of an independently reproduced paid smoke test. Kimi and GLM recommended APPROVE. I verified the live GraphQL mutation/input contract, confirmed the PR author documented live API checks for all three routes, and found none of the concerns merge-blocking, so the reconciled disposition is APPROVE.

Test Coverage Gaps

  • The focused new spec passed locally: 10 tests covering all wallet/donor-currency combinations, no-rate-fetch behavior, all three mutations, exact variables, expiry values, backward compatibility, and unsupported currency.
  • Remote CI is green on this exact head: Unit tests and ESLint both passed.
  • The exact-head local full suite ran all 166 tests successfully but exited 1 because Vitest reported two unhandled asynchronous ReferenceError: document is not defined exceptions from timers in pre-existing tests/widget-callbacks.spec.js:27. Therefore the local full-suite command is not green, despite the assertions passing.
  • No coverage provider is configured, so changed-line/branch coverage was assessed manually. The substantive changed branches are covered; low-risk omissions are fractional-sat rounding and the analytics widget_version literal. A real paid smoke was not independently repeated during this review; the PR body records live checks for all three affected routes.

Code Smells

  • js/blink-pay-button.js:1940 and :1949 add trailing whitespace, causing git diff --check to fail. This is cosmetic and non-blocking.
  • createInvoice(walletId, amount, currency, amountUnit) has two stringly-typed denomination inputs that could be mismatched by a future direct caller. The only production caller passes constants consistently, and focused tests cover every current combination, so this is a low-risk maintainability concern rather than a defect.
  • No additional material SOLID-specific issues were found.

Reviewer notes

  • Reviewer A — openai-codex/gpt-5.6-sol: no functional defects; recommended comment-level follow-up for coverage and whitespace.
  • Reviewer B — kimi-coding/kimi-k3: no functional defects; approved.
  • Reviewer C — zai/glm-5.3-flash: no functional defects; approved.

Coverage: full multi-model (3 distinct models)

Summary

Disposition: APPROVE. Reviewed head f69b0a3adae495dc22ae56a9b1e600d98685f312. No model substitutions.

🤖 Multi-model review (gpt-5.6-sol + kimi-k3 + glm-5.3-flash) via @blink-dev-bot

@pretyflaco
pretyflaco merged commit 5b4a791 into main Sep 14, 2026
2 checks passed
@pretyflaco
pretyflaco deleted the fix/sats-denominated-usd-wallet-invoices branch September 14, 2026 10:49
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.

2 participants