Skip to content

Add solution for Challenge 7 by Kosench #612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 23, 2025
Merged

Add solution for Challenge 7 by Kosench #612

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
244 changes: 244 additions & 0 deletions challenge-7/submissions/Kosench/solution-template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
// Package challenge7 contains the solution for Challenge 7: Bank Account with Error Handling.
package challenge7

import (
"sync"
"fmt"
)

type BankAccount struct {
ID string
Owner string
Balance float64
MinBalance float64
mu sync.Mutex
}

const (
MaxTransactionAmount = 10000.0
)

type AccountError struct {
AccountID string
Operation string
}

func (e *AccountError) Error() string {
return fmt.Sprintf("account %s: error during %s", e.AccountID, e.Operation)
}

type InsufficientFundsError struct {
AccountError
Balance float64
Amount float64
MinBalance float64
}

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,
)
}

type NegativeAmountError struct {
AccountError
Amount float64
}

func (e *NegativeAmountError) Error() string {
return fmt.Sprintf(
"account %s: negative amount: $%.2f",
e.AccountID, e.Amount,
)
}

type ExceedsLimitError struct {
AccountError
Amount float64
Limit float64
}

func (e *ExceedsLimitError) Error() string {
return fmt.Sprintf(
"account %s: amount $%.2f exceeds limit $%.2f",
e.AccountID, e.Amount, e.Limit,
)
}

func NewBankAccount(id, owner string, initialBalance, minBalance float64) (*BankAccount, error) {
if id == "" {
return nil, &AccountError{
AccountID: "unknown",
Operation: "create",
}
}

if owner == "" {
return nil, &AccountError{
AccountID: id,
Operation: "create",
}
}

if initialBalance < 0 {
return nil, &NegativeAmountError{
AccountError: AccountError{
AccountID: id,
Operation: "create",
},
Amount: initialBalance,
}
}

if minBalance < 0 {
return nil, &NegativeAmountError{
AccountError: AccountError{
AccountID: id,
Operation: "create",
},
Amount: minBalance,
}
}

if initialBalance < minBalance {
return nil, &InsufficientFundsError{
AccountError: AccountError{
AccountID: id,
Operation: "create",
},
Balance: initialBalance,
Amount: minBalance,
MinBalance: minBalance,
}
}
Comment on lines +104 to +114
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

return &BankAccount{
ID: id,
Owner: owner,
Balance: initialBalance,
MinBalance: minBalance,
}, nil
}

func (a *BankAccount) Deposit(amount float64) error {
a.mu.Lock()
defer a.mu.Unlock()

if amount < 0 {
return &NegativeAmountError{
AccountError: AccountError{
AccountID: a.ID,
Operation: "deposit",
},
Amount: amount,
}
}

if amount > MaxTransactionAmount {
return &ExceedsLimitError{
AccountError: AccountError{
AccountID: a.ID,
Operation: "deposit",
},
Amount: amount,
Limit: MaxTransactionAmount,
}
}

a.Balance += amount
return nil
}

func (a *BankAccount) Withdraw(amount float64) error {
a.mu.Lock()
defer a.mu.Unlock()

if amount < 0 {
return &NegativeAmountError{
AccountError: AccountError{
AccountID: a.ID,
Operation: "withdraw",
},
Amount: amount,
}
}

if amount > MaxTransactionAmount {
return &ExceedsLimitError{
AccountError: AccountError{
AccountID: a.ID,
Operation: "withdraw",
},
Amount: amount,
Limit: MaxTransactionAmount,
}
}

if a.Balance-amount < a.MinBalance {
return &InsufficientFundsError{
AccountError: AccountError{
AccountID: a.ID,
Operation: "withdraw",
},
Balance: a.Balance,
Amount: amount,
MinBalance: a.MinBalance,
}
}

a.Balance -= amount
return nil
}

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()
}
Comment on lines +194 to +205
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

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
}
Comment on lines +194 to +244
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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 -60

Length 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 -30

Length 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 -100

Length 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.go

Length 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 -150

Length of output: 2411

Add guard against self-transfer and nil target in Transfer method.

The implementation lacks validation for two critical edge cases:

  1. 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.

  2. Nil target panic: Line 195 accesses target.ID without checking if target is 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.
Loading