feat(denario): onboard Denario — partner wallet, Gold/Silver Polygon assets, and permanent referral alias#4209
Conversation
joshuakrueger-dfx
changed the title
|
Completed a full-diff conformance and logic review (1 round, 0 findings) before marking ready. Verified:
|
TaprootFreak
left a comment
TaprootFreak
left a comment
There was a problem hiding this comment.
I found three blocking correctness/data-safety issues. The asset rows currently activate pay-in detection without providing a processable pricing/manual path, and both migrations can delete or overwrite state they did not create. Please address the inline findings before merge.
| ("name", "uniqueName", "type", "blockchain", "category", "dexName", "chainId", "decimals", "description", | ||
| "buyable", "sellable", "cardBuyable", "cardSellable", "instantBuyable", "instantSellable", | ||
| "paymentEnabled", "refEnabled", "refundEnabled", "ikna", "personalIbanEnabled", "comingSoon") | ||
| SELECT 'DGC', 'Polygon/DGC', 'Token', 'Polygon', 'Public', 'DGC', '0xf7e2d612f1a0ce09ce9fc6fc0b59c7fd5b75042f', 8, 'Denario Gold Coin', |
There was a problem hiding this comment.
[P1] Prevent inert DGC/DSC deposits from getting stuck in the pay-in pipeline. Supplying chainId makes Alchemy map inbound Polygon transfers to these assets because the register strategy loads all blockchain assets, regardless of isActive. The registration flow then calls validateInput(), which requires an asset→CHF price; with no priceRuleId, pricing throws and the catch leaves the pay-in in CREATED, so the minute job retries and logs it indefinitely. This contradicts the PR's claim that recognition/forwarding is harmless. Either provide the complete priced/manual processing path or explicitly exclude these inert assets from pay-in registration, and add a DGC/DSC deposit regression test.
There was a problem hiding this comment.
Fixed. The register strategies now load getPayInAssets() (filters priceRule IS NOT NULL) instead of getAllBlockchainAssets(), so DGC/DSC (no price rule) are never mapped by Alchemy: an inbound Polygon transfer resolves to asset: null → CryptoInput.create sets FAILED, so it never enters the CREATED retry loop. updateFailedPayments got a matching SQL filter (asset: { priceRule: Not(IsNull()) }) plus a fail-closed in-loop guard, so the payment-quote cron can't resurrect an unpriced input either.
The swap was applied to every multi-asset register strategy (alchemy/evm base, citrea, cardano, zano, solana, tron, binance-pay, kucoin-pay, icp); native coin strategies keep their dedicated priced getters, and the balance/monitoring/asset-API callers of getAllBlockchainAssets are intentionally left unchanged (getPayInAssets uses its own cache key). Regression coverage in alchemy.strategy.spec.ts drives the real strategy + CryptoInput.create + updateFailedPayments for both the priced-ONDO happy path and the unpriced-token FAILED path.
This also surfaced a latent ONDO issue (chainId + buyable/sellable but no price rule → same stuck-pay-in) — fixed via the LinkOndoPriceRule migration and the seed row.
| const value = JSON.stringify(refKeys); | ||
|
|
||
| if (row) { | ||
| await queryRunner.query(`UPDATE "setting" SET "value" = $1, "updated" = NOW() WHERE "key" = $2`, [ |
There was a problem hiding this comment.
[P1] Preserve and audit the previous ref-keys state before overwriting it. Production already has this setting, so this replaces the serialized value without a durable before→after record, contrary to CONTRIBUTING.md's critical “Auditable mutations — no destructive overwrites” rule. Rollback ownership is also unsafe: if denario already pointed to the same ref, up() returns without changing anything, but down() still deletes that pre-existing alias; it also deletes a value changed after deployment. Persist an immutable prior-state/audit record before the update (fail closed if it cannot be written) and make down() restore only the change actually owned by this migration.
There was a problem hiding this comment.
Fixed. Added an append-only migration_audit_* store (immutability triggers rejecting UPDATE/DELETE/TRUNCATE, Apply/Rollback events tied by a composite self-FK with ON DELETE RESTRICT). up() writes the before→after event before mutating the ref-keys setting, in the same batch transaction (TypeORM default transaction: 'all', verified in config.ts), so a crash between the two rolls both back — no fail-open window.
down() now removes the alias only while it still equals the value we set (ownedRef); an admin re-point or re-add after deployment is detected and left intact (skippedAliasRepointed / skippedAliasMissing), and it restores the exact prior state (null vs pruned object) based on the recorded existed flag. Reapply after a rollback fails closed rather than clobbering a later change. Covered by add-denario-permanent-ref.migration.spec.ts, including the repoint and reapply cases.
| * @param {QueryRunner} queryRunner | ||
| */ | ||
| async down(queryRunner) { | ||
| await queryRunner.query(`DELETE FROM "asset" WHERE "uniqueName" IN ('Polygon/DGC', 'Polygon/DSC')`); |
There was a problem hiding this comment.
[P1] Do not delete rows that up() may not have created. Each up() insert is guarded by NOT EXISTS, but down() unconditionally deletes every matching asset and wallet (and wallet.name is not unique). In a pre-seeded environment or after migration-history recovery, an up→down cycle therefore removes pre-existing data. Either fail closed when matching rows already exist or persist exact migration ownership/prior state and revert only that state; add an up→down test covering the pre-existing-row case.
There was a problem hiding this comment.
Fixed. down() reads the ownership snapshot recorded by the audit store and deletes only rows whose full snapshot is still unchanged (assertOwnedRowIsUnchanged); pre-existing or reused rows are never touched, and it fails closed on an ambiguous/altered row. It also refuses to delete the wallet if any user.walletId still references it, and records the exact rollback (deleted asset/wallet ids) before the destructive writes. Double-down() is guarded (if (!applyAudit) return runs before the non-IF EXISTS DROP INDEX). Up→down and the pre-existing-row case are covered by add-denario-wallet-and-assets.migration.spec.ts with two apply/rollback cycles.
|
Thanks — addressed all three findings in 1. Inert DGC/DSC deposits stuck in the pay-in pipeline. Pay-in register strategies now recognize only priced assets via a new 2. 3. All local gates green (type-check, lint, 23 tests across the 3 new/updated specs). |
|
Follow-up review of
The referral controller change itself looks correct, and the DGC/DSC retry-loop cause is addressed. I reran the four targeted suites locally: 29 tests passed. The remaining blockers are test gaps and ownership/audit semantics, not failing CI. |
655c372 to
35d3c29
Compare
|
All three P1 findings are addressed (see the inline replies), and the branch has been rebased onto the current Conflict resolution worth flagging:
Local gate before push: One pre-deploy note (not a code issue): routing the register strategies through |
|
Added one dependency commit |
- pay-in register strategies recognize only priced assets (new AssetService.getPayInAssets); an unpriced token is no longer mapped into the register flow, where validateInput/pricing throws and would loop the pay-in in CREATED forever - AddDenarioPermanentRef: persist an immutable before-image of ref-keys before overwriting (fail closed in the migration transaction) and revert only the alias this migration set - AddDenarioWalletAndAssets: down() removes only still-inert assets and the wallet only while it has no owner or users - specs: unpriced-asset exclusion from recognition, ref-keys audit/ownership, wallet/asset up->down and ownership preservation
70f8f32 to
ca99196
Compare
… account id Guard up() and down() of the three Denario data migrations (AddDenarioPermanentRef, AddDenarioWalletAndAssets, LinkOndoPriceRule) with ENVIRONMENT === 'prd', matching the AddBankFrickCustodyAssets/ActivateBankFrick pattern. Without the guard these boot-blocking migrations throw on dev/loc/CI, where the Denario organization account and ONDO price rule do not exist, and reject DataSource init. The audit-store migration stays unguarded (pure DDL). Resolve the Denario referral target by the immutable prod user_data.id instead of the user-editable organizationName, so a user who completes Organization KYC under the name "Denario" cannot hijack the referral routing. The referral code itself is still read live; accountType/status/ambiguity guards remain as defense-in-depth. Specs force ENVIRONMENT=prd and prove the id pin excludes same-named impostor accounts.
…tion lint The `[DENARIO_USER_DATA_ID]` array literal tripped the migration-psql-check guard, whose MSSQL bracket-quoting detector flags any `[identifier]`. Use Array.of(...) — the single-element param idiom already used elsewhere in this migration — which is equivalent and avoids the false positive.
|
Rebased onto current Two rounds of independent review (conformity, logic, and a focused security/failure-mode pass) surfaced and fixed two issues:
All checks green. Follow-ups tracked separately: recommendation-skip and server-side referral→wallet coupling. |
2 participants
Overview
Bundles the full Denario onboarding into one PR:
Denarioto thewallettable (analogous to existing partner wallets, e.g. Cake Wallet).DGC,DSC) as inert, list-only assets.denarioalias on theref-keyssetting (original scope of this PR).1. Partner wallet
Migration
1784994200000-AddDenarioWalletAndAssets.jsinserts aDenariorow intowallet. Onlyname/displayNameare set; every compliance/behaviour column takes its conservative DB default (isKycClient=false,autoTradeApproval=false,usesDummyAddresses=false,displayFraudWarning=false,amlRules='0',buySpecificIbanEnabled=false) — kept conservative on purpose so the partner cannot bypass any check by default. Idempotent onname, reversible via the audit store (see §4):down()only removes rows this migration provably created.The partial unique index on
wallet.name = 'Denario'(declared on the entity) is created in all environments (CREATE UNIQUE INDEX IF NOT EXISTS), so migration-driven databases stay in sync with the entity schema andmigration:generatestays clean.down()intentionally does not drop it — it is entity schema and carries no data.2. Assets (Polygon ERC-20)
Added in the same migration and mirrored into
migration/seed/asset.csv(ids 413/414), values verified on-chain (decimals()/symbol()/name()):0xf7e2…042f0x5d4e…c7e7Contract addresses independently verified against Denario's official token pages. Backing: 1 DGC = 1 oz gold, 1 DSC = 1 oz silver (999.9), vaulted in Switzerland.
Both are inert, list-only:
type=Token,blockchain=Polygon,category=Public, nopriceRuleId, and every trade/payment flagfalse→isActive=false. Consequences:priceRuleId).financialTypeis intentionally leftnull(no precious-metal type exists yet) — set it when trading is enabled.Deliberately left empty (correct for an inert asset — not guessed)
financialType: no precious-metal type exists in the repo (values in use:Other/USD/EUR/BTC/CHF/DEPS/FPS). It drives fee-matching (fee.entity) and balance grouping (log-job) — introducing a new bucket is a tax/reporting classification decision for DFX/DAS, so leftnull.approxPriceUsd/Chf/Eur: would hard-code a decaying gold/silver snapshot; only read for active/custody/traded assets.sortOrder: display-order only (e.g.dEURO/SANDalso leave it empty).Follow-ups when trading is enabled (verified inputs, need DFX/DAS decision)
DSC/USDCpool on Polygon (0x6722e2405a3b6ee5bf862112a143cf7228ee6c18). Denario also publishes Proof-of-Reserve oracles (DSC PoR0xb507a787cda02d6086f101d6067e9c35d58f1fc5). Attach aPriceRule(DEX/oracle source) then flip the buy/sell flags.financialTypeper token once the metal asset class is decided.isKycClient/amlRules).3. Referral alias (original scope)
denarioalias to the existingref-keyssetting (migration1784994100000-AddDenarioPermanentRef.js, prd-only)user_data.id = 408808(name-based resolution was dropped after review — an editableorganizationNamewould be hijackable)/v1/app?code=<alias>to persist the resolved referral code instead of the unresolved alias, matching/v1/app/:app(includes a prototype-pollution guard on the alias lookup)Stable link after deployment
https://api.dfx.swiss/v1/app/services?code=denario(generic redirect:/v1/app?code=denario)denarioalias unless theref-keyssetting is set manually there.Deployment precondition (prd)
user_data.id 408808must be an active organization account with exactly one distinct active referral code across its active users. The migration intentionally aborts (and thereby blocks the boot migration run) if this invariant is not met — see the Ops checklist below, which must be confirmed before merge. It also refuses to overwrite an existingdenarioalias that points elsewhere.4. Migration audit store
1784994000000-CreateMigrationAuditStore.js(runs in all environments) creates two append-only tables (migration_audit_lock,migration_audit_event) with DB triggers rejectingUPDATE/DELETE/TRUNCATE, check constraints, and an apply↔rollback self-FK (ON DELETE RESTRICT). The data migrations write a before→after event before mutating (same migration transaction — TypeORM defaulttransaction: 'all'), and theirdown()paths only revert state whose full snapshot still matches whatup()recorded: pre-existing rows, admin re-points and post-deploy changes are detected and left intact, double-down()and re-apply are guarded. Rollbacks append events instead of deleting audit rows.5. Pay-in hardening (review round 1 finding, generalised)
Register strategies previously mapped inbound transfers against all blockchain assets; a token with
chainIdbut nopriceRulethen produced aCREATEDcrypto input that the minute jobs re-validated forever (price lookup throws → endless retry + error log). Now:AssetService.getPayInAssets()(priceRule IS NOT NULL, own cache key).getAllBlockchainAssetsand its balance/monitoring/asset-API callers are intentionally unchanged.asset: null→CryptoInputis persistedFAILED(terminal, no retry loop) and triggers an ERROR_MONITORING notification so the funds on the deposit address are visible to Ops.updateFailedPaymentsfilters unpriced assets in SQL and logs a warning in its fail-closed in-loop guard.1784994300000-LinkOndoPriceRule.js(prd-only) links the existingEthereum/ONDOasset (buyable/sellable since its seed, but priceless → every deposit stuck) to the CoinGeckoondo-financeprice rule, fail-closed on missing/ambiguous rule or asset.Declared behaviour change for existing data: the seed lists further assets with
chainIdbut no price rule (Ethereum/BGB, Ethereum/FOX, Ethereum/EURC, Arbitrum/EURC, DFI on Ethereum/Arbitrum/BSC). New deposits of these tokens are no longer mapped to the asset; they persist asFAILEDwithout asset assignment (previously: stuckCREATEDwith asset). Existing stuckCREATEDrows are not migrated — see Ops checklist. Native-coin strategies (bitcoin/lightning/monero/firo) keep their dedicated coin getters without a price-rule filter;Firo/FIROhas no price rule in the seed, so the old stuck-CREATEDmode still applies there — follow-up candidate, out of scope here.Ops checklist (before merge / deploy — read-only)
All four migrations run at boot (
migrationsRun); any failed precondition on prd is a crash-loop, by design (fail closed). Verify on prd:SELECT COUNT(DISTINCT u.ref) FROM "user" u WHERE u."userDataId" = 408808 AND u.status = 'Active' AND u.ref IS NOT NULL;→ must be exactly1, anduser_data 408808is the intended active Denario organization account.1784994300000-LinkOndoPriceRule.js(CoinGecko /ondo-finance), and exactly oneEthereum/ONDOasset withpriceRuleId IS NULL(or already linked to that rule — then the migration no-ops).walletrow withname = 'Denario', and if present, only conservative defaults (otherwise the migration fails closed).SELECT ci.id, a."uniqueName", ci.created FROM crypto_input ci JOIN asset a ON a.id = ci."assetId" WHERE ci.status = 'Created' AND a."priceRuleId" IS NULL;— ONDO rows will be processed retroactively once the price rule is linked (buy-crypto/AML/forwarding of old deposits — confirm this is wanted); rows of other unpriced assets keep retrying every minute until a price rule is linked or they are reconciled manually.Review rounds
ref-keysoverwrite, non-owneddown()deletes — all three addressed (§4/§5) with regression tests (repoint, pre-existing-row beforeup(), two apply/rollback cycles).user_data.idinstead of name.down()no longer drops it), monitoring alert for failed unassigned pay-ins, warn-log in theupdateFailedPaymentsguard, PR body corrected (pay-in note, deployment precondition, seed ids).Tests & checks
up()/down()SQL againstpg-mem(simplified schemas; pg-mem cannot execute the audit-store triggers — their guarantees are enforced by the DDL itself and reviewed, not covered by tests). Ownership scenarios covered: pre-existing rows created beforeup(), admin repoint, double-down(), re-apply, two full cycles.AlchemyStrategy+CryptoInput.create+updateFailedPaymentsfor the priced happy path, the unpricedFAILEDpath, and the new monitoring alert.npm run type-check,npm run lint,npm run format:check.