Repository navigation
fix(withdraw): gate balance increment on event INSERT rowcount to prevent double-counting - #1
Open
freebuff-web[bot] wants to merge 287 commits into
Open
freebuff-web[bot] wants to merge 287 commits into
freebuff-web[bot] wants to merge 287 commits into
Conversation
…t#1271) Extract the duplicated "paginated stream events by sender/recipient wallet" query logic from GET /v1/events (events.routes.ts) and GET /v1/users/:publicKey/events (user.controller.ts) into a single listEventsForWallet helper in the new backend/src/repositories/streamEvent.repository.ts. The shared repository also exposes parseEventTypeFilter, resolveEventsPageSize, and resolveEventsOffset, and now supports the union of both endpoints' prior feature sets: comma-separated `type` filtering (previously only on /v1/events) and an optional includeStream flag that embeds the related stream (previously always-on and only on /v1/users/:publicKey/events). Each endpoint keeps its historical includeStream default (false for /v1/events, true for /v1/users/:publicKey/events) so existing consumers see no response-shape change. stream.controller.ts is updated to import DEFAULT_EVENTS_PAGE_SIZE / MAX_EVENTS_PAGE_SIZE from the new repository module instead of from events.routes.ts, where they no longer live. Adds streamEvent.repository.test.ts covering the new helpers, and extends user.controller.test.ts and events-list.test.ts with includeStream/type-filter coverage plus parity tests asserting both endpoints build identical where-clauses via the shared helper.
…#1281) Both vitest.config.ts and vitest.config.mts existed simultaneously, making it unclear which one vitest run actually resolves. Verified empirically by adding a distinguishing console.error marker to the top of each config and running `npx vitest run`: only the marker from vitest.config.ts printed, confirming Vite/Vitest resolves the .ts file over the .mts one in this repo (Vitest 3.2.7). This also matches the codebase: setupFiles './src/__tests__/setup.ts' (referenced by the .ts config) and the actual test layout under src/__tests__/** and colocated across src/**, both matched by the .ts config's include pattern. The .mts config's setupFiles pointed at './src/test-setup.ts', a byte-identical duplicate of the same setup file, now orphaned and removed. Kept: vitest.config.ts (happy-dom environment, coverage thresholds, include pattern, resolve.alias['@']). Removed: vitest.config.mts (jsdom environment, @vitejs/plugin-react plugin, no coverage thresholds) and the now-unreferenced src/test-setup.ts duplicate. The @vitejs/plugin-react plugin from the removed config was not carried over: the full test suite (32 files, 286 tests) already passes without it, since Next.js/Vite's default esbuild JSX transform handles .tsx test files fine for this project's happy-dom-based unit tests. Added a small regression-guard test asserting only one vitest.config.* file exists in frontend/, so this ambiguity can't silently reappear.
Copy-address buttons called navigator.clipboard.writeText directly with no .then/.catch, so a denied clipboard permission (or an unavailable Clipboard API) failed silently with no feedback to the user. Add a shared copyToClipboard(text, options?) helper in lib/clipboard.ts that awaits the write and shows a toast.success or toast.error accordingly. It never throws, so callers don't need their own try/catch. Use it at every copy-address call site: - settings page: wallet address copy and contract address copy - WalletButton dropdown: copy wallet address - StreamDetailsModal: copy recipient address (also adds a brief copied-state icon swap, matching the existing wallet address pattern) Add unit tests for the shared util and render tests for each call site that simulate a rejected clipboard write and assert an error toast is shown instead of failing silently.
…mplate deletion Replace the native browser window.confirm dialog in handleDeleteTemplate with the app's styled confirmation pattern. Adds a reusable ConfirmModal component generalizing the CancelConfirmModal design so any destructive action uses a consistent, styled confirmation instead of an unstyleable native prompt. Closes LabsCrypt#1229 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…ent poll Adds an integration test that exercises the race condition described in Functional Edge Case #19 (issue LabsCrypt#1293): resetIndexer and replayFromLedger bypass the SorobanEventWorker's batchMutex, so their DB cursor writes can be overwritten by a concurrent poll's stale upsert. The test deliberately fails against current code and will pass once the race is fixed (the poll must re-read the cursor before writing it back, or resetIndexer/replayFromLedger must go through the batchMutex). 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…vent double-counting
The withdraw handler atomically incremented withdrawnAmount on every request,
regardless of whether the corresponding StreamEvent already existed. Because
sorobanWithdraw returns a deterministic txHash ('simulated-withdraw-' +
streamId), retries and concurrent duplicate requests would each re-increment
the balance — a real fund-accounting bug.
Move the StreamEvent creation inside the $transaction using a conditional
INSERT with WHERE NOT EXISTS + RETURNING. The INSERT's rowcount (1 = new,
0 = duplicate) gates the balance UPDATE, guaranteeing exactly-once accounting
per claimable window. This is race-condition-safe because the unique
constraint on (transactionHash, eventType) prevents duplicate events at the
database level, and the rowcount is evaluated within the same transaction.
Updates existing tests in eventRace.test.ts and withdraw.handler.test.ts,
and adds two new integration tests proving idempotent withdrawal behavior.
Closes LabsCrypt#1216
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
…ypt#1198) - Add LiveValue throttled aria-live region for ticking balances (IncomingStreamCard claimable, stream-details StatCard claimable) so screen readers announce on user query rather than per frame. - Respect prefers-reduced-motion in useStreamingAmount: compute accrued value once instead of running the rAF loop. - Global :focus-visible outline token for keyboard users (2.4.7). - Fix dark-mode contrast for ticker labels, badges, and modal copy (slate/emerald/amber/indigo utilities get dark: variants). - give the clipboard icon button in StreamDetailsModal an accessible name. - Replace invalid dl/dt/dd layout in IncomingStreamCard with div-based structure to satisfy axe definition-list/dlitem rules. - Add axe-core a11y suite (a11y.test.tsx) asserting zero critical/serious violations across IncomingStreamCard, StreamDetailsModal, TopUpModal, WalletModal; unit tests for LiveValue throttling.
…ifecycle (LabsCrypt#1199) Adds a Playwright E2E harness for the FlowFi frontend: - Global setup generates self-signed TLS certs for the local Soroban RPC mock. - A combined mock server (REST + SSE + Soroban RPC over HTTPS) supports the wallet connection, stream creation, and stream detail endpoints. - A window-level Freighter postMessage mock plus a persisted non-mocked wallet session enables full create/withdraw flows against the mocked chain. - Tests run against Turbopack dev (port 3101) with mocked API/RPC on 3100/3102 in chromium and firefox. Also pins the Turbopack workspace root so stray package-lock files outside the repo do not break the dev server.
The merge with main renamed indexerService.ts to indexer.service.ts and reduced indexerService.ts to a re-export, but dropped the dryRun preview functions (previewReset, previewReplay and their interfaces) that admin.routes.ts imports. Re-add them to indexer.service.ts so both the npm run build and docker build CI steps compile.
The dryRun=true preview paths in admin.routes.ts were implemented without tests. Add six cases: preview responses for reset and replay, non-true dryRun values falling through to the real mutation, and preview failures returning 500 without mutating state.
The admin indexer reset/replay routes document the dryRun query parameter in their annotations, but the generated artifacts were never updated, so the CI drift check (ci.yml: OpenAPI spec & API types drift check) failed. Regenerate backend/swagger/flowfi.openapi.json and frontend/src/lib/api-types.generated.ts to match.
Remove duplicate theme management from useSettings.ts and settings-content.tsx, both of which independently read/wrote the "flowfi-theme" localStorage key and manually toggled the dark class on <html> without coordinating with each other or with next-themes' ThemeProvider. This caused toggling theme in one surface (e.g. the settings page) to not reflect in another (e.g. ModeToggle in the navbar) until a page reload. All three theme surfaces now use next-themes' useTheme() as the single source of truth: - layout.tsx's ThemeProvider (unchanged — already the provider) - settings-content.tsx now calls useTheme() from next-themes instead of managing its own localStorage read/write and class toggling - useSettings.ts no longer manages theme at all; consumers needing theme use useTheme() from next-themes instead Non-theme settings (displayCurrency, amountFormat, decimalPlaces) and their utilities (formatAmountWithPreference, getDecimalPlaces, etc.) are preserved untouched. Closes LabsCrypt#1262 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
- backend/src/routes/health.routes.ts: define indexerLagDegraded/indexerFailureDegraded/redisStatus/sorobanRpcOk (was referencing undefined names) - backend/src/services/indexerService.ts: add previewReset/previewReplay + runExclusive + requestContext correlation (required by admin.routes) - backend/src/services/sorobanService.ts: restore rpcPool/executeRpc/pollTransactionStatus/getServer fixes for stellar-sdk v17 (ScVal union, resources property, bigint streamId) - backend/src/workers/soroban-event-worker.ts: drop unused rpcPool import - backend/src/middleware/admin-rate-limiter.middleware.ts: use ipKeyGenerator for express-rate-limit v8 IPv6 compliance - backend/package.json: pin @types/pg 8.20.0 to match @prisma/adapter-pg; tests updated for new SDK types - contracts/stream_contract/src/test.rs: remove stray 74 lines breaking cargo fmt/clippy - package.json: add next 16.3.3 to root for eslint-config-next parser resolution; frontend tests/components fixed - backend/swagger/flowfi.openapi.json + frontend api-types: regen for /v1/tokens + /metrics drift check
Frontend (from upstream/main PR LabsCrypt#1565, was breaking merge CI): - sync transaction-feedback.ts wrapper + TransactionTracker/CancelConfirmModal/TopUpModal/stream-details-content impl - tests mock @/lib/transaction-feedback and assert on transactionSuccessToast instead of toast.success - stream-details-content test mocks useTokenPrice, adds 3rd receipt fetch mock, fixes duplicate stream #42 and Amount is required message Contracts (pre-existing clippy failures): - errors.rs: add ArithmeticOverflow=34, StreamStillActive=35, StreamNotActive=36 (used by lib.rs/test.rs) - lib.rs: add missing cliff_time to step/hybrid/allowance constructors; remove stale acceptance_tests mod (references removed BatchStreamInput/batch_create/transfer_recipient APIs) - test.rs: add arbiter/dispute_status/is_allowance_based to Stream literals + DisputeStatus import
- storage.rs: add missing cliff_time to upgrade_legacy_stream, bump STREAM_FIELD_COUNT 16->17 (Stream now has 17 fields) - lib.rs: sync upstream fee refactor (collect_fee tuple + transfer_fee), add cliff_time to step/hybrid/allowance constructors, replace try_invoke allowance check with token_client.allowance (fixes E0308 Val vs Address), prefix unused _token_address, drop stale acceptance_tests mod (references removed BatchStreamInput/batch APIs)
Resolve lib.rs allowance-check conflict in favor of token_client.allowance (fixes E0308 Val vs Address that fails clippy on main). Take upstream: ci frontend coverage upload, globals.css, settings-content, create-stream-content, IncomingStreams, dashboard-view, StreamCreationWizard, reentrancy test.
clippy -D warnings denies unused must-use Result at lib.rs:1403
- stream_record_is_current_shape 13->17 (Stream gained cliff_time/arbiter/dispute/allowance) - remove test_fee_transfer_observes_persisted_stream_on_create_and_top_up: Soroban forbids re-entry, token cannot callback get_stream during transfer
Backend hardening: DB pool gauges, dead-letter alerts, body size limits, token metadata caching
… conflicts - DashboardIncoming: combine shell-controlled batch-claim (c shortcut, onOpenBatchClaim + selectedStreamId) with internal drawer fallback (onBatchClaimSuccess) - dashboard-view: pass both onOpenBatchClaim and onBatchClaimSuccess + selectedStreamId to incoming tab - keeps upstream backend/contracts/frontend fixes + package.json next dep for eslint
…elope Restore the withdraw handler idempotency tests to match the constitent error envelope produced by sendApiError (code + message) and the PR's 'gates balance increment on event INSERT rowcount' integration test (recover a missing firstClaimed declaration and replace the response-amount double-counting assertion, which can never be less than the full claimable amount, with an assertion that the DB balance is unchanged on a duplicate). Closes LabsCrypt#1216 LabsCrypt#1410 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The Prisma schema defined IndexerDeadLetterEvent twice (lines 71 and 164),
causing to fail with P1012 ('model already exists'). The
second definition (with eventType/txHash/ledgerSequence/payload) supersedes
the first, so the duplicate is removed while keeping the complete model
so the Prisma client and npm run build work.
This unblocks the Backend CI and Backend Docker Image CI build steps.
Co-Authored-By: Codebuff <noreply@codebuff.com>
…tions The main->Flowfi merge left duplicate declarations in pg-pool.ts and sorobanService.ts, and its package-lock.json conflict resolution dropped required entries (e.g. @asamuzakjp/dom-selector, @humanwhocodes/retry), breaking `npm ci` across every workflow. Revert the duplicated definitions to their main versions and rebuild the lockfile from the pre-merge branch lock reconciled with the merged package.json so installs and builds pass. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
[Backend] claimable.service.ts cache key does not include token addre…
…uess feat: SDK passphrase validation, troubleshooting docs, Rust pre-commit checks, dashboard shortcuts
# Conflicts: # frontend/src/components/stream-creation/ScheduleStep.tsx # frontend/src/components/stream-creation/StreamCreationWizard.tsx # frontend/src/components/stream-creation/__tests__/StreamCreationWizard.test.tsx # frontend/src/lib/stream-validation.ts
…-table-preferences feat(frontend): persist table column visibility and sorting preferences (LabsCrypt#1504)
…validation fix(frontend): validate Stellar public key checksum in stream creation wizard (LabsCrypt#1506)
# Conflicts: # frontend/src/components/stream-creation/__tests__/StreamCreationWizard.test.tsx
…balance-before-submit fix(frontend): check wallet token balance before stream creation confirmation (LabsCrypt#1507)
Merging main into this branch unioned both sides of every conflicting file instead of combining them, so the tree did not build: `lib.rs` had an unclosed `match`, `errors.rs`/`test.rs` declared duplicate variants and fields, `sorobanService.ts` and `stream-simulation.test.ts` each contained two copies of a function, `indexerService.ts`/`pg-pool.ts`/`health.routes.ts` duplicated declarations, and several frontend files mixed both revisions (duplicate JSX props, hooks and imports). The Docker and preview jobs also failed on `EOVERRIDE` because the backend override pinned `@types/pg@8.23.1` while main had added it as a direct `8.20.0` devDependency. Take main's revision where it supersedes the branch's reconciliation (sorobanService, pg-pool, health routes, and the matching test suites), keep the branch's intentional fixes (actual-toast helper, useSyncExternalStore network provider, `nativeToScVal` vec element type, webhook fetch cancellation), and align the `@types/pg` override with the direct dependency so the standalone container install resolves a single copy again. cargo fmt/clippy/test, the wasm release build, backend build + vitest + OpenAPI drift, and frontend lint/tsc/vitest are all green. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
… 70% The merge dropped `mod acceptance_tests`, so the seven tests that exercise `batch_create_streams`, `create_stream_with_cliff`, `transfer_recipient` and `extend_stream_ttl` stopped running and contract coverage fell to 67.85%, below the 70% tarpaulin gate. The crate compiles and the whole 230-test suite passes with the module restored, so re-declare it. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…rk-recovery-1468 feat(backend): ledger reorg & fork recovery engine with idempotent rollback snapshots (LabsCrypt#1468)
🔍 Preview environment ready — PR #1 (
|
| Service | URL |
|---|---|
| Frontend preview | https://pr-1.flowfi-preview.local |
| Backend preview | https://flowfi-pr-1.fly.dev |
| Backend health | https://flowfi-pr-1.fly.dev/health |
Database: ephemeral flowfi_preview_pr_1 migrated + seeded with demo streams/users.
Closing this PR deallocates all preview resources automatically.
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
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.
Problem
The withdraw handler (
backend/src/routes/v1/streams/withdraw.ts) atomically incrementedwithdrawnAmounton every request, regardless of whether the correspondingStreamEventalready existed. BecausesorobanWithdrawreturns a deterministictxHash('simulated-withdraw-' + streamId), retries and concurrent duplicate requests would each re-increment the balance — a real fund-accounting bug.Root cause: The
StreamEventupsert (which was idempotent via the unique constraint on(transactionHash, eventType)) happened after the blind balance increment. A retry would:withdrawnAmountagain (no guard)This means
withdrawnAmountwas double-counted on every retry.Solution
Move the
StreamEventcreation inside the$transactionusing a conditionalINSERT ... WHERE NOT EXISTS ... RETURNING. The INSERT's rowcount (1= new event,0= duplicate) gates the balanceUPDATE, guaranteeing exactly-once accounting per claimable window.This is race-condition-safe because:
(transactionHash, eventType)prevents duplicate events at the database levelFiles Changed
backend/src/routes/v1/streams/withdraw.ts— Core fix: moved event creation inside transaction, gated balance increment on INSERT rowcountbackend/tests/withdraw.handler.test.ts— Updated existing test + added new idempotency testbackend/tests/eventRace.test.ts— Updated race condition test to verify new INSERT-based approachbackend/tests/integration/streams/withdraw.test.ts— Updated existing tests + added two new integration tests proving idempotent withdrawalTests Added
withdraw.handler.test.ts): "should not increment withdrawnAmount when the event already exists (idempotent)" — proves the balance stays at 150 (not 200) when a duplicate request arriveseventRace.test.ts): "skips balance increment when event already exists (worker race condition)": proves that when the worker creates the event first, the handler's balance increment is skippedValidation
tsc --noEmit)Closes LabsCrypt#1216
🤖 Generated with Codebuff
Co-Authored-By: Codebuff noreply@codebuff.com