Skip to content

fix(cli): bump SDK pin past #397 + treat invoice amounts as human units#35

Merged
vrogojin merged 1 commit into
mainfrom
fix/sdk-bump-and-invoice-amount-units
Jun 5, 2026
Merged

fix(cli): bump SDK pin past #397 + treat invoice amounts as human units#35
vrogojin merged 1 commit into
mainfrom
fix/sdk-bump-and-invoice-amount-units

Conversation

@vrogojin
Copy link
Copy Markdown
Contributor

@vrogojin vrogojin commented Jun 5, 2026

Two related fixes surfaced by re-running the soak suite (manual-test-{full-recovery,roundtrip-391,accounting-roundtrip}.sh) against main after merging PR #33 (canonical UX) and PR #34 (integration/all-fixes rollup).

(1) ci: bump pinned sphere-sdk SHA from 3f3dadfd949844

The previous pin (3f3dadf) predates sphere-sdk PR #400 / #397, which
routes invoice delivery through the standard TOKEN_TRANSFER pipeline
(Nostr kind 31113) instead of the bespoke invoice_delivery: NIP-17 DM.

Symptom against the old pin:

$ bash manual-test-accounting-roundtrip.sh
…
Invoice delivery result:
{ "sent": 1, "failed": 0, ... }       ← bob's deliver thinks it succeeded
…
Syncing... Ready. No invoices found.  ← alice never sees it
…
ASSERT FAIL (invoice-ingest-timeout): alice's wallet did NOT ingest invoice … within 60s

The new pin (d949844) is sphere-sdk main and includes:

  • 903fc80 fix(accounting)(#397): route invoice delivery through the token pipeline
  • 541b448 feat(accounting)(#401): wire publishUxfBundle into OUTBOX/SENT
  • 85700b6 Merge PR #400 (#397)
  • d949844 Merge PR #402 (#401)

After bump, the receiver decoder recognises bob's invoice DM (because
it's just a TOKEN_TRANSFER of an INVOICE_TOKEN_TYPE_HEX token), the
existing OUTBOX/SENT/at-least-once machinery handles it end-to-end,
and Alice's invoice list surfaces it within seconds.

(2) cli: invoice-create + invoice-return amount in human units

Cross-command inconsistency that PR #32 (canonical UX) was supposed to
eliminate but missed at rebase time. Before this PR:

sphere payments send @bob 7 UCT             → 7 whole UCT (7×10^18 atoms)
sphere invoice create --asset 7 UCT         → 7 ATOMS (!!)  ← inconsistent
sphere invoice return … --asset 100 UCT     → 100 ATOMS (!!) ← inconsistent

After this PR, all three accept human units (the same shape payments send already did). Conversion is done via toSmallestUnit(amount, decimals) where decimals comes from resolveCoin(symbol). The
SDK's AccountingModule.createInvoice continues to treat the amount
as a raw atom count (decimals: 0) — the conversion happens entirely
in the CLI, matching the payments send pattern.

Bonus: 0.5 BTC and other fractional inputs now work for invoices.

invoice-pay --amount is intentionally NOT changed in this PR — it
needs an additional SDK lookup for the invoice's coin/decimals, which
warrants its own change. The soak doesn't exercise --amount so this
is non-blocking. (TODO follow-up.)

Verification

$ npx tsc --noEmit       # clean
$ npx vitest run         # 126/126 pass
$ bash manual-test-accounting-roundtrip.sh   # ALL GREEN
ASSERT OK (deliver-acked):           invoice delivery DM sent
ASSERT OK (invoice-covered):         bob's invoice transitioned to COVERED or CLOSED
ASSERT OK (bob-uct-delta-plus-7):    bob received exactly 7 UCT
ASSERT OK (alice-uct-delta-minus-7): alice paid exactly 7 UCT
ASSERT OK (alice-balance-1):         no unconfirmed residue post-finalize
ASSERT OK (bob-balance-2):           no unconfirmed residue post-finalize
bob UCT delta:   +7000000000000000000  (= 7 UCT exactly)
alice UCT delta: -7000000000000000000  (= 7 UCT exactly)

The other two soaks (roundtrip-391, full-recovery) are unaffected
by either change — they don't pass --asset <amount> <coin> for an
invoice amount.

Risk

  • The invoice-amount semantic change is breaking for anyone who
    was passing pre-converted atom counts to invoice create --asset
    / invoice return --asset. The fix matches payments send's
    established convention, so anyone already passing 7 UCT was
    silently creating an invoice for 7 atoms (almost certainly a bug,
    not intended behavior). Documented in the commit message + the
    COMMAND_HELP help blocks.

== Two fixes, one PR ==

(1) ci: bump SPHERE_SDK_SHA from 3f3dadf → d949844 (current sphere-sdk
    main tip), which includes:
      - 903fc80  fix(accounting)(#397): route invoice delivery through
                 the token pipeline  (the architectural fix)
      - 541b448  feat(accounting)(#401): wire publishUxfBundle into
                 OUTBOX/SENT + recovery exhaustion event
      - 85700b6  Merge PR #400 (#397)
      - d949844  Merge PR #402 (#401)

    Pre-bump symptom: `manual-test-accounting-roundtrip.sh` failed at
    Alice's "wait for invoice to be ingested" step. Bob's `invoice
    deliver` reported `sent: 1, failed: 0` but Alice's `invoice list`
    showed "No invoices found" forever, because the old SDK still used
    the bespoke `invoice_delivery:` NIP-17 DM and the soak script (and
    the matching receiver decoder) was authored against the new
    TOKEN_TRANSFER (kind 31113) pipeline.

(2) cli(legacy-cli.ts): make invoice-create and invoice-return treat
    the `--asset <amount> <coin>` amount as HUMAN units (e.g.
    "7 UCT" = 7 whole UCT, "0.5 BTC" = 0.5 BTC), matching what
    `payments send 7 UCT` does. Convert to smallest-unit integers
    via toSmallestUnit(amount, decimals) before handing off to the
    SDK (AccountingModule treats the amount as a raw atom count with
    decimals: 0).

    Closes the cross-command inconsistency that PR #32 set out to
    eliminate but missed at rebase time. The accounting soak's
    `--asset 7 UCT` now produces a 7-UCT invoice instead of a
    7-atom invoice; the assertion that bob receives exactly
    7000000000000000000 atoms passes.

== Verification ==

- npx tsc --noEmit: clean
- npx vitest run: 126/126 pass
- manual-test-accounting-roundtrip.sh (real testnet): ALL GREEN
  (bob +7 UCT, alice -7 UCT, invoice COVERED/CLOSED)
- manual-test-roundtrip-391.sh and manual-test-full-recovery.sh
  unaffected (they don't use --asset for invoice amounts)
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.

1 participant