Solution (Mohit Kumar): wallet transfer service - idempotency, double-entry ledger, concurrency safety - #129
Open
Mohitshalout wants to merge 2 commits into
Open
Conversation
…rrency safety - POST /transfers with double-entry ledger and PENDING/PROCESSED/FAILED states - Idempotency via unique-key insert; safe under retries and duplicate delivery - Row-level locking (FOR UPDATE, ordered) prevents overdraw and deadlocks - DB constraints enforce invariants (non-negative balance, one entry per side) - Layered handlers/services/repositories/domain - Alembic migration, docker-compose, pytest suite (transfers, idempotency, concurrency)
There was a problem hiding this comment.
Pull request overview
Implements a wallet-to-wallet transfer service using FastAPI + SQLAlchemy/PostgreSQL, with idempotent transfer creation, a double-entry ledger, and concurrency safety via DB constraints and SELECT ... FOR UPDATE row locking.
Changes:
- Added core service/repository layers for transfer execution, idempotency recording, and double-entry ledger posting.
- Added initial Alembic migration and local run artifacts (Docker/Docker Compose, seed script, README/docs).
- Added pytest coverage for happy path, validation, idempotency replay/conflict, and concurrent debit safety.
Reviewed changes
Copilot reviewed 28 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_transfers.py | Happy-path + validation tests for transfer API. |
| tests/test_idempotency.py | Tests idempotent replay, conflict behavior, and idempotency for failed transfers. |
| tests/test_concurrency.py | Threaded test to validate no overdraw under concurrent debits. |
| tests/conftest.py | Test DB schema setup, table cleanup, and helper fixtures. |
| tests/init.py | Marks tests as a package. |
| scripts/seed.py | Seeds example wallets for local testing. |
| requirements.txt | Pins runtime + test dependencies. |
| README.md | Documents run instructions, API behavior, and correctness approach. |
| pytest.ini | Configures pytest discovery. |
| migrations/versions/0001_initial.py | Initial DB schema for wallets/transfers/ledger/idempotency. |
| migrations/script.py.mako | Alembic revision template. |
| migrations/env.py | Alembic environment wiring to app metadata and DATABASE_URL. |
| docs/DESIGN.md | Design rationale for idempotency, locking, and ledger invariants. |
| Dockerfile | Container build/run with migrations + uvicorn startup. |
| docker-compose.yml | Local orchestration for app + Postgres. |
| app/services/transfer_service.py | Implements idempotency workflow and transactional transfer execution. |
| app/services/init.py | Exposes services package. |
| app/repositories/wallet.py | Wallet retrieval + deterministic FOR UPDATE locking helper. |
| app/repositories/transfer.py | Transfer persistence and status transitions. |
| app/repositories/ledger.py | Double-entry ledger write and query helpers. |
| app/repositories/idempotency.py | Idempotency record persistence helpers. |
| app/repositories/init.py | Exposes repositories package. |
| app/main.py | FastAPI app wiring and health endpoint. |
| app/domain/errors.py | Domain errors mapped to API responses. |
| app/domain/enums.py | Transfer/ledger/idempotency enums. |
| app/domain/init.py | Exposes domain package. |
| app/db/models.py | SQLAlchemy models for all tables and constraints. |
| app/db/base.py | Engine/session setup and FastAPI session dependency. |
| app/db/init.py | Exposes db package. |
| app/config.py | Pydantic settings for DB URL and app name. |
| app/api/schemas.py | Pydantic request/response models and request validation. |
| app/api/routes.py | Transfer + wallet read endpoints. |
| app/api/errors.py | Exception handlers for domain + validation errors. |
| app/api/init.py | Exposes api package. |
| app/init.py | Exposes app package. |
| alembic.ini | Alembic configuration. |
| .gitignore | Updates ignore patterns for Python tooling and envs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+24
to
+25
| ledger = client.get(f"/transfers/{body['transferId']}") | ||
| assert ledger.status_code == 200 |
Comment on lines
+22
to
+27
| def mark(session: Session, transfer: Transfer, status: TransferStatus) -> None: | ||
| """Only PENDING transfers may transition; guards against duplicate processing.""" | ||
| if transfer.status != TransferStatus.PENDING.value: | ||
| return | ||
| transfer.status = status.value | ||
| session.flush() |
Comment on lines
+47
to
+48
| `201` on success, `200` on an idempotent replay: | ||
|
|
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.
Overview
A wallet-to-wallet transfer service with idempotent requests, a double-entry ledger, and safe concurrent execution. Correctness is enforced at the database layer (constraints + row locks) rather than in application code. Design rationale is in
docs/DESIGN.md; the design commit precedes the implementation commit, per the documentation-first workflow.Stack: FastAPI, SQLAlchemy 2.0, Alembic, PostgreSQL, pytest.
Database schema
Four tables -
wallets,transfers,ledger_entries,idempotency_records. Constraints carry the guarantees:CHECK (balance >= 0)- an overdraw is rejected by the database.idempotency_keyPRIMARY KEY - duplicate detection is a uniqueness guarantee, not a racy read-then-write.UNIQUE (transfer_id, type)on the ledger - a transfer can never be double-posted.CHECK (from_wallet <> to_wallet),amount > 0- invalid transfers cannot be persisted.Idempotency strategy
The idempotency key is stored with a hash of the request body. The insert of the idempotency record is itself the concurrency gate, so a retried request can never create a second transfer:
The idempotency record and the transfer commit in the same transaction, so neither can exist without the other.
Concurrency handling
Each transfer runs in one transaction:
SELECT ... FOR UPDATElocks both wallets, ordered by id so A->B and B->A cannot deadlock.CHECK (balance >= 0)constraint is a second line of defence.WHERE status = 'PENDING'.Isolation is READ COMMITTED; explicit row locks over the mutated rows make a higher level unnecessary.
Assumptions / tradeoffs
Testing
pytest against PostgreSQL (the concurrency test needs real row locks): happy path, idempotency replay + conflict, insufficient funds, validation, and a 10-thread concurrency test asserting no double-spend. Verified end-to-end via docker compose against real PostgreSQL and Alembic migrations.
AI usage
Built with the help of Claude Code (AI assistant), used to draft the design document, scaffold boilerplate, and cross-check test coverage. All design decisions - idempotency via unique-insert, deterministic lock ordering, and pushing invariants into database constraints - were directed and reviewed by me, and I can walk through any part of the implementation.