feat(wallet): operator-editable asset catalog and custody adapter seam - #88
Closed
zaxovaiko wants to merge 1 commit into
Closed
feat(wallet): operator-editable asset catalog and custody adapter seam#88zaxovaiko wants to merge 1 commit into
zaxovaiko wants to merge 1 commit into
Conversation
Adds wallet_asset: the (currency, network) pairs the platform accepts, with per-pair minimums, withdrawal fee, and independent deposit/withdrawal enable flags. Currencies become admin-managed rows instead of a hardcoded list, so adding one is no longer a deploy. Public GET /wallet/assets serves the enabled pairs to the player pickers without the vendor asset id; the admin CRUD group is gated on a new 'wallet-asset' resource and audits every mutation in the mutating transaction. Removing a pair is blocked while players still hold that currency. WALLET_ASSET_CATALOG lets a payment adapter read the catalog without importing wallet internals; the wallet plugin binds a DB-backed default. PaymentAdapter gains supportsAsset, so a vendor can reject a pair it cannot serve at config time rather than at a player's deposit request, plus optional listSweepableBalances/sweepToPool/listTransactions/getWithdrawalStatus for custody vendors that pool per-player balances.
zaxovaiko
requested review from
agniev-a-hub,
damianrzepka,
klaudia-blazyczek-blurify and
marek-chmielowski-blurify
as code owners
August 20, 2026 20:47
klaudia-blazyczek-blurify
requested changes
Aug 21, 2026
| }), | ||
| }, | ||
|
|
||
| listAssets: os.listAssets.handler(() => wallet.listEnabledWalletAssets()), |
Collaborator
There was a problem hiding this comment.
This only filters the picker response. The deposit-address route still accepts any pair, and the deposit and withdrawal flows never read this catalog. A player can call the API directly after an asset is disabled or removed, bypassing the flags, limits, and fee. Enforce the configured asset in every server-side flow; a withdrawal also needs to carry and persist its network.
| this.assertAdapterSupports(currency, network); | ||
| } | ||
| return this.drizzle.db.transaction(async (txn) => { | ||
| const [before] = await txn |
Collaborator
There was a problem hiding this comment.
Lock this row, or use a versioned update, before reading it. Two admins can both read the same old state and update it in sequence, so the second audit record misses the first change. The audit trail needs the real before and after state.
| if ((held?.n ?? 0) > 0) { | ||
| throw new WalletAssetInUseError(); | ||
| } | ||
| await txn |
Collaborator
There was a problem hiding this comment.
Check that the delete actually removed a row before writing the audit record and returning true. Two concurrent deletes can both read the row; after the first commits, the second deletes nothing but still records another deletion and reports success.
Member
Author
|
Superseded by #92. Every commit here is in that branch - the asset catalog and the custody adapter seam were cherry-picked onto it unchanged, and the tests came with them. Closing this one rather than rebasing it, because the reason to fold it in was structural: this PR and #85 both generated a wallet migration No review comments were left here, so nothing is lost by moving. The work continues in #92 -> #95 -> #96. |
zaxovaiko
added a commit
that referenced
this pull request
Aug 22, 2026
…route (#92) ## Summary - Absorbs #85, #88 and #89, and adds the rest of the custody surface: the provider registry, the remaining tables, every request/response type, the permissions, and all five routes. - Generates **one** migration, `0010`, covering the whole effort - `wallet_asset`, `wallet_transaction.network`, the `manual_credit`/`manual_debit` enum values, `wallet_custody_sweep`, `wallet_job_run`, `wallet_reconciliation_finding` and four new enums. - `PAYMENT_PROVIDERS` resolves a vendor by the name an operator wrote on a `wallet_asset` row, so a fiat PSP and a crypto custodian can be bound at the same time. - The four sweep and reconciliation routes assert their permission for real, then fail with 501. The stacked PRs replace only the bodies. ## Why Six migrations across a five-deep stack means roughly 6000 lines of generated snapshot JSON to skim past, and every rebase of an earlier PR regenerates every later one. Nothing here has been applied to a deployed environment, so there is no history worth preserving - one migration is simply better. #85 and #88 both created a `0010`, which had to be resolved either way. After this PR **no further PR in this effort touches the database or the contract**, so the sweep and reconciliation PRs are pure logic and pure review attention where the money paths are. `providerName` also stops being a hardcoded vendor literal. It now resolves from the asset catalog, which is what reconciliation scopes its diff by. ## Alternatives considered Keeping #85, #88 and #89 as separate PRs and renumbering the migrations. That leaves three snapshots and a rebase chain that regenerates on every change, and #85 turned out to be five commits including a revert of its own work - replaying those individually would land a revert into a history that never had the thing being reverted. It is squashed to its net change instead. Stubbing the already-implemented endpoints from those three PRs for consistency. Rejected: they are written, tested and green, so unwriting and re-adding them is motion with a regression risk and nothing to show a reviewer. A second DI token for the custody methods rather than optional methods on `PaymentAdapter`. Optional methods match the existing `issueDepositAddress`/`parseWebhook` idiom and leave `MockPaymentAdapter` and every PSP adapter untouched. ## Risks `ALTER TYPE ... ADD VALUE` runs inside the migration transaction. Safe on PG12+ as long as the value is not used in the same transaction, and it is not; the repo runs `postgres:16`. Applied and verified two ways: on a fresh database, and on one already at `0009`. Four routes are live and return 501 until their stacked PRs land. They are marked `Not implemented yet` in the contract so the generated OpenAPI reference is honest. None of them returns a success shape - an empty list from the reconciliation report would read as "nothing to reconcile", which is the worst thing that endpoint could say while unimplemented. `wallet-custody` and `wallet-reconciliation` are new permission resources. The built-in `admin` role gets both here, but any role whose levels are already stored has no level for them and therefore no access until granted. `providerName` on `wallet_asset` fails closed against the bound registry, and is immutable on update plus blocked while an in-flight transaction exists for the pair, so a vendor reference cannot be rewritten out from under a payout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wallet_asset: the(currency, network)pairs the platform accepts, each with its own minimum deposit, minimum withdrawal, withdrawal fee, and independent deposit/withdrawal enable flags.GET /wallet/assetsserving the enabled pairs to the player currency and network pickers, plus an admin CRUD group behind a newwallet-assetpermission resource.WALLET_ASSET_CATALOGso a payment adapter can read the catalog without importing wallet internals. The wallet plugin binds a DB-backed default, so an operator gets a working catalog with no wiring.PaymentAdapterwithsupportsAsset, plus optionallistSweepableBalances,sweepToPool,listTransactionsandgetWithdrawalStatusfor a custody vendor that pools per-player balances.Why
Which currencies a platform accepts was operator config living in code and env vars, so adding one meant a deploy. The limits were also per-currency, which is wrong the moment a currency exists on more than one chain: USDT costs cents to move on one network and dollars on another, so a single currency-wide floor is wrong on at least one of them.
Deposit and withdrawal enable separately because they genuinely come up at different times - an operator can accept deposits for an asset before the withdrawal-side float behind it exists.
providerAssetIdis the only vendor-touching column and the wallet module treats it as an opaque string, so a custody vendor's identifier scheme stays entirely in that vendor's adapter. The four optionalPaymentAdaptermethods are the seam a sweeping and reconciliation job will sit on later - this PR only defines them, nothing calls them yet.Alternatives considered
A second DI token for the custody methods, instead of optional methods on
PaymentAdapter. Optional methods match the existingissueDepositAddress/parseWebhookidiom, and meanMockPaymentAdapterand every PSP adapter need no change at all.Keeping the currency and network sets as enums validated at boot. That catches a bad value earlier, but it is exactly what makes adding a currency a deploy.
supportsAssetrecovers the safety: the catalog write path asks the bound adapter first, so an admin cannot save a pair the vendor cannot serve, and the failure lands on the admin at config time instead of on a player at deposit time.Risks
The
wallet_assetmigration is additive and nothing existing reads the table, so current deposit and withdrawal behaviour is unchanged.Deleting a pair is blocked while players still hold that currency.
wallet_balanceis keyed by currency alone, with nonetworkcolumn, so that guard is necessarily currency-wide: removing one network of a currency someone holds is blocked even if their balance arrived over a different one. It fails safe, and widening it would mean puttingnetworkonwallet_balance, which is a bigger change than this PR should carry.wallet-assetis a new permission resource. The built-inadminrole gets it here, but any role whose levels are already stored will have no level for it and so no access until it is granted.The
(currency, network)key is deliberately immutable on update - renaming is a delete plus a create, so a vendor reference cannot be rewritten out from under a pending transaction.