Add solution for Challenge 7 by Kosench #612
Conversation
WalkthroughThis PR introduces a new Go file implementing a BankAccount system with public types for accounts and specific error conditions. It provides constructor, deposit, withdrawal, and transfer methods with input validation, balance constraints, and mutex-based concurrency control to prevent race conditions and deadlocks. Changes
Sequence Diagram(s)sequenceDiagram participant Client participant Account_A participant Account_B Client->>Account_A: Transfer(amount, Account_B) Note over Account_A: Validate amount<br/>(positive, within limit) rect rgb(200, 220, 255) Note over Account_A,Account_B: Deadlock-avoiding locking<br/>(ordered by ID) Account_A->>Account_A: Lock (lower ID) Account_A->>Account_B: Lock (higher ID) end rect rgb(220, 240, 220) Account_A->>Account_A: Check sufficient funds alt Insufficient Balance Account_A->>Account_A: Unlock both Account_A-->>Client: InsufficientFundsError else Sufficient Balance Account_A->>Account_A: Deduct from source Account_A->>Account_B: Add to target Note over Account_A,Account_B: Atomic update end end Account_A->>Account_B: Unlock (higher ID) Account_A->>Account_A: Unlock (lower ID) Account_A-->>Client: nil or error Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The review requires understanding multiple interconnected error types, validation logic across three related methods, and careful verification of the concurrency control pattern—particularly the deadlock-avoiding lock ordering during transfers and atomic balance updates. Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
challenge-7/submissions/Kosench/solution-template.go (7)
9-15: Avoid exposing fields and copying a type with sync.Mutex.Exported fields allow unsynchronized mutation and the struct is copyable (contains a Mutex). Prefer unexported fields + accessors or at least add a prominent “do not copy” doc comment.
37-42: Clarify InsufficientFundsError message (requested vs shortfall).“needed: $%.2f” is ambiguous across operations. Consider formatting based on Operation and include shortfall for clarity.
Apply:
func (e *InsufficientFundsError) Error() string { - return fmt.Sprintf( - "account %s: insufficient funds (balance: $%.2f, needed: $%.2f, min: $%.2f)", - e.AccountID, e.Balance, e.Amount, e.MinBalance, - ) + if e.Operation == "create" { + short := e.MinBalance - e.Balance + if short < 0 { short = 0 } + return fmt.Sprintf( + "account %s: opening balance $%.2f below minimum $%.2f (short by $%.2f)", + e.AccountID, e.Balance, e.MinBalance, short, + ) + } + short := e.MinBalance - (e.Balance - e.Amount) + if short < 0 { short = 0 } + return fmt.Sprintf( + "account %s: insufficient funds (balance: $%.2f, requested: $%.2f, min: $%.2f, short by $%.2f)", + e.AccountID, e.Balance, e.Amount, e.MinBalance, short, + ) }
4-7: Reject non‑finite deposit amounts.Guard against NaN/Inf to avoid poisoning state.
import ( - "sync" "fmt" + "math" + "sync" ) @@ func (a *BankAccount) Deposit(amount float64) error { a.mu.Lock() defer a.mu.Unlock() + if math.IsNaN(amount) || math.IsInf(amount, 0) { + return &AccountError{AccountID: a.ID, Operation: "deposit"} + }Also applies to: 124-151
153-192: Reject non‑finite withdrawal amounts.Same guard as Deposit.
func (a *BankAccount) Withdraw(amount float64) error { a.mu.Lock() defer a.mu.Unlock() + if math.IsNaN(amount) || math.IsInf(amount, 0) { + return &AccountError{AccountID: a.ID, Operation: "withdraw"} + }
244-244: Optional: concurrency‑safe balance accessor.Expose a read method to avoid racy external reads of Balance.
+// BalanceSnapshot returns the current balance under lock. +func (a *BankAccount) BalanceSnapshot() float64 { + a.mu.Lock() + defer a.mu.Unlock() + return a.Balance +}
12-13: Money amounts as float64.For financial values, prefer fixed‑precision (e.g., int64 cents or decimal) to avoid rounding drift.
21-28: Generic AccountError for invalid id/owner.Consider specific error types/messages (e.g., EmptyIDError, EmptyOwnerError) to aid callers.
| if initialBalance < minBalance { | ||
| return nil, &InsufficientFundsError{ | ||
| AccountError: AccountError{ | ||
| AccountID: id, | ||
| Operation: "create", | ||
| }, | ||
| Balance: initialBalance, | ||
| Amount: minBalance, | ||
| MinBalance: minBalance, | ||
| } | ||
| } |
There was a problem hiding this comment.
Ctor sets Amount to minBalance; use shortfall instead.
Amount should reflect the additional funds needed when creating the account.
- Balance: initialBalance, - Amount: minBalance, - MinBalance: minBalance, + Balance: initialBalance, + Amount: minBalance - initialBalance, // shortfall + MinBalance: minBalance,🤖 Prompt for AI Agents
In challenge-7/submissions/Kosench/solution-template.go around lines 104 to 114, the InsufficientFundsError constructed on account creation sets Amount to minBalance instead of the shortfall; change Amount to the difference (minBalance - initialBalance) so it represents the additional funds required (ensure it remains a non-negative value), leaving MinBalance as minBalance and Balance as initialBalance. | func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { | ||
| if a.ID < target.ID { | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| target.mu.Lock() | ||
| defer target.mu.Unlock() | ||
| } else { | ||
| target.mu.Lock() | ||
| defer target.mu.Unlock() | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
Fix nil‑target panic and self‑transfer deadlock; enforce stable lock order and validate early.
- Panic: target.ID is read before nil check.
- Deadlock: a==target attempts to lock the same mutex twice.
- Equal IDs across opposite transfers can lock in opposite orders.
func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { - if a.ID < target.ID { - a.mu.Lock() - defer a.mu.Unlock() - target.mu.Lock() - defer target.mu.Unlock() - } else { - target.mu.Lock() - defer target.mu.Unlock() - a.mu.Lock() - defer a.mu.Unlock() - } + // Pre-lock validations + if target == nil { + return &AccountError{AccountID: a.ID, Operation: "transfer(nil)"} + } + if a == target { + return &AccountError{AccountID: a.ID, Operation: "transfer(self)"} + } + if amount < 0 { + return &NegativeAmountError{ + AccountError: AccountError{AccountID: a.ID, Operation: "transfer"}, + Amount: amount, + } + } + if amount > MaxTransactionAmount { + return &ExceedsLimitError{ + AccountError: AccountError{AccountID: a.ID, Operation: "transfer"}, + Amount: amount, + Limit: MaxTransactionAmount, + } + } + // Stable lock order: by ID, tie-break by address to avoid equal-ID deadlocks. + first, second := a, target + if a.ID > target.ID || (a.ID == target.ID && fmt.Sprintf("%p", a) > fmt.Sprintf("%p", target)) { + first, second = target, a + } + first.mu.Lock() + defer first.mu.Unlock() + second.mu.Lock() + defer second.mu.Unlock() - if amount < 0 { - return &NegativeAmountError{ ... } - } - if amount > MaxTransactionAmount { - return &ExceedsLimitError{ ... } - } if a.Balance-amount < a.MinBalance { return &InsufficientFundsError{ AccountError: AccountError{ AccountID: a.ID, Operation: "transfer", }, Balance: a.Balance, Amount: amount, MinBalance: a.MinBalance, } } a.Balance -= amount target.Balance += amount return nil }Also applies to: 206-244
🤖 Prompt for AI Agents
In challenge-7/submissions/Kosench/solution-template.go around lines 194-205 (and also applies to 206-244): the Transfer method reads target.ID before checking for nil and may deadlock when a == target or when IDs are equal and two transfers use opposite ordering. First validate target != nil and return an error immediately; then if a == target return a no-op or error to avoid trying to lock the same mutex twice. To enforce a stable lock order, order locks by a stable key that cannot cause ambiguity (for example pointer address comparison or combine ID with pointer) and handle equal-key case by falling back to pointer comparison; acquire the first lock, then the second, and defer their unlocks. Ensure no code path attempts to lock the same mutex twice. | func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { | ||
| if a.ID < target.ID { | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| target.mu.Lock() | ||
| defer target.mu.Unlock() | ||
| } else { | ||
| target.mu.Lock() | ||
| defer target.mu.Unlock() | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| } | ||
| | ||
| if amount < 0 { | ||
| return &NegativeAmountError{ | ||
| AccountError: AccountError{ | ||
| AccountID: a.ID, | ||
| Operation: "transfer", | ||
| }, | ||
| Amount: amount, | ||
| } | ||
| } | ||
| | ||
| if amount > MaxTransactionAmount { | ||
| return &ExceedsLimitError{ | ||
| AccountError: AccountError{ | ||
| AccountID: a.ID, | ||
| Operation: "transfer", | ||
| }, | ||
| Amount: amount, | ||
| Limit: MaxTransactionAmount, | ||
| } | ||
| } | ||
| | ||
| if a.Balance-amount < a.MinBalance { | ||
| return &InsufficientFundsError{ | ||
| AccountError: AccountError{ | ||
| AccountID: a.ID, | ||
| Operation: "transfer", | ||
| }, | ||
| Balance: a.Balance, | ||
| Amount: amount, | ||
| MinBalance: a.MinBalance, | ||
| } | ||
| } | ||
| | ||
| a.Balance -= amount | ||
| target.Balance += amount | ||
| | ||
| return nil | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Verification: scan repo for self/nil transfers.
Run to spot potential problematic callsites.
🏁 Script executed:
#!/bin/bash # Find Transfer call sites with suspicious args rg -nP --type=go -C2 '\.\s*Transfer\s*\('Length of output: 327
I need to examine the test cases more closely to determine if self-transfer or nil-transfer scenarios are tested or could be problematic.
🏁 Script executed:
#!/bin/bash # Find Transfer test cases and look for self-transfer or nil scenarios rg -nP --type=go -C5 'Transfer.*error|shouldError' challenge-7/solution-template_test.go | head -60Length of output: 1658
🏁 Script executed:
#!/bin/bash # Check if implementation validates target parameter or guards against self-transfer ast-grep --pattern $'func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { $$$ }' Length of output: 10601
🏁 Script executed:
#!/bin/bash # Look for any test table definitions that might include self-transfer or nil cases rg -nP --type=go -B20 'source.Transfer' challenge-7/solution-template_test.go | grep -E '(tc|case|struct|Transfer)' | head -30Length of output: 126
Let me check the Transfer test cases to see if these edge cases are tested:
🏁 Script executed:
#!/bin/bash # Find Transfer test cases with full context rg -nP --type=go -A100 'func.*TestTransfer' challenge-7/solution-template_test.go | grep -A80 'testCases.*:=' | head -100Length of output: 2648
🏁 Script executed:
#!/bin/bash # Search for any test cases mentioning self, same, identical, or nil in transfer context rg -nP --type=go -i '(self|same|identical|nil).*[Tt]ransfer|[Tt]ransfer.*(self|same|identical|nil)' challenge-7/solution-template_test.goLength of output: 0
🏁 Script executed:
#!/bin/bash # Check the complete Transfer test function sed -n '/func.*TestTransfer/,/^func /p' challenge-7/solution-template_test.go | head -150Length of output: 2411
Add guard against self-transfer and nil target in Transfer method.
The implementation lacks validation for two critical edge cases:
-
Self-transfer deadlock: When
a.ID == target.ID, the else branch (lines 200-204) attempts to lock the same mutex twice, causing a deadlock since Go mutexes are non-reentrant. -
Nil target panic: Line 195 accesses
target.IDwithout checking iftargetis nil, causing a panic.
Add checks at the start of Transfer (after line 194):
- Return error if
target == nil - Return error if
a.ID == target.ID
Current test usage (distinct accounts) is safe, but the implementation should handle these edge cases defensively.
🤖 Prompt for AI Agents
In challenge-7/submissions/Kosench/solution-template.go around lines 194 to 244, add defensive checks at the start of Transfer: first verify target != nil and return an error if it is (so you don't dereference target.ID), then check if a.ID == target.ID and return an error to forbid self-transfers (this prevents attempting to lock the same mutex twice and deadlocking); keep the existing ID-based lock ordering only after these validations. Use an appropriate error return (existing error type in the package or fmt.Errorf) consistent with other methods. 
2 participants
Challenge 7 Solution
Submitted by: @Kosench
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/Kosench/solution-template.goTesting
Thank you for reviewing my submission! 🚀