Solution/vikasyadav - #128
Open
vikaszv96 wants to merge 10 commits into
Open
Conversation
Documentation-first design in DESIGN.md, then a layered implementation (handler -> service -> repository -> domain) covering: - idempotent POST /transfers via a claimed idempotency_records row, replayed from the linked transfer on duplicate requests - double-entry ledger: exactly 2 balanced rows per PROCESSED transfer, zero for FAILED - concurrency-safe transfers via SELECT ... FOR UPDATE on both wallets, locked in a deterministic order to avoid deadlocks - PENDING -> PROCESSED|FAILED state machine, all writes atomic in one DB transaction Includes unit tests (in-memory fakes, race detector) and integration tests against real Postgres proving no double-spend under concurrent load, plus docker-compose/Makefile for local dev.
A transfer between wallets with different currencies was silently processed as if the amounts were the same unit -- no conversion, no rejection. Add ErrCurrencyMismatch, checked once both wallets are locked (before any transfer/ledger row is written), mapped to 400. A rejected attempt releases the idempotency key like the wallet-not -found case, so a retry with valid wallets isn't blocked. Also corrects two stale DESIGN.md references to a lease/auto-reclaim idempotency mechanism that was replaced earlier with the simpler no-reclaim approach (409 + manual retry) but the doc wasn't updated to match at the time.
Required by ASSIGNMENT.md: tool used, how it was used, and the full prompt history for this session.
- requestHash: replace delimiter-joined preimage ("%s|%s|%d") with a
JSON-encoded struct. Wallet IDs are unconstrained strings, so a "|"
inside one could make two distinct (from, to) pairs hash identically
-- e.g. ("a|b","c") and ("a","b|c") both produced "a|b|c|100". JSON's
string escaping makes field boundaries unambiguous regardless of
content. Added a regression test asserting the specific collision
case now hashes differently.
- DESIGN.md: the §3 data-model snippet still listed response_code /
response_body columns on idempotency_records from an earlier design
that was simplified away (replay reconstructs from the linked
transfer row instead of a stored snapshot) -- the doc just wasn't
updated to match at the time. Removed the stale columns.
- Migrate: the schema_migrations EXISTS check ran outside the
transaction and the version INSERT had no ON CONFLICT, so two
instances starting concurrently could both see a migration as
unapplied and the second would fail its INSERT on the primary key,
crashing startup. Fixed by wrapping the whole migration run in a
Postgres session-level advisory lock held on a single dedicated
connection, so concurrent callers serialize instead of racing; added
ON CONFLICT DO NOTHING on the insert as defense in depth. Added an
integration test that runs Migrate from 8 concurrent goroutines
against a freshly reset schema and verifies every migration is
recorded exactly once.
CreateWallet took positional scalar args (id, openingBalance, currency) while CreateTransfer already used a CreateTransferInput struct -- an inconsistency, not two deliberately different patterns. The struct form matters more for CreateTransfer specifically (three adjacent string fields -- fromWalletId/toWalletId are swappable with zero compiler complaint under positional args), but there's no reason for the two creation paths in this codebase to disagree on style. Adds CreateWalletInput and updates the handler and all integration test call sites accordingly.
- transfer_handler: a replayed FAILED transfer incorrectly returned 200
instead of 422, because the status switch checked Replayed before
Status, so any replay short-circuited past the FAILED case regardless
of the transfer's actual outcome -- breaking idempotent HTTP
semantics (same request, different apparent result on retry).
Extracted the decision into transferStatusCode() and reordered so
FAILED always wins; added a handler-level unit test covering all
four (Status, Replayed) combinations. Verified live: an
insufficient-funds transfer and its replay both now return 422 with
identical bodies.
- TxManager.WithinTx: Rollback used the caller's ctx, which may already
be cancelled/timed out by the time an error path runs (e.g. the
client disconnected). Root-caused via a live probe against
pg_stat_activity: pgxpool actually discards a connection outright
after a failed operation rather than pooling it in a bad state, and
Postgres rolls back on connection close -- so this was never a
permanently-stuck transaction. The real, demonstrable bug is
narrower: Rollback(ctx) fails fast on a dead context and that
failure gets wrapped onto the real error as a misleading "(rollback
also failed: context canceled)" suffix, plus a connection gets
burned for no real reason. Fixed by giving Rollback its own bounded
background context. Added an integration test that deterministically
cancels ctx from inside the failing closure and asserts the returned
error is byte-for-byte the original -- confirmed it fails with the
old code and passes with the fix.
- idempotency_records allowed status='COMPLETED' with a NULL
transfer_id. Current code never does this, but nothing stopped a
future change from doing it by mistake, silently corrupting a key
(replay depends on transfer_id being valid once COMPLETED). Added a
CHECK constraint; added an integration test proving both the
rejection and that the normal PENDING-with-no-transfer-yet state
still works.
- DESIGN.md described the idempotency hash preimage as delimiter
-joined ("fromWalletId|toWalletId|amount"), which was already wrong
-- the actual implementation was fixed to JSON-encoding in an
earlier commit, but the doc wasn't updated to match at the time.
- README described the seeded demo wallet balances as "500.00" /
"200.00" / "0.00", implying decimal major-unit amounts, when the
actual seeded and API-returned values are integers in minor units
(50000 / 20000 / 0) -- ambiguous against the integer amounts used
everywhere else in the API.
- README's "cp .env.example .env" step did nothing: nothing in the
app or Makefile ever read a .env file. Rather than just removing the
now-misleading instruction, wired it up for real -- Makefile now
does -include .env + export, so PORT/DATABASE_URL overrides in .env
actually take effect. Verified live: PORT=9090 in .env made the
server bind :9090.
All fixes verified against real Postgres, several by deliberately
reverting each one and confirming its new test fails, then restoring
it and confirming the test passes again -- not just asserted.
There was a problem hiding this comment.
Pull request overview
This PR delivers a complete Go + Gin + Postgres implementation of the wallet transfer service, including idempotent POST /transfers, row-locking concurrency control to prevent double-spend, and a double-entry ledger, along with unit + integration test coverage and runnable local tooling.
Changes:
- Implemented handler/service/repository/domain layers for wallets, transfers, ledger entries, and idempotency records.
- Added Postgres schema + embedded migrations and a Docker Compose dev database, plus
make run/make test-*workflows. - Added unit tests (service/handler) and Postgres-backed integration/concurrency tests, plus a Postman collection and design/AI disclosure docs.
Reviewed changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration_test.go | Adds Postgres-backed integration tests covering end-to-end behavior, idempotency concurrency, locking, and migration safety. |
| README.md | Documents solution layout, run/test commands, and API surface. |
| postman/wallet-transfer-assignment.postman_collection.json | Provides a Postman collection covering happy paths and edge cases (idempotency, validation, concurrency scenarios). |
| Makefile | Adds local dev/test targets (db-up/down, run, unit/integration tests, lint, fmt-check). |
| internal/service/wallet_service.go | Implements wallet creation, retrieval, and ledger listing service APIs. |
| internal/service/transfer_service.go | Implements the core transfer workflow (idempotency, wallet locking order, ledger, balance updates). |
| internal/service/transfer_service_test.go | Adds unit tests for transfer workflow correctness and idempotency semantics. |
| internal/service/hash.go | Implements request hashing for idempotency payload consistency. |
| internal/service/hash_test.go | Adds tests guarding against delimiter-collision and ensuring deterministic hashing. |
| internal/service/fakes_test.go | Adds in-memory repository fakes to unit test service logic without Postgres. |
| internal/service/dto.go | Defines service-layer DTOs for transfers and wallets. |
| internal/router/router.go | Wires Gin routes and request logging middleware. |
| internal/repository/repository.go | Defines repository interfaces/ports and TxManager contract used by services. |
| internal/repository/postgres/wallet_repo.go | Implements wallet persistence, including SELECT ... FOR UPDATE and balance updates. |
| internal/repository/postgres/transfer_repo.go | Implements transfer persistence (create/update/get). |
| internal/repository/postgres/querier.go | Implements TxManager and ctx-based querier selection for tx vs pool execution. |
| internal/repository/postgres/ledger_repo.go | Implements ledger batch insert and per-wallet listing. |
| internal/repository/postgres/idempotency_repo.go | Implements idempotency claim/get/complete/release using Postgres constraints. |
| internal/handler/wallet_handler.go | Adds HTTP handlers for wallet create/get/ledger. |
| internal/handler/transfer_handler.go | Adds HTTP handlers for transfer create/get and status-code mapping for replays/failures. |
| internal/handler/transfer_handler_test.go | Tests HTTP status selection logic for transfer creation results. |
| internal/handler/errors.go | Centralizes error-to-HTTP mapping. |
| internal/handler/dto.go | Defines HTTP request/response DTOs and mapping helpers. |
| internal/domain/wallet.go | Introduces Wallet domain model and funds check helper. |
| internal/domain/transfer.go | Introduces Transfer model and state machine rules. |
| internal/domain/ledger.go | Introduces LedgerEntry model and double-entry constructor. |
| internal/domain/idempotency.go | Introduces idempotency record/status domain model. |
| internal/domain/errors.go | Adds domain sentinel errors used across layers. |
| internal/db/migrations/0002_seed_demo_wallets.up.sql | Seeds demo wallets for local runs. |
| internal/db/migrations/0002_seed_demo_wallets.down.sql | Removes seeded demo wallets on down migration. |
| internal/db/migrations/0001_init.up.sql | Creates core tables, constraints, and indexes (wallets/transfers/ledger/idempotency). |
| internal/db/migrations/0001_init.down.sql | Drops core tables. |
| internal/db/migrate.go | Adds embedded SQL migration runner with advisory-lock serialization. |
| internal/db/db.go | Adds Postgres pool connect + ping helper. |
| internal/config/config.go | Adds env-based configuration loader (PORT, DATABASE_URL). |
| go.sum | Adds dependency checksums. |
| go.mod | Defines module, Go version, and dependencies (gin, pgx, testify, uuid). |
| docker-compose.yml | Adds local Postgres service definition with healthcheck and persisted volume. |
| DESIGN.md | Adds pre-implementation design note describing schema/idempotency/concurrency strategy. |
| cmd/server/main.go | Adds application entrypoint (config, db connect/migrate, wiring, graceful shutdown). |
| AI_USAGE.md | Adds AI usage disclosure per submission requirements. |
| .gitignore | Updates ignores for env files, build artifacts, and test binaries. |
| .github/workflows/ci.yml | Updates CI Go version to 1.25. |
| .env.example | Adds example environment variables for local running. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+58
to
+66
| result, err := s.execute(ctx, in) | ||
| if err != nil { | ||
| // Nothing durable was recorded for this attempt (bad wallet, infra | ||
| // error, ...) -- free the key rather than wedging it on a response | ||
| // that was never written. | ||
| if releaseErr := s.idempotency.Release(ctx, in.IdempotencyKey); releaseErr != nil { | ||
| return nil, fmt.Errorf("%w (release also failed: %v)", err, releaseErr) | ||
| } | ||
| return nil, err |
Comment on lines
+61
to
+64
| if err := tx.Commit(ctx); err != nil { | ||
| return fmt.Errorf("commit tx: %w", err) | ||
| } | ||
| return nil |
…n retry CreateTransfer released the idempotency key on any error from execute(), including a commit that failed without confirming whether it landed server-side (e.g. connection drop after COMMIT was sent). Releasing in that case lets a retry re-run the transfer on top of a commit that already succeeded. WithinTx now runs Commit on its own short-lived context (like Rollback already does, so an unrelated caller-context cancellation can't masquerade as this) and wraps a failed commit in the new domain.ErrCommitOutcomeUnknown; CreateTransfer leaves the key PENDING instead of releasing it when it sees that error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Describe your solution briefly.
AI disclosure
Detail how you used AI to help with your submission (including the tools you used, how
you used them and what your prompts were).
Include these points in detail
Schema Design
Describe the tables, constraints, and indexes you introduced.
Idempotency Strategy
Explain how duplicate requests are handled safely.
Concurrency Strategy
Explain how you prevent race conditions and double spending.
How to Run
How to Test
Tradeoffs / Assumptions
Checklist