feat(assets): Arkade asset management + dedicated Arkade Asset payment method#55
feat(assets): Arkade asset management + dedicated Arkade Asset payment method#55Kukks wants to merge 26 commits into
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — feat/arkade-assets-payment
Well-structured PR. Clean separation between rate model, metadata cache, settlement, and UI. The design doc and test coverage are solid — 11 unit tests on the resolver math, 2 e2e config tests, and the NNark parity work in #94 are all well-motivated. SDK parity analysis is thorough.
That said, the settlement path is protocol-critical (asset VTXOs settling invoices = money), so I'm requesting changes on a few items and flagging for human review.
🔴 P0 — Must Fix
1. Overpayment in assets is silently capped — merchant accounting breaks
ArkContractInvoiceListener.cs (new code, ~line 148 in diff):
var ratio = Math.Min(1m, (decimal)received / promptDetails.AssetBaseUnitsDue);
assetCreditBtc = arkPrompt.Calculate().TotalDue * ratio;The Math.Min(1m, ...) cap means a customer who sends 2x the required asset units gets only 1x BTC credit recorded. BTCPay normally surfaces overpayment to the merchant (refund workflows, accounting reconciliation). Here, excess asset is received on the VTXO but invisible in BTCPay's books.
Fix: Remove the Math.Min(1m, ...) cap and let the proportional credit reflect actual receipt. BTCPay's existing overpayment accounting will handle the rest. If there's a reason to cap (e.g. preventing inflated BTC credit for a non-BTC denomination), document it prominently.
2. Math.Pow precision loss in AssetMetadataService.FormatAmount
AssetMetadataService.cs:50:
var divisor = (decimal)Math.Pow(10, decimals);Math.Pow returns double. For decimals > 15, 10^decimals exceeds double mantissa precision (2^53 ≈ 9×10^15). The cast to decimal bakes in the rounding error. AssetRateResolver.Pow10 already does this correctly with a decimal loop.
Fix: Extract Pow10 to a shared utility (or duplicate the decimal-loop approach) so both paths are precision-safe. The resolver and the display formatter must agree on the divisor.
🟡 P1 — Should Fix
3. Duplicate FormatAmount logic between two services
AssetMetadataService.FormatAmount and AssetRateResolver both format base units → display string, using different approaches (one Math.Pow + "0." + new string('#', …), the other Pow10 + "0." + new string('#', …)). Two paths to render the same number is a correctness risk.
Fix: One canonical FormatAmount, called from both sites.
4. CancellationToken.None in asset-enrichment calls during prompt config
ArkadePaymentMethodHandler.cs (new lines ~107–130):
var assetDetails = await assetMetadataService.GetAssetDetailsAsync(
acceptance.AssetId, CancellationToken.None);
...
assetDue = await assetRateResolver.ResolveAsync(
context.Store, acceptance, dueSats, assetDecimals, CancellationToken.None);If the indexer or rate provider hangs, these block the prompt indefinitely. GetServerInfoAsync already uses a 5-second timeout higher up. These should use a bounded token too.
5. No negative caching in AssetMetadataService
AssetMetadataService.cs:21–30: On indexer failure, the result is not cached, so every subsequent call retries the network round-trip. Under indexer outage, this becomes N×(timeout) per invoice/dashboard render.
Fix: Cache a sentinel null with a short TTL (30–60s) for failed lookups, or use IMemoryCache with expiration.
🟢 P2 — Nit / Consider
6. Unbounded ConcurrentDictionary cache
AssetMetadataService.cs:8: Process-lifetime cache with no eviction. Immutable-after-issuance justifies no TTL, but there's no size guard. Fine for a BTCPay plugin (merchants won't configure thousands of assets), but worth a comment acknowledging the assumption.
7. Settlement: TotalDue read outside _paymentLock
ArkContractInvoiceListener.cs (new code ~line 148): arkPrompt.Calculate().TotalDue is computed from the pre-lock inv fetch. Since TotalDue is an invoice-creation-time constant (not affected by subsequent payments), this is safe. But the code would benefit from a comment explaining why the pre-lock read is valid, since the existing HandlePaymentData re-fetches inside the lock for good reason.
8. Asset ID comparison is case-insensitive — confirm this matches arkd
ArkContractInvoiceListener.cs (new code ~line 138):
.Where(a => string.Equals(a.AssetId, promptDetails.AssetId, StringComparison.OrdinalIgnoreCase))Defensive, but confirm arkd normalizes asset IDs to lowercase hex. If arkd ever returns mixed-case and the prompt stores lowercase, Ordinal would fail — so OrdinalIgnoreCase is the right call. Just verify the assumption.
✅ What's Good
- Rate model design is clean: two well-defined modes, reference-currency leg routes through BTCPay's existing pipeline, no invented pseudo-currencies.
- Round-up-never-underpay in
AssetRateResolverwith≥1 base unitclamp — correct and well-tested. - Stray-BTC rejection in settlement (received == 0 → return) prevents wrongful settlement on non-asset VTXOs.
IsValidvalidation onArkadeAssetAcceptanceis thorough, server-side (not just JS).- Indexer existence check before persisting asset config prevents phantom-asset configs.
- Test coverage: 11 unit tests on resolver math pin the rules that matter (rounding, clamping, guards), 2 e2e tests on config validation round-trip.
- Additive schema changes:
AssetAcceptancenullable on config, new fields nullable onArkadePromptDetails— backward-compatible deserialization. - No cross-repo breakage: all changed types are plugin-internal; NNark submodule bump (#94) is an independent fix.
⚠️ Protocol-Critical Flag
The settlement path in ArkContractInvoiceListener.OnVtxoChanged — asset recognition, proportional BTC credit, stray-VTXO rejection — is protocol-critical. Bugs here under/over-credit merchants. Requires human sign-off before merge, per project rules.
Verdict: Request Changes (P0 items 1–2). The rest of the PR is solid.
|
Thanks for the thorough review — all items addressed in 🔴 P0
🟡 P1
🟢 P2
The protocol-critical settlement path is flagged for human sign-off as you noted — the change there is the cap removal + clarifying comments only. |
There was a problem hiding this comment.
Arkana Follow-Up Review — 484c1724
All P0/P1/P2 items from the previous review are resolved. Checking each:
✅ P0 #1 — Overpayment cap removed
ArkContractInvoiceListener.cs:~151: Math.Min(1m, ...) removed. Ratio is now uncapped so overpayment flows through BTCPay's existing reconciliation. Good comment explaining the TotalDue pre-lock read safety.
✅ P0 #2 — Math.Pow precision
New AssetAmount.Pow10 uses a decimal loop, clamped 0..18. Exact for all asset-decimals values. AssetMetadataService.FormatAmount and AssetRateResolver both delegate to this single implementation.
✅ P1 #3 — Duplicate FormatAmount
Unified into AssetAmount.Format. One canonical path for both display and rate resolution.
✅ P1 #4 — Unbounded CancellationToken.None
ArkadePaymentMethodHandler.cs:~117: 10-second bounded CTS for indexer + rate calls. Catch filter extended to OperationCanceledException. Clean.
✅ P1 #5 — Negative caching
AssetMetadataService.cs: 60-second negative cache via ConcurrentDictionary<string, DateTimeOffset>. Covers both indexer errors and null returns. Cleared on subsequent success.
✅ P2 #6–#8 — Comments
Unbounded cache rationale documented. Pre-lock TotalDue read explained. Case-insensitive asset ID comparison annotated.
New code review — AssetAmount.cs
Pow10: Correct. Clamp prevents out-of-range. Decimal loop is exact.Format: Correct.decimals <= 0early-return avoids division. Format string"0." + "#"×decimalstrims trailing zeros properly. InvariantCulture prevents locale issues.- Test coverage: 7
Formatcases including the 1e-18 regression guard, 5Pow10exactness cases, and boundary clamping. Solid.
No new issues found. Approving.
There was a problem hiding this comment.
Arkana Follow-Up Review — e6bf00b3
Submodule re-pin only. No code changes.
submodules/NNark:5509d8a→bbcd960. Verifiedbbcd960is HEAD ofarkade-os/dotnet-sdkmaster (squash-merge of PR #94). Old pin5509d8awas on the now-deletedassets-paritybranch — unreachable in fresh clones. Fix is correct.docs/...design.md: Text update documenting the re-pin rationale. Accurate.
Previous approval sustained. No new issues.
PR #25's asset display intent re-applied onto current master (its storage/migration commits are obsolete — VTXO storage moved into the NNark SDK, which already persists ArkVtxo.Assets end-to-end). - AssetMetadataService: process-lifetime cache over the indexer's GetAssetDetailsAsync; exposes name/ticker/decimals + decimals-aware amount formatting. - ArkBalancesViewModel.AssetBalances + AssetBalanceViewModel. - GetArkBalances aggregates ArkCoin.Assets across spendable coins, enriches via the metadata cache, sorted by ticker/id. - _ArkBalances.cshtml renders an "Arkade assets" section in the detailed balance card (data-testid=asset-balances/asset-balance). - DI registration in RegisterPluginServices. Plugin builds clean (0 errors) on current master with the asset-aware SDK (NNark @ assets-parity 8c0fe77).
Extends ArkadePaymentMethodConfig with an optional trailing AssetAcceptance record (additive, serialization-safe). Introduces AssetRateMode (FixedReferenceCurrency | SatsPerUnit) and ArkadeAssetAcceptance with internal-consistency validation. Null = asset acceptance off, BTC-VTXO behaviour unchanged.
AssetRateResolver prices a merchant-accepted asset against an invoice's BTC amount due. SatsPerUnit is self-contained; FixedReferenceCurrency routes the BTC->reference leg through the store's own RateFetcher rules (same path payouts use), so spread/fallback/rate-source config keep applying. Base units rounded up (never underpay), clamped to >=1. ConfigurePrompt resolves and stashes asset id/ticker/decimals/amount-due into ArkadePromptDetails when AssetAcceptance is set; on rate failure the asset method is simply not offered (PaymentMethodUnavailable), the BTC destination/expiry machinery untouched.
Adds an Asset Payments row + modal on the Arkade store overview: asset id, pricing model (FixedReferenceCurrency | SatsPerUnit), price/unit, and reference currency (shown only for the fixed-currency model). save-asset-acceptance validates the config and verifies the asset exists on the indexer before enabling; disable-asset-acceptance clears it. StoreOverviewViewModel carries the current config + resolved label.
The TrimEnd('0') formatting corrupted whole amounts ("100" -> "1"),
which would have shown customers a 100x-too-small asset amount due.
Fixed: decimals==0 formats the integer base units directly; otherwise
'0.###' trims only fractional zeros. 11 standalone unit tests
(SatsPerUnit math, round-up-never-underpay, >=1 base unit clamp,
invalid-config / bad-currency / non-positive-due guards).
When an invoice is priced in an accepted Arkade asset, the checkout
extension passes assetAcceptance (id/name/ticker/decimals/amount) via
CheckoutModel.AdditionalData; the Vue checkout component renders a
prominent 'Pay {amount} {ticker}' notice above the QR, clarifying the
customer settles by sending the asset to the same Ark address (the BTC
figure being the reference value).
When an invoice's Arkade prompt carries an asset id + base-units due, the contract listener credits BTC proportional to the asset amount on the incoming VTXO (capped at 100%), instead of the carrier VTXO's dust sats — so partial/settled/overpaid accounting stays correct. A VTXO that carries none of the expected asset is ignored for that invoice (no wrongful settle on stray BTC).
Two integration tests driving the real overview modal: unknown asset id rejected with the indexer-not-found error (and not persisted), and fixed-currency mode without a reference currency rejected server-side by IsValid. Full pay-with-asset settlement needs issued-asset funding infra; money math is covered by AssetRateResolverTests.
P0-1: remove Math.Min(1m,...) overpayment cap in settlement — credit BTC strictly proportional to asset received so over-payments surface in BTCPay accounting instead of being silently clamped. P0-2: precision-safe Pow10 — new shared AssetAmount.Pow10 (decimal loop, exact for decimals>15) replaces (decimal)Math.Pow(10,n). P1-3: canonical AssetAmount.Format used by both AssetRateResolver and AssetMetadataService.FormatAmount — single rendering, no divergence. P1-4: bound the indexer+rate round-trips in ConfigurePrompt with a 10s CTS; catch OperationCanceledException → PaymentMethodUnavailable. P1-5: negative-cache failed indexer lookups (60s TTL) so a down indexer isn't re-hit every render. P2: comments — unbounded success cache assumption, pre-lock TotalDue validity, case-insensitive asset-id rationale. +13 AssetAmount unit tests (Pow10 exactness incl. 10^18, clamp, Format incl. 1e-18). 24/24 unit green.
PR #94 was squash-merged into NNark master as bbcd960; the assets-parity branch and its pre-squash commits (8c0fe77, 5509d8a) were orphaned and the branch deleted. The plugin pinned the now- unreachable 5509d8a — a fresh CI/submodule clone could not fetch it. Re-pin to the durable master commit bbcd960 (identical tree, zero file-content change). Build clean, 24/24 unit tests green.
Kukks
force-pushed
the
feat/arkade-assets-payment
branch
from
e6bf00b to
ad1a707
Compare
…method + display)
… currency registration, CRUD Replace the single ArkadeAssetAcceptance with a managed list of tracked assets: - ArkadePaymentMethodConfig.TrackedAssets + TrackedArkadeAsset (validated record) - AssetRateResolver evaluates a free-form per-asset BTCPay rate script, combined into the store rules primary/fallback legs via RateRules.Combine (the global store RateScript is never mutated) then priced through RateFetcher; settlement money-math extracted to the pure AssetAmount.BaseUnitsDue - ArkadeAssetCurrencyDataProvider registers each tracked code as a BTCPay currency; AssetCurrencyRegistrar reloads the currency table after CRUD - ArkController: add/edit/remove-asset commands + asset-metadata fetch endpoint - StoreOverview: tracked-asset list + add/edit modal with add-by-id prefill - Drop the old asset block from the BTC-VTXO Arkade handler Asset pricing + settlement move to the dedicated ARKADE-ASSET method (next). Tests: AssetRateResolverTests (money-math + guards) + TrackedArkadeAssetTests (validation) — 28 passing.
- _VtxoTable + Vtxos: render a per-asset badge (formatted amount + ticker, indexer-resolved) beneath the BTC amount on asset-carrying VTXOs - _ArkBalances: mirror the full-mode Arkade-asset section into the compact (dashboard) branch so asset balances show inline alongside BTC
…t options Asset checkout options are ark-only URIs carrying ark=<addr>, asset=<id>, and amount=<asset display units> (per the Arkade convention, amount denominates the asset when an asset param is present). Settlement still matches by asset id and the server-stored base units, so the URI amount is wallet-prefill UX only.
…ablement) - ArkadeAssetPaymentMethodConfig: thin per-store enabler (wallet id only); the asset list stays on the ARKADE config (single source of truth) - ArkadeAssetPromptDetails + ArkadeAssetOption: one Ark address + one option per enabled asset (asset id, currency, base units due, formatted due, BIP-321 URI) - ArkadeAssetPaymentMethodHandler: derives a receive address, prices every enabled asset via AssetRateResolver, builds per-asset options; an asset that can't be priced is skipped, and the method is unavailable if none resolve - ArkController.SyncAssetPaymentMethod: enables the method iff the store has an enabled tracked asset (called on asset CRUD); ClearWallet clears it too - Register handler + "Arkade Asset" pretty name (checkout/link wiring next)
- ArkadeAssetCheckoutModelExtension: selects the arkadeAssetCheckoutBody component and hands it one option per enabled asset (ticker, amount due, BIP-321 URI) - ArkadeAssetMethodCheckout component: a payer-facing picker — select an asset, see its amount due + QR + pay-in-wallet link; all options share one Ark address (model.address) - ArkadeAssetPaymentLinkExtension: default link = first option's BIP-321 URI - Register link + checkout-model extensions and the checkout-end UI extension
- ArkContractInvoiceListener: when a VTXO arrives at an ARKADE-ASSET address, match its assets against the invoice's offered options by asset id and credit BTC proportional to the asset received (over-payment surfaces, never clamped); a VTXO carrying none of the offered assets is ignored - Generalize HandlePaymentData to any IPaymentMethodHandler + PaymentMethodId so it can register either the ARKADE or ARKADE-ASSET payment - ToggleArkadeContract resolves the wallet id from either Arkade prompt, so the asset method's receive contract is deactivated on settle even when only the asset tab was opened - Remove the now-dead single-asset embedding from the BTC-VTXO path: ArkadePromptDetails.Asset* fields, the checkout-model asset block, and the asset notice in the BTC checkout component
…e methods - ArkadePaymentMethodHandler: remove unused AssetMetadataService / AssetRateResolver ctor params (dead after asset pricing moved to the dedicated ARKADE-ASSET method) - ArkContractInvoiceListener.QueueMonitoredInvoices: scan both ARKADE and ARKADE-ASSET monitored invoices on startup (deduped by invoice id), so an invoice with the BTC-VTXO method excluded but the asset method active still has its contracts re-activated after a restart - Remove the now-dead GetListenedArkadeInvoice + ArkadeListenedContract (the GetArkadeInvoiceWalletId lookup supersedes them); literal cache-key prefix
Update AssetAcceptanceTests to the new add/edit modal: an unknown asset id is rejected by the indexer existence check (config not persisted), and an empty rate script is rejected by TrackedArkadeAsset.IsValid before any indexer lookup. Both paths are deterministic and need no issued asset; the money math, rate resolution and BIP-321 URI are covered by the unit suites.
Kukks
changed the title
…vider) ArkadeAssetCurrencyDataProvider injected PaymentMethodHandlerDictionary into its constructor. Because CurrencyNameTable takes IEnumerable<CurrencyDataProvider> and is built eagerly by LoadCurrencyNameTableStartupTask, this forced the whole payment-handler graph to resolve while CurrencyNameTable was still constructing — and core handlers (e.g. BitcoinLikePaymentHandler → DisplayFormatter) depend back on CurrencyNameTable, forming a cycle. MS.DI's StackGuard bounced it across threads and the host never finished starting (E2E: "BTCPay startup didn't complete within 3 minutes"; every integration test failed with ERR_CONNECTION_REFUSED). Resolve StoreRepository + PaymentMethodHandlerDictionary lazily via IServiceProvider inside LoadCurrencyData instead — by then CurrencyNameTable is a cached singleton, so the handler graph resolves without re-entering it. Same pattern the checkout extension already uses for its own cycle.
…ler dictionary ArkadeAssetPaymentMethodHandler injected PaymentMethodHandlerDictionary to read the BTC-VTXO ARKADE config, but it is itself registered as an IPaymentMethodHandler. HandlersDictionary enumerates IEnumerable<IPaymentMethodHandler> eagerly, so building the dictionary resolves this handler, which then needs the dictionary — a self-referential cycle that hangs startup via MS.DI StackGuard (host never came up; E2E showed all 30 integration tests failing with the 3-minute startup timeout and ERR_CONNECTION_REFUSED). Read the ARKADE config via store.GetPaymentMethodConfigs() + this handler's own BlobSerializer (both Arkade methods share the same serializer), dropping the dictionary dependency entirely. Pairs with the earlier CurrencyDataProvider fix.
…payment # Conflicts: # BTCPayServer.Plugins.ArkPayServer/ArkPlugin.cs # BTCPayServer.Plugins.ArkPayServer/Models/StoreOverviewViewModel.cs
1 participant
Arkade asset management + dedicated payment method
Lets a merchant track a managed list of Arkade assets and accept them as payment through a dedicated checkout where the payer picks which asset to send. Replaces the earlier single-asset "acceptance" prototype (never shipped) with a full CRUD + multi-asset model, a dedicated payment method, and asset display.
Design & plan:
docs/superpowers/specs/2026-05-22-arkade-asset-management-design.mddocs/superpowers/plans/2026-05-22-arkade-asset-management.mdPart 1 — Asset management (foundation)
ArkadePaymentMethodConfig.TrackedAssetsholds a list ofTrackedArkadeAsset(asset id, currency code, ticker, name, decimals, rate script, enabled). Add / edit / remove from the store overview's "Asset Payments" modal.GET stores/{id}/asset-metadata) to prefill the form. Add verifies the asset exists on the indexer before tracking it.USDARK_USD = 1;).AssetRateResolvercombines it into the store's existing rules (primary + fallback) viaRateRules.Combineand evaluates theBTC → assetpair throughRateFetcher— the globalStoreBlob.RateScriptis never mutated, and chained legs (asset→USD→BTC) resolve through the store's own rate-source / spread / fallback config.ArkadeAssetCurrencyDataProviderregisters each tracked code as a BTCPay currency;AssetCurrencyRegistrarreloadsCurrencyNameTableafter every CRUD op (no restart needed).Part 2 — Dedicated
ARKADE-ASSETpayment method (payer picks)ARKADEmethod.ArkadeAssetPaymentMethodHandlerderives one Ark receive address and prices every enabled asset, exposing one option per asset (asset id, amount due, BIP-321 URI). An asset whose rate can't be evaluated is skipped; the method is unavailable if none resolve.ArkadeAssetPaymentMethodConfig(wallet id) enables the method;ArkController.SyncAssetPaymentMethodwrites it iff the store has ≥1 enabled tracked asset, and clears it otherwise (and onClearWallet). The asset list stays on theARKADEconfig (single source of truth).arkadeAssetCheckoutBodycomponent renders a picker: select an asset → see its amount due + per-asset QR + pay-in-wallet link. All options settle to the same Ark address.ArkadeBip21Builder.WithAssetemitsark=<addr>&asset=<id>&amount=<asset display units>(per the Arkade convention,amountdenominates the asset whenassetis present).ARKADEsettlement path is untouched.Part 3 — Display
Vtxos,_VtxoTable)._ArkBalances).Removed
The earlier single-asset embedding on the BTC-VTXO prompt is gone:
ArkadePromptDetails.Asset*fields, the checkout-model asset block, and the BTC checkout's asset notice. The old rate modes (FixedReferenceCurrency/SatsPerUnit) are replaced by the free-form rate script. No back-compat migration — the asset feature never shipped (releases v2.1.15–v2.1.18 exclude it).Tests
AssetAmountmoney-math,AssetRateResolver(money-math + input guards),TrackedArkadeAssetvalidation,ArkadeBip21AssetURI.TrackedArkadeAsset.IsValid, before any indexer lookup).Review
An independent review found no critical / settlement / accounting bugs and confirmed the proportional-credit math, the no-double-credit guard (outpoint dedupe under
_paymentLock), the enablement lifecycle, and theBTC → assetrate direction. Two minor findings fixed: removed dead DI params onArkadePaymentMethodHandler;QueueMonitoredInvoicesnow scans both Arkade methods on startup so an invoice with the BTC-VTXO method excluded but the asset method active still has its contracts re-activated after a restart.Notes for the reviewer
ArkContractInvoiceListener) flagged for human sign-off per project rules.amountis wallet-prefill UX only.a9e990f, which carries the asset format/ops support.