Skip to content

refactor: Make domain objects immutable (frozen dataclasses) and hashable #65

@przemyslawbialon

Description

@przemyslawbialon

Problem

Domain value objects are mutable and have inconsistent equality/hashing:

B4. Mutable + defensive copy.deepcopy

# pit38/domain/stock/queue.py:11-13
def append(self, item: Transaction):
    copied = copy.deepcopy(item)
    self._queue.append(copied)

The deepcopy is there because Transaction is mutable and the FIFO queue must protect itself from callers modifying the original. This is defensive programming against a missing invariant. Every Queue.append now deep-copies Transaction + AssetValue + FiatValue + pendulum.DateTime.

Worse: per_stock_calculator.py:69 does buy_queue.replace_head(new_head) where new_head = buy_queue.head() * (1 - ratio). This relies on Transaction.__mul__ returning a new Transaction (which it does — good), but the pattern of mutating "the queue but not the transaction" works only because of the deepcopy in append.

B5. Custom __eq__ without __hash__

# pit38/domain/transactions/transaction.py:31-32
def __eq__(self, other):
    return self.__dict__ == other.__dict__
# no __hash__

Python convention: when you override __eq__, Python sets __hash__ = None → object becomes unhashable. Today no code puts Transaction in a set or dict key, so nothing breaks visibly. Tomorrow someone adds visited_transactions: set[Transaction] → silent TypeError: unhashable type.

Same pattern in AssetValue.__eq__ (asset.py:10-11), FiatValue.__eq__ (currencies.py:76-77).

Goal

Make all domain value objects immutable via @dataclass(frozen=True). This:

  1. Removes the need for copy.deepcopy in Queue
  2. Provides __eq__ and __hash__ for free
  3. Prevents accidental mutation bugs in tax-sensitive code paths
  4. Enables use in sets/dicts (e.g. deduplication, caching)

Acceptance criteria

  • FiatValue, AssetValue, Transaction, Dividend, ServiceFee, StockSplit all @dataclass(frozen=True)
  • Operators (__add__, __mul__, __sub__) return new instances (already the case — verify)
  • Drop copy.deepcopy from pit38/domain/stock/queue.py:12
  • Drop all manual __eq__ implementations — dataclass provides them
  • Queue._queue: list[Transaction] works without deep-copying append
  • All 92 existing tests pass
  • New test: hash(Transaction(...)) works (proves __hash__ available)
  • New test: verify Transaction.__mul__(0.5) returns a new instance, doesn't mutate original

Implementation sketch

from dataclasses import dataclass
from decimal import Decimal  # after A1

@dataclass(frozen=True)
class FiatValue:
    amount: Decimal
    currency: Currency
    
    def __add__(self, other: "FiatValue") -> "FiatValue":
        if self.currency != other.currency:
            raise InvalidCurrencyException(...)
        return FiatValue(self.amount + other.amount, self.currency)
    
    # __eq__, __hash__ auto-generated

@dataclass(frozen=True)
class Transaction:
    asset: AssetValue
    fiat_value: FiatValue
    action: Action
    date: pendulum.DateTime
    
    @property
    def year(self) -> int:
        return self.date.year
    
    # Dropped: __mul__ moved to explicit scale() method, more clear
    def scale(self, ratio: float | Decimal) -> "Transaction":
        return dataclasses.replace(
            self, 
            asset=self.asset.scale(ratio),
            fiat_value=self.fiat_value.scale(ratio),
        )

Dataclass field with mutable default? Currency.ZLOTY is an enum singleton — safe. pendulum.DateTime is technically mutable (it shouldn't be in practice) — needs check.

Scale

3-4h incl. tests.

Priority

P1 — correctness-adjacent (removes a class of bugs), low effort, big readability win.

Related

  • Resolves B4 + B5 from the tech review
  • Best done together with or right after A1 (Decimal) — same value types, natural batching
  • Should happen before MarketEvent refactor (so the new hierarchy starts immutable)

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions