Solution for Wallet-Transfer-Assignment - Abhishek Badgujar - #131
Open
AbhishekBadgujar wants to merge 5 commits into
Open
Solution for Wallet-Transfer-Assignment - Abhishek Badgujar#131AbhishekBadgujar wants to merge 5 commits into
AbhishekBadgujar wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an initial design document for a wallet-to-wallet transfer service, covering API semantics, data model, idempotency approach, and concurrency strategy.
Changes:
- Documented requirements (exactly-once, double-entry ledger, concurrency, transfer state machine)
- Proposed API contract and error codes for
POST /transfers - Outlined database tables/constraints and transaction/locking strategy
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…s scenarios with idempotency, race condition, deadlock . Seems to be working. Added schema file as well.
…ssible as well. Added Makefile as well, made changes in design and readme documents.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 3 comments.
Suppressed comments (4)
internals/repository/idempotency_repository.go:53
- transfer_id is scanned into transferIDStr but never parsed/assigned into rec.TransferID, so callers will always see a zero UUID even when transfer_id is present. Either (1) scan transfer_id into a nullable UUID type and set rec.TransferID accordingly, or (2) remove TransferID from IdempotencyRecord if it’s intentionally unused.
var rec IdempotencyRecord
var transferIDStr string
err := tx.QueryRow(ctx, query, key).Scan(
&rec.IdempotencyKey, &rec.RequestHash, &transferIDStr, &rec.ResponseBody, &rec.StatusCode,
)
if err != nil {
if errors.Is(err, pgx.ErrNoRows) {
return nil, nil
}
return nil, fmt.Errorf("looking up idempotency record: %w", err)
}
return &rec, nil
schema.sql:44
- There are two schema sources (
schema.sqlanddb/init/001_schema.sql) and they already diverge (e.g., these indexes exist here but not in the Docker init schema). This will cause different behavior/perf depending on how the DB was created. Prefer a single canonical schema file (or generate one from the other) so local dev, CI, and manual runs stay consistent.
-- fast lookups for wallet history / balance recompute
CREATE INDEX idx_transfers_from_wallet ON transfers (from_wallet_id);
CREATE INDEX idx_transfers_to_wallet ON transfers (to_wallet_id);
internals/tests/transfer_test.go:25
- The integration test docs mention
TEST_DATABASE_URLandgo test ./tests/..., but the helper usesDATABASE_URLand the tests live underinternals/tests/. Update the comment to match the actual env var and package path to avoid misleading run instructions.
// Requires a running Postgres reachable via TEST_DATABASE_URL, with the
// schema already applied. Run with: go test ./tests/... -run Concurrent -v
func TestConcurrentTransfers_SameSourceWallet(t *testing.T) {
internals/service/transfer__service.go:125
- Correct typo in comment: 'violatiion' -> 'violation'.
// Check unique constraint violatiion on DB Side
| // Check unique constraint violatiion on DB Side | ||
| if isUniqueViolation(err) { | ||
|
|
||
| return s.handleRaceLostToDuplicate(ctx, tx, in.IdempotencyKey, &result) |
Comment on lines
+202
to
+212
| func (s *TransferService) handleRaceLostToDuplicate(ctx context.Context, tx pgx.Tx, key string, result **TransferResult) error { | ||
| existing, err := s.transferRepo.GetByIdempotencyKey(ctx, tx, key) | ||
| if err != nil { | ||
| return fmt.Errorf("resolving concurrent duplicate transfer: %w", err) | ||
| } | ||
| if existing == nil { | ||
| return errors.New("unique violation on idempotency key but no row found — unexpected state") | ||
| } | ||
| *result = toResult(existing) | ||
| return nil | ||
| } |
Comment on lines
+18
to
+20
| CREATE TABLE wallets ( | ||
| id uuid DEFAULT gen_random_uuid() NOT NULL, | ||
| owner_name text NOT NULL, |
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 -
Implements a wallet-to-wallet transfer service with idempotent request handling, a double-entry ledger, and concurrency-safe balance updates. Exposes POST /transfers. Built with a layered architecture (handler → service → repository → domain) so logic stays loosely coupled
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
The tools I used were:
Claude (Anthropic)
ChatGPT (OpenAI)
How you generally use the tool for your work.
I primarily used these tools to improve my understanding of distributed-system concepts, validate design decisions, review my implementation, identify potential edge cases, and improve my testing strategy.
The final implementation, architectural decisions, database design, and verification were reviewed and understood by me. I did not treat AI-generated suggestions as authoritative; I evaluated them against the assignment requirements and the behavior of the actual code and database. Used AI for a lot of documentation as well.
A transcript of your entire session with your AI tool of choice. You can add this to the repo or email it to us with your submission. If for some reason, this is not possible, give us all the prompts that you used with the AI.
Explain the general approach to the problem statement I have sent above, what would be your approach, don't implement anything yet, just list the tradeoffs and reasoning behind the decisions.
Explain how we will bypass if same idempotency key is used by or copied by other requests, what can we do to bypass that and solve it.
Give me the general schema according to the tables I have sent you also give me scripts to create the tables
Applying indexes at this stage of the code would be helpful right ? I have a general idea of applying indexes to superkeys and primary keys, tell me if anything else can be done to optimize this further
I am locking both wallets using SELECT FOR UPDATE. Explain why deterministic lock ordering is important and how it prevents deadlocks when two transfers happen in opposite directions.
Two identical requests arrive simultaneously with the same idempotency key. Both initially see no idempotency record. Explain what race can occur and how a database unique constraint can safely resolve it.
Would it be wise to run the server in a goroutine ? for cleaner exit and everything ?
409 — insufficient balance, or idempotency key reused with a different payload is this correct ?
Generate unit tests for this according to my expectations, also need integration tests for testing DB on a live DB (I reviewed the tests)
Schema Design
wallets —
id(uuid, PK),owner_name,balance(bigint, minor units,CHECK balance >= 0),version(int, bumped on every balance update),created_at,updated_attransfers —
id(uuid, PK),from_wallet_id/to_wallet_id(FK → wallets),amount(bigint,
CHECK amount > 0),state(PENDING/PROCESSED/FAILED),failure_reason,idempotency_key(unique index — backs both key lookups and the concurrent-duplicate racedetection),
created_at,updated_at.CHECK from_wallet_id <> to_wallet_idblocksself-transfers at the DB level.
ledger_entries —
id(uuid, PK),transfer_id(FK → transfers),wallet_id(FK → wallets),type(DEBIT/CREDIT),amount(bigint,CHECK amount > 0),created_at. Every processedtransfer writes exactly one DEBIT and one CREDIT row (double-entry), enforced by a unique
constraint on
(transfer_id, wallet_id, type).idempotency_records —
idempotency_key(PK),request_hash(detects same-key/different-payloadreuse),
transfer_id(FK → transfers, nullable),response_body(jsonb, cached full response),status_code,created_at. Durable enough that a retried request replays the exact prior HTTPoutcome without re-running the transfer.
Actual implementation might differ this is just designed before implementation.
Idempotency Strategy
Every request carries a client-supplied
idempotencyKey. On receipt, the service hashes thepayload (SHA-256 over
fromWalletId|toWalletId|amount) and looks upidempotency_recordsby key:as-is, without re-running the transfer.
ErrIdempotencyKeyConflict(409 Conflict).PROCESSEDorFAILED) ispersisted to
idempotency_recordsin the same database transaction as the transfer itself,so the cached record and the transfer's actual state can never diverge.
Two identical concurrent requests can both pass the initial lookup and race to insert the same
idempotency_keyintotransfers(which has a unique index on that column). The loser doesn'terror out — it detects the Postgres unique-violation (
23505), looks up the winner's transferby idempotency key, and returns that as its own result, so both callers see a consistent outcome.
A
FAILEDtransfer (e.g. insufficient balance) is treated as a terminal, cacheable outcome too —its idempotency record is saved just like a successful one, so a retry replays the failure
instead of re-attempting the transfer.
Concurrency Strategy
Both wallet rows are locked with
SELECT ... FOR UPDATE, always in ascending wallet-ID orderregardless of transfer direction — this consistent ordering is what prevents deadlocks between
two transfers moving money between the same wallet pair in opposite directions.
The wallets are locked before the transfer row is inserted, not after. Postgres takes an
implicit
FOR KEY SHARElock on both referenced wallet rows when the transfer insert's foreignkeys are checked — locking
FOR UPDATEfirst means that check is satisfied for free, avoiding ashared-lock-upgrade race that otherwise surfaces as a genuine Postgres deadlock (
40P01).All steps — balance checks, balance updates (with
versionbumped on each write), ledgerinserts, and the transfer's state transition — happen inside one database transaction, committed
only after every step succeeds. This gives atomicity: either the whole transfer lands, or none
of it does.
Isolation level: READ COMMITTED (Postgres default). Combined with row-level locking via
FOR UPDATE, this is sufficient — the explicit locks, not the isolation level, are whatserialize concurrent transfers on the same wallets.
How to Run
make db-upmake runOther useful targets:
make db-psql — open a psql shell into the container (wallet-transfer-postgres)
make db-reset — wipe data and re-apply schema fresh
make db-down — stop Postgres, keeping data
make build — build the binary to bin/api
How to Test
make test-unitmake db-upfirst):make test-integrationmake testTradeoffs / Assumptions
between any two wallets.
(e.g. via SQL) rather than through the API;
POST /transfersis the only exposed operation.unique key per logical transfer attempt (e.g. a UUID) and reuse it verbatim on retries. There's
no server-side dedup based on payload alone. Idempotency key cannot be empty
fromWalletId,toWalletId, andamount— it doesn't includethe idempotency key itself or any future fields (e.g. a memo/description), so extending the
request shape later means revisiting
hashRequest.practice, but it means lock order has no relation to insertion order or "from/to" semantics;
it's purely deterministic to avoid deadlocks, not meaningful otherwise.
FAILEDtransfer (e.g. insufficient balance) is terminal and cached — retrying with thesame idempotency key replays the same failure rather than re-checking the current balance. This
is intentional (idempotency implies "same request, same outcome") but means a transient failure
can't self-heal on retry — the caller must use a new idempotency key to try again.
GET /transfers/{id}or wallet-balance endpoint;verification is expected via direct DB inspection (
make db-psql) for this assignment's scope.on explicit
SELECT ... FOR UPDATElocking rather than Postgres's isolation level doing thework. Sufficient here since every write path goes through the same lock-then-modify pattern.
int64minor units (e.g. cents) — avoids floating-point precision issues, butassumes callers agree on currency/denomination out of band; there's no currency field or
multi-currency support.
Checklist