Send Bridge cashout USDT to transfer deposit addresses#407
Conversation
islandbitcoin
left a comment
islandbitcoin
left a comment
There was a problem hiding this comment.
Reviewed the cashout USDT settlement + reconciliation (the core of this PR). The happy path is well-structured — recording the Bridge transferId before the IBEX send is the right ordering, and the send-failed orphan tracking is thorough. My main concern is a failure-mode correctness/funds-safety bug: a successful IBEX send whose response can't be parsed for a payout id is marked send_failed, and the reconciliation cron then deletes the Bridge transfer for send_failed rows assuming no funds moved — so an in-flight USDT send could be orphaned. That one's worth resolving before merge. (Your end-to-end run on MacMax covers the happy path but wouldn't exercise these edges.)
| const ibexPayoutId = ibexPayoutIdFromSendResponse(sendResult) | ||
| if (!ibexPayoutId) { | ||
| const error = new Error("IBEX crypto send did not return transaction id") | ||
| await BridgeAccountsRepo.updateWithdrawalSendFailed( |
There was a problem hiding this comment.
Funds-safety concern (worth fixing before merge). By this point sendCrypto has already succeeded — the USDT has left the user's wallet toward the Bridge deposit address. If ibexPayoutIdFromSendResponse can't find an id in the response, this marks the withdrawal send_failed and returns an error, i.e. a completed, irreversible fund movement is recorded as a failure. Combined with the reconciliation cron (reconciliation.ts:272, which deletes the Bridge transfer for send_failed rows assuming no funds moved), this could cancel a transfer whose USDT is already in flight → orphaned funds. After sendCrypto succeeds, this path should persist a success state (even with an unknown payout id) and raise a reconciliation alert for manual linking — never send_failed. The 5-candidate shape-guessing in ibexPayoutIdFromSendResponse makes an unparsed id a realistic case.
|
|
||
| if (withdrawal.status === "send_failed") { | ||
| try { | ||
| await BridgeApiClient.deleteTransfer(bridgeTransferId) |
There was a problem hiding this comment.
This deletes the Bridge transfer for every send_failed withdrawal on the premise that the IBEX send failed and no funds moved. That holds only if send_failed is set exclusively before/at a failed send — but index.ts:997 sets send_failed after a successful sendCrypto when the payout id can't be parsed, in which case money has moved and deleting the transfer would strand the in-flight USDT. Fixing the send_failed semantics in index.ts is the real remedy; alternatively gate this deleteTransfer on confirming no IBEX payout exists for the row.
| bridgeExternalAccountId: externalAccount.id, | ||
| bankName: externalAccount.bank_name ?? "", | ||
| accountNumberLast4: bridgeExternalAccountLast4(externalAccount), | ||
| status: "verified", |
There was a problem hiding this comment.
syncExternalAccountsFromBridge persists every account with active !== false as status: "verified". Since requestWithdrawal only allows verified accounts as cashout destinations, this is load-bearing — please confirm Bridge's active flag means payment-ready/verified, not merely active-but-pending (ownership/micro-deposit verification still outstanding). If active can be true pre-verification, a cashout could target an unverified bank account.
| if (persistResult instanceof Error) { | ||
| baseLogger.error( | ||
| { accountId, operation: "createExternalAccount", error: persistResult }, | ||
| "Failed to persist external account locally", |
There was a problem hiding this comment.
createExternalAccount logs but swallows a local-persist failure and still returns success with the Bridge account id — leaving Bridge and the local repo out of sync (Bridge has it, local doesn't). A later syncExternalAccountsFromBridge may paper over it, but returning 'success' for an account not locally recorded is misleading. Consider returning the error (or a clearly degraded result) so the client can retry.
| const result: ExternalAccountResult = { | ||
| bridgeExternalAccountId: externalAccount.id, | ||
| bankName: externalAccount.bank_name ?? "", | ||
| accountNumberLast4: ea.last_4 ?? "", |
There was a problem hiding this comment.
Inconsistent with syncExternalAccountsFromBridge, which uses the bridgeExternalAccountLast4 helper (account_number_last_4 ?? last_4). This manual path reads only ea.last_4, so a Bridge response using account_number_last_4 would yield an empty last-4. Reuse the helper in both paths.
| external_account_id: externalAccountId, | ||
| }, | ||
| features: { | ||
| allow_any_from_address: true, |
There was a problem hiding this comment.
from_address was dropped in favor of allow_any_from_address: true — Bridge will accept the USDT from any source (the pooled IBEX wallet rather than a per-user address). Assuming that's the intended custodial model; just confirming Bridge correlates the inbound deposit purely by the unique source_deposit_instructions.to_address, so funds from the shared IBEX wallet can't be misattributed across concurrent withdrawals.
|
Addressed in
Focused verification run:
|
…ccount The bridgeCreateExternalAccount resolver was registered in src/graphql/public/mutations.ts but the generated schema.graphql and the Apollo supergraph were never regenerated, so the field was absent from the SDL/supergraph (and would fail check:sdl in CI). Regenerate both so the mutation is exposed on the public API.
… vandana/bridge-cashout-onchain-settlement # Conflicts: # src/services/bridge/index.ts # src/services/mongoose/bridge-accounts.ts # src/services/mongoose/schema.ts # test/flash/unit/services/bridge/index.spec.ts
* feat(bridge): send cashout USDT to Bridge deposits * fix: show pending bridge cashouts in erp * fix: omit idempotency key when deleting bridge transfers * fix: preserve accepted bridge cashout sends * chore(graphql): regenerate SDL + supergraph for bridgeCreateExternalAccount The bridgeCreateExternalAccount resolver was registered in src/graphql/public/mutations.ts but the generated schema.graphql and the Apollo supergraph were never regenerated, so the field was absent from the SDL/supergraph (and would fail check:sdl in CI). Regenerate both so the mutation is exposed on the public API. --------- Co-authored-by: Vandana <forge@getflash.io>
2 participants
Summary
Verification
git diff --checkeslint --no-ignoreon the changed cashout/Bridge/IBEX filesjest --config ./test/flash/unit/jest.config.js test/flash/unit/graphql/error-map.spec.ts test/flash/unit/services/bridge/index.spec.ts test/flash/unit/services/bridge/reconciliation.spec.ts test/flash/unit/services/bridge/return-shapes.spec.ts test/flash/unit/services/ibex/client-usd-wallet.spec.ts --runInBandwith the main checkout.envsourced: 5 suites passed, 95 tests passed.