Skip to content

fix: address Devin Review financial-correctness findings from PR #31#34

Merged
devin-ai-integration[bot] merged 1 commit into
basefrom
devin/1783370450-devin-review-fixes-pr31
Jul 6, 2026
Merged

fix: address Devin Review financial-correctness findings from PR #31#34
devin-ai-integration[bot] merged 1 commit into
basefrom
devin/1783370450-devin-review-fixes-pr31

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Devin Review flagged 11 issues on the (already-merged) PR #31. Most are concrete financial-correctness bugs that are now live in base. This PR fixes them and verifies tsc, go build, and cargo check all pass.

TypeScript — transfer pipeline & escrow

  • Saga compensation voided a fabricated pending id. compensateFailedTransfer generated pendingId from Date.now()*999 + random, so voidPendingTransfer never targeted the hold created by the pipeline — the real TigerBeetle hold leaked. Fixed by deriving a deterministic id from the transfer UUID in both places:

    function pendingTransferIdFor(transferId: string): bigint {
      const hex = transferId.replace(/-/g, "").slice(0, 32).padEnd(32, "0");
      return BigInt(`0x${hex}`);
    }
    // pipeline: createPendingTransfer({ id: pendingTransferIdFor(id) ... })
    // compensation: voidPendingTransfer({ pendingId: pendingTransferIdFor(id) ... })
  • Escrow wallet debit was not refunded if the ledger hold failed. payDeposit/payInstallment debited the wallet (auto-committed), then called createPendingTransfer; a throw left the buyer short with no escrow record. Now the TB call is wrapped and the wallet debit is refunded before rethrowing.

  • Double TB hold on deposit. payDeposit created its own escrow-ledger hold and then called executeTransferPipeline, which created a second generic hold for the same funds. Added a skipLedger option to the pipeline; payDeposit sets it (sanctions/fraud/Kafka still run). payInstallment does not call the pipeline, so it was already single-hold.

Go ledger service (services/ledger-service/main.go)

  • Negative amounts. handleCreateTransfer now rejects amount <= 0 (a negative amount passed the available < amount balance check and reversed fund flow).
  • Post/void lookups fell through on non-ErrNoRows errors, proceeding with empty debitID/creditID/amountStr. Both handlers now fail-closed (500) on any DB error.
  • Non-atomic insert + balance updates. The transfer insert and the four balance UPDATEs now run inside a single sql.Tx (begin/commit/rollback), so a crash between them can't leave the ledger inconsistent.
  • Hardcoded default credentials. initDB now refuses to fall back to the remitflow:remitflow123@localhost default when NODE_ENV/GO_ENV is production.
  • Updated go.mod/go.sum (the rewritten service imports lib/pq; go build now succeeds).

Rust ledger service (services/rust-tigerbeetle-service/src/main.rs)

  • Negative / non-finite amounts. initiate_transfer rejects !amount.is_finite() || amount <= 0.0 (a negative f64 cast to u128 wraps to a huge value and bypasses the balance check).
  • Non-atomic insert + balance updates. New db_record_transfer_atomic runs the insert and both balance updates in one sqlx transaction; the previous code also only logged balance-update failures (transfer persisted, balances didn't) — now it fails closed.

Python reconciliation (services/python-tigerbeetle-reconciliation/main.py)

  • Float truncation manufactured phantom drift. int(float(x) * 1_000_000) truncates (0.07 * 1e6 == 69999.999…69999). Replaced with a _to_minor helper that rounds, so the reconciliation worker doesn't flag (or mask) false 1-unit drifts.

Verification

  • npx tsc --noEmit → 0 errors
  • go build ./... (ledger-service, Go 1.22) → success
  • cargo check (rust-tigerbeetle-service) → success (warnings only)
  • python3 -m ast parse of reconciliation main.py → OK
  • vitest run → 164 failed / 1804 passed — all failures are pre-existing environmental failures (no live PostgreSQL), ≤ the ~175 baseline; no new regressions.

Link to Devin session: https://app.devin.ai/sessions/64d054ae77da41e9a2b74d8593fa635c
Requested by: @munisp

- transferPipeline: derive deterministic pending transfer id from the
  transfer UUID so saga compensation voids the real hold (not a random id)
- propertyEscrow: refund wallet debit if the escrow ledger hold fails;
  skip the pipeline ledger step for deposits to avoid double-booking
- ledger-service (Go): reject non-positive amounts, fail-closed on any
  non-ErrNoRows lookup error in post/void, wrap insert+balance updates in
  a transaction, require DATABASE_URL in production
- rust-tigerbeetle-service: reject non-positive/non-finite amounts, record
  transfer + balance updates atomically in one DB transaction
- reconciliation (Python): round major->minor conversion to avoid float
  truncation manufacturing phantom drift

Co-Authored-By: Patrick Munis <pmunis@gmail.com>
@munisp munisp self-assigned this Jul 6, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author
Original prompt from Patrick

https://drive.google.com/file/d/14K-94cZoOVgiYCUA-VympU-4_8IBqv2d/view?usp=sharing
extract the contents of the archive. List all the features of the platform

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot merged commit 40569d2 into base Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant