Implement thread-safe BankAccount with deposit, withdraw, and transfer methods

[PopovMarko]

Soulution for banking account

[coderabbitai]

Walkthrough

A new BankAccount implementation in Go is introduced with error-handled deposit, withdrawal, and transfer operations. The implementation includes parameter validation, minimum balance constraints, transaction limits, and thread-safe concurrent access using mutexes with deadlock-avoidant locking for transfers between accounts.

Changes

Cohort / File(s)	Summary
BankAccount Implementation challenge-7/submissions/PopovMarko/solution-template.go	Adds BankAccount struct with four exported error types (AccountError, InsufficientFundsError, NegativeAmountError, ExceedsLimitError), NewBankAccount constructor with validation, three methods (Deposit, Withdraw, Transfer) with mutex-based thread safety and deadlock-avoidant locking, and MaxTransactionAmount constant.

Sequence Diagram

sequenceDiagram
    participant Client
    participant AccountA as BankAccount A
    participant AccountB as BankAccount B

    Client->>AccountA: Transfer(amount, B)
    Note over AccountA: Validate amount > 0
    Note over AccountA: Check amount ≤ MaxTransactionAmount
    
    rect rgba(100, 150, 200, 0.5)
    Note over AccountA,AccountB: Deadlock-avoidant lock acquisition<br/>(sort by pointer address)
    AccountA->>AccountA: Acquire lock (lower address)
    AccountA->>AccountB: Acquire lock (higher address)
    end
    
    AccountA->>AccountA: Verify sufficient funds<br/>Balance - amount ≥ MinBalance
    AccountA->>AccountA: Deduct amount from A
    AccountA->>AccountB: Add amount to B
    
    rect rgba(100, 150, 200, 0.5)
    Note over AccountA,AccountB: Release locks
    AccountA->>AccountA: Release lock
    AccountA->>AccountB: Release lock
    end
    
    AccountA-->>Client: Return error or nil

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Add solution for Challenge 7 by RezaSi #555: Implements an almost identical BankAccount with the same struct, error types, constructor, Deposit/Withdraw/Transfer methods, and MaxTransactionAmount constant.
• Add solution for Challenge 7 by Kosench #612: Contains virtually identical BankAccount implementation including per-account mutexes and deadlock-avoiding transfer locking.
• Add solution for Challenge 7 by kiramux #733: Adds the same BankAccount struct, error types, methods, and mutex-based concurrency patterns with deadlock-safe lock ordering.

Poem

🐰 A ledger so fair, with mutexes to spare,
No deadlocks shall dare to appear!
From withdrawal to transfer, each hop is more perfect,
With balances balanced, the vault's crystal clear. 💰

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: implementing a thread-safe BankAccount with the three core methods (deposit, withdraw, transfer).
Description check	✅ Passed	The description contains a typo ('Soulution' instead of 'Solution') but is related to the changeset, referring to a banking account solution.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (3)

challenge-7/submissions/PopovMarko/solution-template.go (3)

4-8: Consider avoiding unsafe package by using account IDs for lock ordering.

The pointer-based lock ordering works correctly, but using unsafe may raise concerns during code audits. Since accounts already have unique ID fields, you could compare IDs lexicographically to determine lock order, eliminating the need for unsafe.

♻️ Alternative using ID comparison

-import (
-	"fmt"
-	"sync"
-	"unsafe"
-)
+import (
+	"fmt"
+	"sync"
+)

Then in Transfer:

// Lock in canonical order to prevent deadlock
first, second := a, target
if a.ID > target.ID {
    first, second = second, first
}

---

10-17: Exported Balance field allows unsynchronized reads, potentially causing data races.

External code can read account.Balance directly while another goroutine is modifying it via Deposit, Withdraw, or Transfer, creating a data race. Since mu is unexported, callers cannot safely acquire the lock before reading.

Consider making Balance unexported and providing a thread-safe getter method. However, if the challenge tests only use the provided methods (and don't access fields directly), this may be acceptable.

♻️ Proposed fix with thread-safe getter

 type BankAccount struct {
 	ID         string
 	Owner      string
-	Balance    float64
-	MinBalance float64
+	balance    float64
+	minBalance float64
 	mu         sync.Mutex // For thread safety
 }
+
+// GetBalance returns the current balance in a thread-safe manner.
+func (a *BankAccount) GetBalance() float64 {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	return a.balance
+}

Note: This would require updating all internal references from Balance to balance and MinBalance to minBalance.

---

41-44: Minor: Remove unnecessary blank line.

There's an extraneous blank line inside the Error() method.

🧹 Proposed fix

 func (e *InsufficientFundsError) Error() string {
-
 	return fmt.Sprintf("Insufficient funds, balance %.2f, min balance %.2f", e.Balance, e.MinBalance)
 }

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7ac764 and 4b76af5.

📒 Files selected for processing (1)

• challenge-7/submissions/PopovMarko/solution-template.go