From c50455d5f6dbc52cca46671bb7bafd89daa98e5f Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 12:23:04 +0200 Subject: [PATCH 1/8] fix(migrations): make all migrations idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration version bump lands in the core dbversions table while the DDL lands in ext_libra — the two writes are not atomic. A failed bump re-runs the whole migration on next boot; bare CREATE/ALTER/INSERT then crashes the extension until manual dbversions surgery. - CREATE TABLE / CREATE INDEX -> IF NOT EXISTS - ALTER TABLE ADD COLUMN -> _alter_add_column_safe (same swallow pattern as the events/withdraw fork migrations) - seed INSERTs (default accounts, virtual parents, default roles) -> ON CONFLICT (name) DO NOTHING Tests run the chain twice against a fresh SQLite DB (full-chain rerun and per-migration rerun); both fail against the previous migrations. Addresses CODE-REVIEW-2026-06 findings #3 and #12. Co-Authored-By: Claude Fable 5 --- migrations.py | 105 +++++++++++++++++++++++-------------- tests/test_migrations.py | 109 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 38 deletions(-) create mode 100644 tests/test_migrations.py diff --git a/migrations.py b/migrations.py index 9c38c55..2c39507 100644 --- a/migrations.py +++ b/migrations.py @@ -34,9 +34,33 @@ Original migration sequence (Nov 2025): - m014: Removed legacy equity accounts (MemberEquity, RetainedEarnings) - m015: Converted entry_lines to single amount field - m016: Dropped journal_entries and entry_lines tables (Fava integration) + +IDEMPOTENCY CONTRACT: +Every statement here must be a silent no-op on re-run. The migration +version bump lands in the core LNbits DB (`dbversions`) while the DDL +lands in `ext_libra` — the two writes are not atomic. If the version +bump fails after the DDL commits, the whole migration re-runs on next +boot; a bare CREATE/ALTER/INSERT then crashes the extension until +manual dbversions surgery. """ +async def _alter_add_column_safe(db, sql: str) -> None: + """ALTER TABLE ADD COLUMN that swallows duplicate-column errors. + + Neither SQLite nor Postgres supports ADD COLUMN IF NOT EXISTS + portably, so re-runs are made no-ops by swallowing the error both + backends raise for an existing column. + """ + try: + await db.execute(sql) + except Exception as exc: + msg = str(exc).lower() + if "duplicate column" in msg or "already exists" in msg: + return + raise + + async def m001_initial(db): """ Initial Libra database schema (squashed from m001-m016). @@ -63,7 +87,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE accounts ( + CREATE TABLE IF NOT EXISTS accounts ( id TEXT PRIMARY KEY, name TEXT NOT NULL UNIQUE, account_type TEXT NOT NULL, @@ -76,13 +100,13 @@ async def m001_initial(db): await db.execute( """ - CREATE INDEX idx_accounts_user_id ON accounts (user_id); + CREATE INDEX IF NOT EXISTS idx_accounts_user_id ON accounts (user_id); """ ) await db.execute( """ - CREATE INDEX idx_accounts_type ON accounts (account_type); + CREATE INDEX IF NOT EXISTS idx_accounts_type ON accounts (account_type); """ ) @@ -93,7 +117,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE extension_settings ( + CREATE TABLE IF NOT EXISTS extension_settings ( id TEXT NOT NULL PRIMARY KEY, libra_wallet_id TEXT, fava_url TEXT NOT NULL DEFAULT 'http://localhost:3333', @@ -111,7 +135,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE user_wallet_settings ( + CREATE TABLE IF NOT EXISTS user_wallet_settings ( id TEXT NOT NULL PRIMARY KEY, user_wallet_id TEXT, updated_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now} @@ -126,7 +150,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE manual_payment_requests ( + CREATE TABLE IF NOT EXISTS manual_payment_requests ( id TEXT PRIMARY KEY, user_id TEXT NOT NULL, amount INTEGER NOT NULL, @@ -143,14 +167,14 @@ async def m001_initial(db): await db.execute( """ - CREATE INDEX idx_manual_payment_requests_user_id + CREATE INDEX IF NOT EXISTS idx_manual_payment_requests_user_id ON manual_payment_requests (user_id); """ ) await db.execute( """ - CREATE INDEX idx_manual_payment_requests_status + CREATE INDEX IF NOT EXISTS idx_manual_payment_requests_status ON manual_payment_requests (status); """ ) @@ -163,7 +187,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE balance_assertions ( + CREATE TABLE IF NOT EXISTS balance_assertions ( id TEXT PRIMARY KEY, date TIMESTAMP NOT NULL, account_id TEXT NOT NULL, @@ -188,21 +212,21 @@ async def m001_initial(db): await db.execute( """ - CREATE INDEX idx_balance_assertions_account_id + CREATE INDEX IF NOT EXISTS idx_balance_assertions_account_id ON balance_assertions (account_id); """ ) await db.execute( """ - CREATE INDEX idx_balance_assertions_status + CREATE INDEX IF NOT EXISTS idx_balance_assertions_status ON balance_assertions (status); """ ) await db.execute( """ - CREATE INDEX idx_balance_assertions_date + CREATE INDEX IF NOT EXISTS idx_balance_assertions_date ON balance_assertions (date); """ ) @@ -216,7 +240,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE user_equity_status ( + CREATE TABLE IF NOT EXISTS user_equity_status ( user_id TEXT PRIMARY KEY, is_equity_eligible BOOLEAN NOT NULL DEFAULT FALSE, equity_account_name TEXT, @@ -230,7 +254,7 @@ async def m001_initial(db): await db.execute( """ - CREATE INDEX idx_user_equity_status_eligible + CREATE INDEX IF NOT EXISTS idx_user_equity_status_eligible ON user_equity_status (is_equity_eligible) WHERE is_equity_eligible = TRUE; """ @@ -245,7 +269,7 @@ async def m001_initial(db): await db.execute( f""" - CREATE TABLE account_permissions ( + CREATE TABLE IF NOT EXISTS account_permissions ( id TEXT PRIMARY KEY, user_id TEXT NOT NULL, account_id TEXT NOT NULL, @@ -262,7 +286,7 @@ async def m001_initial(db): # Index for looking up permissions by user await db.execute( """ - CREATE INDEX idx_account_permissions_user_id + CREATE INDEX IF NOT EXISTS idx_account_permissions_user_id ON account_permissions (user_id); """ ) @@ -270,7 +294,7 @@ async def m001_initial(db): # Index for looking up permissions by account await db.execute( """ - CREATE INDEX idx_account_permissions_account_id + CREATE INDEX IF NOT EXISTS idx_account_permissions_account_id ON account_permissions (account_id); """ ) @@ -278,7 +302,7 @@ async def m001_initial(db): # Composite index for checking specific user+account permissions await db.execute( """ - CREATE INDEX idx_account_permissions_user_account + CREATE INDEX IF NOT EXISTS idx_account_permissions_user_account ON account_permissions (user_id, account_id); """ ) @@ -286,7 +310,7 @@ async def m001_initial(db): # Index for finding permissions by type await db.execute( """ - CREATE INDEX idx_account_permissions_type + CREATE INDEX IF NOT EXISTS idx_account_permissions_type ON account_permissions (permission_type); """ ) @@ -294,7 +318,7 @@ async def m001_initial(db): # Index for finding expired permissions await db.execute( """ - CREATE INDEX idx_account_permissions_expires + CREATE INDEX IF NOT EXISTS idx_account_permissions_expires ON account_permissions (expires_at) WHERE expires_at IS NOT NULL; """ @@ -320,6 +344,7 @@ async def m001_initial(db): f""" INSERT INTO accounts (id, name, account_type, description, created_at) VALUES (:id, :name, :type, :description, {db.timestamp_now}) + ON CONFLICT (name) DO NOTHING """, { "id": str(uuid.uuid4()), @@ -342,17 +367,18 @@ async def m002_add_account_is_active(db): Default: All existing accounts are marked as active (TRUE). """ - await db.execute( + await _alter_add_column_safe( + db, """ ALTER TABLE accounts ADD COLUMN is_active BOOLEAN NOT NULL DEFAULT TRUE - """ + """, ) # Create index for faster queries filtering by is_active await db.execute( """ - CREATE INDEX idx_accounts_is_active ON accounts (is_active) + CREATE INDEX IF NOT EXISTS idx_accounts_is_active ON accounts (is_active) """ ) @@ -372,17 +398,18 @@ async def m003_add_account_is_virtual(db): Default: All existing accounts are real (is_virtual = FALSE). """ - await db.execute( + await _alter_add_column_safe( + db, """ ALTER TABLE accounts ADD COLUMN is_virtual BOOLEAN NOT NULL DEFAULT FALSE - """ + """, ) # Create index for faster queries filtering by is_virtual await db.execute( """ - CREATE INDEX idx_accounts_is_virtual ON accounts (is_virtual) + CREATE INDEX IF NOT EXISTS idx_accounts_is_virtual ON accounts (is_virtual) """ ) @@ -402,6 +429,7 @@ async def m003_add_account_is_virtual(db): f""" INSERT INTO accounts (id, name, account_type, description, is_active, is_virtual, created_at) VALUES (:id, :name, :type, :description, TRUE, TRUE, {db.timestamp_now}) + ON CONFLICT (name) DO NOTHING """, { "id": str(uuid.uuid4()), @@ -438,7 +466,7 @@ async def m004_add_rbac_tables(db): await db.execute( f""" - CREATE TABLE roles ( + CREATE TABLE IF NOT EXISTS roles ( id TEXT PRIMARY KEY, name TEXT NOT NULL UNIQUE, description TEXT, @@ -451,13 +479,13 @@ async def m004_add_rbac_tables(db): await db.execute( """ - CREATE INDEX idx_roles_name ON roles (name); + CREATE INDEX IF NOT EXISTS idx_roles_name ON roles (name); """ ) await db.execute( """ - CREATE INDEX idx_roles_is_default ON roles (is_default) + CREATE INDEX IF NOT EXISTS idx_roles_is_default ON roles (is_default) WHERE is_default = TRUE; """ ) @@ -469,7 +497,7 @@ async def m004_add_rbac_tables(db): await db.execute( f""" - CREATE TABLE role_permissions ( + CREATE TABLE IF NOT EXISTS role_permissions ( id TEXT PRIMARY KEY, role_id TEXT NOT NULL, account_id TEXT NOT NULL, @@ -484,19 +512,19 @@ async def m004_add_rbac_tables(db): await db.execute( """ - CREATE INDEX idx_role_permissions_role_id ON role_permissions (role_id); + CREATE INDEX IF NOT EXISTS idx_role_permissions_role_id ON role_permissions (role_id); """ ) await db.execute( """ - CREATE INDEX idx_role_permissions_account_id ON role_permissions (account_id); + CREATE INDEX IF NOT EXISTS idx_role_permissions_account_id ON role_permissions (account_id); """ ) await db.execute( """ - CREATE INDEX idx_role_permissions_type ON role_permissions (permission_type); + CREATE INDEX IF NOT EXISTS idx_role_permissions_type ON role_permissions (permission_type); """ ) @@ -507,7 +535,7 @@ async def m004_add_rbac_tables(db): await db.execute( f""" - CREATE TABLE user_roles ( + CREATE TABLE IF NOT EXISTS user_roles ( id TEXT PRIMARY KEY, user_id TEXT NOT NULL, role_id TEXT NOT NULL, @@ -522,19 +550,19 @@ async def m004_add_rbac_tables(db): await db.execute( """ - CREATE INDEX idx_user_roles_user_id ON user_roles (user_id); + CREATE INDEX IF NOT EXISTS idx_user_roles_user_id ON user_roles (user_id); """ ) await db.execute( """ - CREATE INDEX idx_user_roles_role_id ON user_roles (role_id); + CREATE INDEX IF NOT EXISTS idx_user_roles_role_id ON user_roles (role_id); """ ) await db.execute( """ - CREATE INDEX idx_user_roles_expires ON user_roles (expires_at) + CREATE INDEX IF NOT EXISTS idx_user_roles_expires ON user_roles (expires_at) WHERE expires_at IS NOT NULL; """ ) @@ -542,7 +570,7 @@ async def m004_add_rbac_tables(db): # Composite index for checking specific user+role assignments await db.execute( """ - CREATE INDEX idx_user_roles_user_role ON user_roles (user_id, role_id); + CREATE INDEX IF NOT EXISTS idx_user_roles_user_role ON user_roles (user_id, role_id); """ ) @@ -586,6 +614,7 @@ async def m004_add_rbac_tables(db): f""" INSERT INTO roles (id, name, description, is_default, created_by, created_at) VALUES (:id, :name, :description, :is_default, :created_by, {db.timestamp_now}) + ON CONFLICT (name) DO NOTHING """, { "id": str(uuid.uuid4()), diff --git a/tests/test_migrations.py b/tests/test_migrations.py new file mode 100644 index 0000000..b66b595 --- /dev/null +++ b/tests/test_migrations.py @@ -0,0 +1,109 @@ +"""Migration idempotency tests. + +The migration version bump lands in the core LNbits DB (`dbversions`) +while the DDL lands in `ext_libra` — the two writes are not atomic. If +the bump fails after the DDL commits, the whole migration re-runs on +next boot, so every statement must be a silent no-op on re-run instead +of crashing with `duplicate column` / `table exists` / UNIQUE +violations (which bricks the extension until manual dbversions +surgery). + +These tests run the full migration chain twice against a fresh SQLite +database — the second pass simulates the lost-version-bump re-run. +""" +import importlib +import re +from uuid import uuid4 + +import pytest + +from lnbits.db import Database + +pytestmark = pytest.mark.anyio + + +def _module(name: str): + """Import a libra submodule under whichever path the active LNbits layout + uses (default `lnbits.extensions.libra` or bare `libra`).""" + for prefix in ("lnbits.extensions.libra", "libra"): + try: + return importlib.import_module(f"{prefix}.{name}") + except ModuleNotFoundError: + continue + raise ModuleNotFoundError(f"libra.{name}: tried both import paths") + + +migrations = _module("migrations") + +# Same discovery as lnbits.core.helpers.run_migration: m### prefix, +# module definition order. +_MIGRATION_RE = re.compile(r"^m(\d\d\d)_") +MIGRATION_FUNCTIONS = [ + fn for name, fn in vars(migrations).items() if _MIGRATION_RE.match(name) +] + +# Tables the chain must leave behind — one probe row read per table +# proves both existence and queryability after a double run. +EXPECTED_TABLES = [ + "accounts", + "extension_settings", + "user_wallet_settings", + "manual_payment_requests", + "balance_assertions", + "user_equity_status", + "account_permissions", + "roles", + "role_permissions", + "user_roles", +] + + +async def _run_all_migrations(db: Database) -> None: + async with db.connect() as conn: + for migrate in MIGRATION_FUNCTIONS: + await migrate(conn) + + +async def _seed_counts(db: Database) -> dict: + async with db.connect() as conn: + accounts = await conn.fetchall("SELECT id, name FROM accounts") + roles = await conn.fetchall("SELECT id, name FROM roles") + return { + "account_names": sorted(r["name"] for r in accounts), + "account_ids": sorted(r["id"] for r in accounts), + "role_names": sorted(r["name"] for r in roles), + "role_ids": sorted(r["id"] for r in roles), + } + + +async def test_migrations_rerun_is_noop(): + """Full chain twice: second run must not raise and not re-seed.""" + db = Database(f"ext_libra_migtest_{uuid4().hex[:8]}") + + await _run_all_migrations(db) + first = await _seed_counts(db) + + # Simulate the lost dbversions bump: everything runs again. + await _run_all_migrations(db) + second = await _seed_counts(db) + + # Seeds must not duplicate (names) and must not be replaced (ids). + assert second == first + assert first["account_names"], "seed accounts missing after migration" + assert "Employee" in first["role_names"] + + # Every table exists and is queryable after the double run. + async with db.connect() as conn: + for table in EXPECTED_TABLES: + await conn.fetchall(f"SELECT * FROM {table} LIMIT 1") # noqa: S608 + + +async def test_single_migration_rerun_is_noop(): + """Each migration individually survives an immediate re-run (the + version bump fails right after that one migration committed).""" + db = Database(f"ext_libra_migtest_{uuid4().hex[:8]}") + + async with db.connect() as conn: + for migrate in MIGRATION_FUNCTIONS: + await migrate(conn) + await migrate(conn) # re-run before "bumping" to the next From 4fdb358bb0d76d2de5299cd601850635d03cc56a Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 12:33:42 +0200 Subject: [PATCH 2/8] fix(payments): add local idempotency gate for Lightning recording The Fava-side duplicate checks (add_entry_idempotent, journal-link scan) are read-then-write races: on restart with a persisted invoice queue, or webhook + poller firing together, both callers pass the "not present" check and both insert. New processed_payments table (m005) keyed on payment_hash; exactly one claimant wins the INSERT ... ON CONFLICT DO NOTHING. Lifecycle: 'processing' while the write is in flight, 'done' after; failed recordings release the claim so redelivery retries, and 'processing' rows from a crashed process are cleared at listener startup. Also wraps the invoice-listener loop body in try/except so one poison payment can't kill payment recording for the process lifetime. Addresses CODE-REVIEW-2026-06 findings #4 and #9. Co-Authored-By: Claude Fable 5 --- crud.py | 70 ++++++++++++++++++++++++++++++++++++++++ migrations.py | 27 ++++++++++++++++ tasks.py | 40 ++++++++++++++++++++++- tests/test_migrations.py | 1 + 4 files changed, 137 insertions(+), 1 deletion(-) diff --git a/crud.py b/crud.py index 0692806..b49bb9b 100644 --- a/crud.py +++ b/crud.py @@ -1696,3 +1696,73 @@ async def check_user_has_role_permission( return True return False + + +# ============================================================================= +# PROCESSED PAYMENTS (Lightning payment idempotency gate) +# ============================================================================= +# The Fava-side duplicate checks are read-then-write races; this table's +# primary key on payment_hash makes exactly one claimant win. Shared by the +# background invoice listener (tasks.on_invoice_paid) and the client-driven +# /record-payment endpoint. + + +async def claim_payment(payment_hash: str) -> bool: + """Atomically claim a Lightning payment for recording. + + Returns True when this caller owns the claim; False when the payment + is already recorded or another coroutine is recording it right now. + """ + result = await db.execute( + """ + INSERT INTO processed_payments (payment_hash, status) + VALUES (:payment_hash, 'processing') + ON CONFLICT (payment_hash) DO NOTHING + """, + {"payment_hash": payment_hash}, + ) + return result.rowcount == 1 + + +async def get_processed_payment(payment_hash: str) -> Optional[dict]: + row = await db.fetchone( + "SELECT payment_hash, status, entry_id FROM processed_payments" + " WHERE payment_hash = :payment_hash", + {"payment_hash": payment_hash}, + ) + return dict(row) if row else None + + +async def mark_payment_done(payment_hash: str, entry_id: Optional[str] = None) -> None: + await db.execute( + """ + UPDATE processed_payments SET status = 'done', entry_id = :entry_id + WHERE payment_hash = :payment_hash + """, + {"payment_hash": payment_hash, "entry_id": entry_id}, + ) + + +async def release_payment_claim(payment_hash: str) -> None: + """Compensating delete after a failed recording, so redelivery retries. + + Only removes an in-flight claim — a 'done' row is permanent. + """ + await db.execute( + "DELETE FROM processed_payments" + " WHERE payment_hash = :payment_hash AND status = 'processing'", + {"payment_hash": payment_hash}, + ) + + +async def clear_stale_payment_claims() -> int: + """Drop 'processing' claims left behind by a previous process life. + + A live claim only exists inside a running coroutine, so anything + still 'processing' at listener startup belongs to a crashed or + restarted process and would otherwise block that payment forever. + """ + result = await db.execute( + "DELETE FROM processed_payments WHERE status = 'processing'" + ) + return result.rowcount diff --git a/migrations.py b/migrations.py index 2c39507..d8a4bba 100644 --- a/migrations.py +++ b/migrations.py @@ -624,3 +624,30 @@ async def m004_add_rbac_tables(db): "created_by": "system", # System-created default roles }, ) + + +async def m005_add_processed_payments(db): + """ + Local idempotency gate for Lightning payment recording. + + The Fava-side duplicate check (`add_entry_idempotent`, journal-link + scan) is a read-then-write race: the background invoice listener and + the client-driven /record-payment endpoint can both pass the "not + present" check for the same payment_hash and both insert. The + primary key on payment_hash makes exactly one claimant win. + + status lifecycle: 'processing' (claimed, write in flight) → 'done' + (entry recorded). Failed claims are deleted so redelivery retries; + 'processing' rows from a crashed process are cleared at listener + startup. + """ + await db.execute( + f""" + CREATE TABLE IF NOT EXISTS processed_payments ( + payment_hash TEXT PRIMARY KEY, + status TEXT NOT NULL DEFAULT 'processing', + entry_id TEXT, + created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now} + ); + """ + ) diff --git a/tasks.py b/tasks.py index 8ed5a33..158d913 100644 --- a/tasks.py +++ b/tasks.py @@ -179,12 +179,31 @@ async def wait_for_paid_invoices(): This ensures payments are recorded even if the user closes their browser before the payment is detected by client-side polling. """ + from .crud import clear_stale_payment_claims + invoice_queue = Queue() register_invoice_listener(invoice_queue, "ext_libra") + # Claims from a previous process life can't be live anymore — clear + # them so those payments aren't blocked forever. + cleared = await clear_stale_payment_claims() + if cleared: + logger.warning( + f"[LIBRA] Cleared {cleared} stale in-flight payment claim(s) " + "from a previous run" + ) + while True: payment = await invoice_queue.get() - await on_invoice_paid(payment) + try: + await on_invoice_paid(payment) + except Exception: + # One bad payment must not kill the listener for the rest of + # the process lifetime; its claim was released, so redelivery + # can retry it. + logger.exception( + f"[LIBRA] Failed to record payment {payment.payment_hash}" + ) async def on_invoice_paid(payment: Payment) -> None: @@ -210,8 +229,20 @@ async def on_invoice_paid(payment: Payment) -> None: logger.warning(f"Libra invoice {payment.payment_hash} missing user_id in metadata") return + from .crud import claim_payment, mark_payment_done, release_payment_claim from .fava_client import get_fava_client + # Local idempotency gate: exactly one claimant (this listener or the + # /record-payment endpoint) gets to record a given payment_hash. The + # Fava-side idempotent write below stays as a second layer for + # entries recorded before this table existed. + if not await claim_payment(payment.payment_hash): + logger.info( + f"Payment {payment.payment_hash} already recorded or being " + "recorded; skipping" + ) + return + fava = get_fava_client() # Use idempotency key based on payment hash - this ensures duplicate @@ -245,6 +276,7 @@ async def on_invoice_paid(payment: Payment) -> None: if not fiat_currency or not fiat_amount: logger.error(f"Payment {payment.payment_hash} missing fiat currency/amount metadata") + await release_payment_claim(payment.payment_hash) return # Get user's current balance to determine receivables and payables @@ -276,6 +308,7 @@ async def on_invoice_paid(payment: Payment) -> None: lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning") if not lightning_account: logger.error("Lightning account 'Assets:Bitcoin:Lightning' not found") + await release_payment_claim(payment.payment_hash) return # Query for unsettled entries to link this settlement back to them @@ -324,6 +357,11 @@ async def on_invoice_paid(payment: Payment) -> None: f"{result.get('data', 'Unknown')}" ) + await mark_payment_done(payment.payment_hash, idempotency_key) + except Exception as e: logger.error(f"Error recording Libra payment {payment.payment_hash}: {e}") + # Release the claim so redelivery (or the /record-payment + # endpoint) can retry this payment. + await release_payment_claim(payment.payment_hash) raise diff --git a/tests/test_migrations.py b/tests/test_migrations.py index b66b595..1586473 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -55,6 +55,7 @@ EXPECTED_TABLES = [ "roles", "role_permissions", "user_roles", + "processed_payments", ] From 44e10caac7ca7bee011ff9e010896623058c1aee Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 12:41:42 +0200 Subject: [PATCH 3/8] fix(payments): record-payment fails closed and shares the claim gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes to POST /api/v1/record-payment: - The Fava duplicate check caught every exception and proceeded to write, so a transient Fava blip produced double entries. It now fails closed: transport errors return 503 and the client retries. While here: the check queried {base_url}/api/journal, but base_url already ends in /api — the doubled path 404'd, meaning the duplicate check has silently never run. - The endpoint now goes through the same processed_payments claim gate as the background invoice listener, so the webhook+poller pair can't both record the same payment_hash: a 'done' claim replays as "already recorded", an in-flight claim returns 409. Addresses CODE-REVIEW-2026-06 finding #10. Co-Authored-By: Claude Fable 5 --- tests/test_payment_idempotency.py | 281 ++++++++++++++++++++++++++++++ views_api.py | 176 +++++++++++-------- 2 files changed, 389 insertions(+), 68 deletions(-) create mode 100644 tests/test_payment_idempotency.py diff --git a/tests/test_payment_idempotency.py b/tests/test_payment_idempotency.py new file mode 100644 index 0000000..969107b --- /dev/null +++ b/tests/test_payment_idempotency.py @@ -0,0 +1,281 @@ +"""Lightning payment idempotency — the `processed_payments` claim gate. + +The background invoice listener (`tasks.on_invoice_paid`) and the +client-driven `POST /record-payment` endpoint can both fire for the +same `payment_hash` (queue redelivery after restart, webhook + poller). +The Fava-side duplicate checks are read-then-write races; the local +`processed_payments` primary key makes exactly one claimant win. + +These tests bypass invoice generation (blocked by libra/issues/40) by +delivering synthetic paid `Payment` objects straight to +`on_invoice_paid` and by inserting paid payment rows via the LNbits +core crud for the endpoint tests. +""" +import asyncio +import importlib +from uuid import uuid4 + +import pytest + +from lnbits.core.crud.payments import create_payment +from lnbits.core.models.payments import CreatePayment, Payment, PaymentState + +from .helpers import list_user_entries, post_receivable + +pytestmark = pytest.mark.anyio + + +def _module(name: str): + """Import a libra submodule under whichever path the active LNbits layout + uses (default `lnbits.extensions.libra` or bare `libra`).""" + for prefix in ("lnbits.extensions.libra", "libra"): + try: + return importlib.import_module(f"{prefix}.{name}") + except ModuleNotFoundError: + continue + raise ModuleNotFoundError(f"libra.{name}: tried both import paths") + + +tasks = _module("tasks") +libra_crud = _module("crud") + + +def _paid_payment( + wallet_id: str, + user_id: str, + *, + fiat_amount: str = "100.00", + fiat_currency: str = "EUR", + sats: int = 100_000, +) -> Payment: + payment_hash = uuid4().hex + uuid4().hex[:32] + return Payment( + checking_id=payment_hash, + payment_hash=payment_hash, + wallet_id=wallet_id, + amount=sats * 1000, + fee=0, + bolt11="lnbcfake", + status=PaymentState.SUCCESS, + extra={ + "tag": "libra", + "user_id": user_id, + "fiat_currency": fiat_currency, + "fiat_amount": fiat_amount, + }, + ) + + +async def _setup_receivable( + client, super_user_headers, configured_user, standard_accounts, + amount: str = "100.00", +): + user, wallet = configured_user + await post_receivable( + client, + super_user_headers=super_user_headers, + user_id=user.id, + amount=amount, + description=f"Idempotency setup {uuid4().hex[:6]}", + revenue_account=standard_accounts["revenue_rent"]["name"], + ) + # Force a Fava reload before downstream balance reads (see #37). + await list_user_entries(client, wallet_inkey=wallet.inkey) + return user, wallet + + +async def _entries_with_link(client, wallet_inkey: str, link: str) -> list: + payload = await list_user_entries(client, wallet_inkey=wallet_inkey) + return [ + e for e in payload["entries"] if link in (e.get("links") or []) + ] + + +# --------------------------------------------------------------------------- +# on_invoice_paid — the background listener path +# --------------------------------------------------------------------------- + + +async def test_double_delivery_records_exactly_once( + client, super_user_headers, configured_user, standard_accounts +): + """Same payment delivered twice (queue redelivery) → one ledger entry.""" + user, wallet = await _setup_receivable( + client, super_user_headers, configured_user, standard_accounts + ) + payment = _paid_payment(wallet.id, user.id) + + await tasks.on_invoice_paid(payment) + await tasks.on_invoice_paid(payment) + + link = f"ln-{payment.payment_hash[:16]}" + assert len(await _entries_with_link(client, wallet.inkey, link)) == 1 + + row = await libra_crud.get_processed_payment(payment.payment_hash) + assert row is not None and row["status"] == "done" + + +async def test_failed_recording_releases_claim_and_retry_succeeds( + client, super_user_headers, configured_user, standard_accounts, monkeypatch +): + """A Fava failure mid-write must not permanently block the payment.""" + user, wallet = await _setup_receivable( + client, super_user_headers, configured_user, standard_accounts + ) + payment = _paid_payment(wallet.id, user.id) + + fava_client = _module("fava_client") + fava = fava_client.get_fava_client() + + async def _boom(*args, **kwargs): + raise RuntimeError("fava down") + + monkeypatch.setattr(fava, "add_entry_idempotent", _boom) + with pytest.raises(RuntimeError): + await tasks.on_invoice_paid(payment) + monkeypatch.undo() + + # Claim released → nothing recorded, retry allowed. + assert await libra_crud.get_processed_payment(payment.payment_hash) is None + + await tasks.on_invoice_paid(payment) + row = await libra_crud.get_processed_payment(payment.payment_hash) + assert row is not None and row["status"] == "done" + link = f"ln-{payment.payment_hash[:16]}" + assert len(await _entries_with_link(client, wallet.inkey, link)) == 1 + + +async def test_listener_survives_poison_payment_and_clears_stale_claims( + client, super_user_headers, configured_user, standard_accounts, monkeypatch +): + """One bad payment must not kill the listener; stale 'processing' + claims from a previous process life are cleared at startup.""" + user, wallet = await _setup_receivable( + client, super_user_headers, configured_user, standard_accounts + ) + + # A claim left behind by a "crashed" previous run. + stale_hash = uuid4().hex + uuid4().hex[:32] + assert await libra_crud.claim_payment(stale_hash) + + captured: dict = {} + monkeypatch.setattr( + tasks, + "register_invoice_listener", + lambda queue, name: captured.update(queue=queue), + ) + + listener = asyncio.create_task(tasks.wait_for_paid_invoices()) + try: + for _ in range(50): + if "queue" in captured: + break + await asyncio.sleep(0.05) + assert "queue" in captured, "listener never registered its queue" + + poison = _paid_payment(wallet.id, user.id, fiat_amount="not-a-number") + good = _paid_payment(wallet.id, user.id) + captured["queue"].put_nowait(poison) + captured["queue"].put_nowait(good) + + row = None + for _ in range(100): + row = await libra_crud.get_processed_payment(good.payment_hash) + if row and row["status"] == "done": + break + await asyncio.sleep(0.1) + assert row is not None and row["status"] == "done", ( + "good payment was not recorded after the poison payment" + ) + finally: + listener.cancel() + + # Startup cleared the stale claim; the poison payment's claim was + # released on failure so redelivery could retry it. + assert await libra_crud.get_processed_payment(stale_hash) is None + assert await libra_crud.get_processed_payment(poison.payment_hash) is None + + +# --------------------------------------------------------------------------- +# POST /record-payment — the client-driven path +# --------------------------------------------------------------------------- + + +async def _insert_paid_payment_row(wallet_id: str, user_id: str) -> str: + payment_hash = uuid4().hex + uuid4().hex[:32] + await create_payment( + checking_id=payment_hash, + data=CreatePayment( + wallet_id=wallet_id, + payment_hash=payment_hash, + bolt11="lnbcfake", + amount_msat=100_000_000, + memo="idempotency test", + extra={ + "tag": "libra", + "user_id": user_id, + "fiat_currency": "EUR", + "fiat_amount": "100.00", + }, + ), + status=PaymentState.SUCCESS, + ) + return payment_hash + + +async def test_record_payment_conflicts_while_in_flight( + client, super_user_headers, configured_user, standard_accounts +): + user, wallet = await _setup_receivable( + client, super_user_headers, configured_user, standard_accounts + ) + payment_hash = await _insert_paid_payment_row(wallet.id, user.id) + + # Another claimant (e.g. the background listener) is mid-recording. + assert await libra_crud.claim_payment(payment_hash) + + r = await client.post( + "/libra/api/v1/record-payment", + headers={"X-Api-Key": wallet.inkey}, + json={"payment_hash": payment_hash}, + ) + assert r.status_code == 409, r.text + + # Once that claimant finishes, a replay reports "already recorded" + # instead of writing a second entry. + await libra_crud.mark_payment_done(payment_hash, f"ln-{payment_hash[:16]}") + r = await client.post( + "/libra/api/v1/record-payment", + headers={"X-Api-Key": wallet.inkey}, + json={"payment_hash": payment_hash}, + ) + assert r.status_code == 200, r.text + assert "already recorded" in r.json()["message"].lower() + + +async def test_record_payment_records_once_then_replays_safely( + client, super_user_headers, configured_user, standard_accounts +): + user, wallet = await _setup_receivable( + client, super_user_headers, configured_user, standard_accounts + ) + payment_hash = await _insert_paid_payment_row(wallet.id, user.id) + + r = await client.post( + "/libra/api/v1/record-payment", + headers={"X-Api-Key": wallet.inkey}, + json={"payment_hash": payment_hash}, + ) + assert r.status_code == 200, r.text + assert r.json()["message"] == "Payment recorded successfully" + + r = await client.post( + "/libra/api/v1/record-payment", + headers={"X-Api-Key": wallet.inkey}, + json={"payment_hash": payment_hash}, + ) + assert r.status_code == 200, r.text + assert "already recorded" in r.json()["message"].lower() + + link = f"ln-{payment_hash[:16]}" + assert len(await _entries_with_link(client, wallet.inkey, link)) == 1 diff --git a/views_api.py b/views_api.py index 1b3149a..4c36427 100644 --- a/views_api.py +++ b/views_api.py @@ -1850,90 +1850,130 @@ async def api_record_payment( try: async with httpx.AsyncClient(timeout=5.0) as client: - # Get recent entries from Fava's journal endpoint + # Get recent entries from Fava's journal endpoint. base_url + # already ends in /api — the previous "/api/journal" path + # 404'd, so this duplicate check silently never ran. response = await client.get( - f"{fava.base_url}/api/journal", + f"{fava.base_url}/journal", params={"time": ""} # Get all entries ) + response.raise_for_status() + response_data = response.json() + entries = response_data.get('entries', []) - if response.status_code == 200: - response_data = response.json() - entries = response_data.get('entries', []) - - # Check if any entry has our payment link - for entry in entries: - entry_links = entry.get('links', []) - if link_to_find in entry_links: - # Payment already recorded, return existing entry - balance_data = await fava.get_user_balance_bql(target_user_id) - return { - "journal_entry_id": f"fava-exists-{data.payment_hash[:16]}", - "new_balance": balance_data["balance"], - "message": "Payment already recorded", - } - except Exception as e: + # Check if any entry has our payment link + for entry in entries: + entry_links = entry.get('links', []) + if link_to_find in entry_links: + # Payment already recorded, return existing entry + balance_data = await fava.get_user_balance_bql(target_user_id) + return { + "journal_entry_id": f"fava-exists-{data.payment_hash[:16]}", + "new_balance": balance_data["balance"], + "message": "Payment already recorded", + } + except httpx.HTTPError as e: + # Fail CLOSED: if Fava can't confirm the payment isn't already + # recorded, refuse to write — proceeding on a transient blip is + # how double entries happen. The client can simply retry. logger.warning(f"Could not check Fava for duplicate payment: {e}") - # Continue anyway - Fava/Beancount will catch duplicate if it exists - - # Convert amount from millisatoshis to satoshis - amount_sats = payment.amount // 1000 - - # Extract fiat metadata from invoice (if present) - fiat_currency = None - fiat_amount = None - if payment.extra and isinstance(payment.extra, dict): - logger.info(f"Payment.extra contents: {payment.extra}") - fiat_currency = payment.extra.get("fiat_currency") - fiat_amount_str = payment.extra.get("fiat_amount") - if fiat_amount_str: - from decimal import Decimal - fiat_amount = Decimal(str(fiat_amount_str)) - - logger.info(f"Extracted fiat metadata - currency: {fiat_currency}, amount: {fiat_amount}") - - # Get user's receivable account (what user owes) - user_receivable = await get_or_create_user_account( - target_user_id, AccountType.ASSET, "Accounts Receivable" - ) - - # Get lightning account - lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning") - if not lightning_account: raise HTTPException( - status_code=HTTPStatus.NOT_FOUND, detail="Lightning account not found" + status_code=HTTPStatus.SERVICE_UNAVAILABLE, + detail="Cannot verify payment duplicate status; try again shortly", ) - # Get unsettled receivable entries to link to this settlement - unsettled = await fava.get_unsettled_entries_bql(target_user_id, "receivable") - settled_links = [e["link"] for e in unsettled if e.get("link")] - - # Format payment entry and submit to Fava - entry = format_payment_entry( - user_id=target_user_id, - payment_account=lightning_account.name, - payable_or_receivable_account=user_receivable.name, - amount_sats=amount_sats, - description=f"Lightning payment from user {target_user_id[:8]}", - entry_date=datetime.now().date(), - is_payable=False, # User paying libra (receivable settlement) - fiat_currency=fiat_currency, - fiat_amount=fiat_amount, - payment_hash=data.payment_hash, - reference=data.payment_hash, - settled_entry_links=settled_links + # Local idempotency gate shared with the background invoice listener + # (tasks.on_invoice_paid): exactly one claimant records a payment_hash. + from .crud import ( + claim_payment, + get_processed_payment, + mark_payment_done, + release_payment_claim, ) - logger.info(f"Formatted payment entry: {entry}") + if not await claim_payment(data.payment_hash): + existing = await get_processed_payment(data.payment_hash) + if existing and existing["status"] == "done": + balance_data = await fava.get_user_balance_bql(target_user_id) + return { + "journal_entry_id": existing.get("entry_id") + or f"fava-exists-{data.payment_hash[:16]}", + "new_balance": balance_data["balance"], + "message": "Payment already recorded", + } + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Payment is being recorded; check balance shortly", + ) - # Submit to Fava - result = await fava.add_entry(entry) - logger.info(f"Payment entry submitted to Fava: {result.get('data', 'Unknown')}") + # Convert amount from millisatoshis to satoshis + try: + amount_sats = payment.amount // 1000 + + # Extract fiat metadata from invoice (if present) + fiat_currency = None + fiat_amount = None + if payment.extra and isinstance(payment.extra, dict): + logger.info(f"Payment.extra contents: {payment.extra}") + fiat_currency = payment.extra.get("fiat_currency") + fiat_amount_str = payment.extra.get("fiat_amount") + if fiat_amount_str: + from decimal import Decimal + fiat_amount = Decimal(str(fiat_amount_str)) + + logger.info(f"Extracted fiat metadata - currency: {fiat_currency}, amount: {fiat_amount}") + + # Get user's receivable account (what user owes) + user_receivable = await get_or_create_user_account( + target_user_id, AccountType.ASSET, "Accounts Receivable" + ) + + # Get lightning account + lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning") + if not lightning_account: + raise HTTPException( + status_code=HTTPStatus.NOT_FOUND, detail="Lightning account not found" + ) + + # Get unsettled receivable entries to link to this settlement + unsettled = await fava.get_unsettled_entries_bql(target_user_id, "receivable") + settled_links = [e["link"] for e in unsettled if e.get("link")] + + # Format payment entry and submit to Fava + entry = format_payment_entry( + user_id=target_user_id, + payment_account=lightning_account.name, + payable_or_receivable_account=user_receivable.name, + amount_sats=amount_sats, + description=f"Lightning payment from user {target_user_id[:8]}", + entry_date=datetime.now().date(), + is_payable=False, # User paying libra (receivable settlement) + fiat_currency=fiat_currency, + fiat_amount=fiat_amount, + payment_hash=data.payment_hash, + reference=data.payment_hash, + settled_entry_links=settled_links + ) + + logger.info(f"Formatted payment entry: {entry}") + + # Submit to Fava + result = await fava.add_entry(entry) + logger.info(f"Payment entry submitted to Fava: {result.get('data', 'Unknown')}") + + entry_id = f"ln-{data.payment_hash[:16]}" + await mark_payment_done(data.payment_hash, entry_id) + except BaseException: + # Release the claim so a retry (client or background listener) + # can record this payment. + await release_payment_claim(data.payment_hash) + raise # Get updated balance from Fava balance_data = await fava.get_user_balance_bql(target_user_id) return { - "journal_entry_id": f"fava-{datetime.now().timestamp()}", + "journal_entry_id": entry_id, "new_balance": balance_data["balance"], "message": "Payment recorded successfully", } From cf1a0967bff39975aae699bf354e51f283288a33 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 12:54:46 +0200 Subject: [PATCH 4/8] fix(settlement): per-currency netting, balance guards, Decimal rates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settlement correctness cluster from CODE-REVIEW-2026-06 (#2, #8, #13) plus libra-#38: - format_net_settlement_entry now enforces the same inline balance constraint as the fiat formatter (payment = receivable - payable + credit) and grows an optional credit leg. An unbalanced settlement raises instead of reaching the ledger. - on_invoice_paid settles only what the payment covers: a partial payment clears that much receivable; excess (or a payment with nothing owed) becomes user credit. Previously the full prior balance was cleared against a smaller payment, shipping unbalanced postings. Settlement links are attached only when the payment clears the full open balance, and only for same-currency entries. - get_unsettled_entries_bql returns each entry's real posting currency (was hardcoded "EUR") and exact Decimal amount strings (was float). /receivables/settle nets only entries denominated in the settlement currency. - fiat_rate/btc_rate metadata computed via Decimal (new fiat_rate_metadata helper) instead of float division — cost-basis records no longer carry float drift. - format_posting_at_average_cost omits the cost braces when cost_currency is unset ("SATS {}" is invalid Beancount). - Underpay error payload serializes amounts as exact Decimal strings. - validate_metadata catches decimal.InvalidOperation so bad fiat_amount input becomes ValidationError (libra-#38); flipped the tracking xfail. Co-Authored-By: Claude Fable 5 --- beancount_format.py | 87 +++++++++++++++++++++++---- core/validation.py | 9 ++- fava_client.py | 15 +++-- tasks.py | 71 ++++++++++++++-------- tests/test_settlement_api.py | 11 ++-- tests/test_unit.py | 113 +++++++++++++++++++++++++++++++++-- views_api.py | 38 +++++++----- 7 files changed, 276 insertions(+), 68 deletions(-) diff --git a/beancount_format.py b/beancount_format.py index f233ee5..4cbc5c7 100644 --- a/beancount_format.py +++ b/beancount_format.py @@ -252,9 +252,10 @@ def format_posting_at_average_cost( amount_str = f"{amount_sats} SATS {{{cost_currency}}}" logger.info(f"format_posting_at_average_cost: Generated amount_str='{amount_str}' with cost_currency='{cost_currency}'") else: - # No cost - amount_str = f"{amount_sats} SATS {{}}" - logger.warning(f"format_posting_at_average_cost: cost_currency is None, using empty cost basis") + # No cost basis — omit the braces entirely. Empty "{}" is not + # valid Beancount syntax and fails to parse on ledger load. + amount_str = f"{amount_sats} SATS" + logger.warning(f"format_posting_at_average_cost: cost_currency is None, omitting cost basis") posting_meta = metadata or {} @@ -304,6 +305,25 @@ def format_posting_simple( } +def fiat_rate_metadata(amount_sats: int, fiat_amount: Decimal) -> Dict[str, str]: + """Exchange-rate metadata (sats per fiat unit, fiat per BTC) as exact + Decimal strings. + + These values become the cost-basis record for the entry, so they must + not carry float drift — CLAUDE.md mandates Decimal for all fiat math. + + Returns: + {"fiat_rate": "", "btc_rate": ""} + """ + if amount_sats <= 0 or fiat_amount <= 0: + return {"fiat_rate": "0", "btc_rate": "0"} + fiat_rate = (Decimal(amount_sats) / fiat_amount).quantize(Decimal("0.000001")) + btc_rate = ( + fiat_amount / Decimal(amount_sats) * Decimal(100_000_000) + ).quantize(Decimal("0.01")) + return {"fiat_rate": str(fiat_rate), "btc_rate": str(btc_rate)} + + def format_expense_entry( user_id: str, expense_account: str, @@ -718,15 +738,18 @@ def format_net_settlement_entry( entry_date: date, payment_hash: Optional[str] = None, reference: Optional[str] = None, - settled_entry_links: Optional[List[str]] = None + settled_entry_links: Optional[List[str]] = None, + credit_account: Optional[str] = None, + credit_overflow_fiat: Decimal = Decimal(0), ) -> Dict[str, Any]: """ Format a net settlement payment entry (user paying net balance). - Creates a three-posting transaction: + Creates a three- to four-posting transaction: 1. Lightning payment in SATS with @@ total price notation 2. Clear receivables in EUR 3. Clear payables in EUR + 4. Credit overflow when the payment exceeds what it clears Example: Assets:Bitcoin:Lightning 565251 SATS @@ 517.00 EUR @@ -734,25 +757,61 @@ def format_net_settlement_entry( Liabilities:Payable:User 38.00 EUR = 517 - 555 + 38 = 0 ✓ + Constraint enforced inline (same contract as + `format_fiat_net_settlement_entry`): + net_fiat_amount = total_receivable_fiat - total_payable_fiat + + credit_overflow_fiat + Args: user_id: User ID payment_account: Payment account (e.g., "Assets:Bitcoin:Lightning") receivable_account: User's receivable account payable_account: User's payable account amount_sats: SATS amount paid - net_fiat_amount: Net fiat amount (receivable - payable) - total_receivable_fiat: Total receivables to clear - total_payable_fiat: Total payables to clear + net_fiat_amount: Fiat value of the payment being recorded + total_receivable_fiat: Receivables cleared by this payment + total_payable_fiat: Payables cleared by this payment fiat_currency: Currency (EUR, USD) description: Payment description entry_date: Date of payment payment_hash: Lightning payment hash reference: Optional reference settled_entry_links: List of expense/receivable links being settled (e.g., ["exp-abc123", "rcv-def456"]) + credit_account: User's credit account receiving overflow (required + when credit_overflow_fiat > 0) + credit_overflow_fiat: Payment excess beyond what it clears, absorbed + as a liability libra owes the user going forward Returns: Fava API entry dict + + Raises: + ValueError: if any amount is negative, or the payment doesn't + balance against what it clears — an unbalanced settlement + must never reach the ledger. """ + for label, value in ( + ("net_fiat_amount", net_fiat_amount), + ("total_receivable_fiat", total_receivable_fiat), + ("total_payable_fiat", total_payable_fiat), + ("credit_overflow_fiat", credit_overflow_fiat), + ): + if value < 0: + raise ValueError(f"{label} must be non-negative; got {value}") + + expected_payment = ( + total_receivable_fiat - total_payable_fiat + credit_overflow_fiat + ) + if abs(net_fiat_amount - expected_payment) > Decimal("0.01"): + raise ValueError( + f"net_fiat_amount {net_fiat_amount} does not match expected " + f"{expected_payment} (= receivable {total_receivable_fiat} " + f"- payable {total_payable_fiat} + credit {credit_overflow_fiat}); " + f"refusing to write an unbalanced settlement" + ) + if credit_overflow_fiat > 0 and not credit_account: + raise ValueError("credit_account required when credit_overflow_fiat > 0") + # Build postings for net settlement # Note: We use @@ (total price) syntax for cleaner formatting, but Fava's API # will convert this to @ (per-unit price) with a long decimal when writing to file. @@ -761,20 +820,26 @@ def format_net_settlement_entry( postings = [ { "account": payment_account, - "amount": f"{abs(amount_sats)} SATS @@ {abs(net_fiat_amount):.2f} {fiat_currency}", + "amount": f"{abs(amount_sats)} SATS @@ {net_fiat_amount:.2f} {fiat_currency}", "meta": {"payment-hash": payment_hash} if payment_hash else {} }, { "account": receivable_account, - "amount": f"-{abs(total_receivable_fiat):.2f} {fiat_currency}", + "amount": f"-{total_receivable_fiat:.2f} {fiat_currency}", "meta": {"sats-equivalent": str(abs(amount_sats))} }, { "account": payable_account, - "amount": f"{abs(total_payable_fiat):.2f} {fiat_currency}", + "amount": f"{total_payable_fiat:.2f} {fiat_currency}", "meta": {} } ] + if credit_overflow_fiat > 0: + postings.append({ + "account": credit_account, + "amount": f"-{credit_overflow_fiat:.2f} {fiat_currency}", + "meta": {} + }) entry_meta = { "user-id": user_id, diff --git a/core/validation.py b/core/validation.py index c29f069..913bb8a 100644 --- a/core/validation.py +++ b/core/validation.py @@ -5,7 +5,7 @@ Comprehensive validation following Beancount's plugin system approach, but implemented as simple functions that can be called directly. """ -from decimal import Decimal +from decimal import Decimal, InvalidOperation from typing import Any, Dict, List, Optional @@ -278,11 +278,14 @@ def validate_metadata( } ) - # Validate fiat amount is valid Decimal + # Validate fiat amount is valid Decimal. InvalidOperation is what + # Decimal actually raises on garbage input ("abc") — it is not a + # ValueError subclass, so without it the raw exception leaked to + # callers (libra-#38). if has_fiat_amount: try: Decimal(str(metadata["fiat_amount"])) - except (ValueError, TypeError) as e: + except (ValueError, TypeError, InvalidOperation) as e: raise ValidationError( f"Invalid fiat_amount: {metadata['fiat_amount']}", {"error": str(e)} diff --git a/fava_client.py b/fava_client.py index eaed06b..a7d1703 100644 --- a/fava_client.py +++ b/fava_client.py @@ -1817,7 +1817,7 @@ class FavaClient: # Query 1: Get all original expense/receivable entries for this user # These are entries with the expense-entry or receivable-entry tag original_query = f""" - SELECT date, narration, account, number, weight, links, + SELECT date, narration, account, number, currency, weight, links, any_meta('entry-id') as entry_id WHERE account ~ '{account_pattern}' AND '{entry_tag}' IN tags @@ -1851,7 +1851,10 @@ class FavaClient: entries_by_link: Dict[str, Dict[str, Any]] = {} for row in original_result["rows"]: - date_val, narration, account, number, weight, links, entry_id = row + ( + date_val, narration, account, number, currency, + weight, links, entry_id, + ) = row # Skip if no links if not links or not isinstance(links, list): @@ -1875,9 +1878,11 @@ class FavaClient: if entry_link in entries_by_link: continue - # Parse amounts - fiat_amount = abs(float(number)) if number else 0.0 - fiat_currency = "EUR" # Default, could be extracted from posting + # Parse amounts. The posting's real currency matters: callers + # net these totals per currency, and the old hardcoded "EUR" + # let USD (or SATS-only) entries be summed as if they were EUR. + fiat_amount = str(abs(Decimal(str(number)))) if number else "0" + fiat_currency = currency or "EUR" # Parse SATS from weight column sats_amount = 0 diff --git a/tasks.py b/tasks.py index 158d913..23dd0b5 100644 --- a/tasks.py +++ b/tasks.py @@ -279,24 +279,33 @@ async def on_invoice_paid(payment: Payment) -> None: await release_payment_claim(payment.payment_hash) return - # Get user's current balance to determine receivables and payables + # Get user's current balance to determine what this payment clears balance = await fava.get_user_balance(user_id) fiat_balances = balance.get("fiat_balances", {}) total_fiat_balance = fiat_balances.get(fiat_currency, Decimal(0)) - # Determine receivables and payables based on balance - # Positive balance = user owes libra (receivable) - # Negative balance = libra owes user (payable) - if total_fiat_balance > 0: - # User owes libra - total_receivable = total_fiat_balance - total_payable = Decimal(0) - else: - # Libra owes user - total_receivable = Decimal(0) - total_payable = abs(total_fiat_balance) + # Settle only what this payment covers. The balance is already + # net (positive = user owes libra); a partial payment clears + # that much receivable, and any excess — or the whole payment + # when nothing is owed — becomes credit libra owes the user. + # (Previously partial payments cleared the FULL balance against + # a smaller payment, shipping unbalanced postings.) + tolerance = Decimal("0.01") + open_receivable = ( + total_fiat_balance if total_fiat_balance > 0 else Decimal(0) + ) + total_receivable = min(open_receivable, fiat_amount) + total_payable = Decimal(0) + credit_overflow = fiat_amount - total_receivable + if credit_overflow < tolerance: + # Absorb sub-cent rounding into the receivable leg. + credit_overflow = Decimal(0) + total_receivable = fiat_amount - logger.info(f"Settlement: {fiat_amount} {fiat_currency} (Receivable: {total_receivable}, Payable: {total_payable})") + logger.info( + f"Settlement: {fiat_amount} {fiat_currency} " + f"(clears receivable: {total_receivable}, credit: {credit_overflow})" + ) # Get account names user_receivable = await get_or_create_user_account( @@ -305,23 +314,35 @@ async def on_invoice_paid(payment: Payment) -> None: user_payable = await get_or_create_user_account( user_id, AccountType.LIABILITY, "Accounts Payable" ) + user_credit = None + if credit_overflow > 0: + user_credit = await get_or_create_user_account( + user_id, AccountType.LIABILITY, "Credit" + ) lightning_account = await get_account_by_name("Assets:Bitcoin:Lightning") if not lightning_account: logger.error("Lightning account 'Assets:Bitcoin:Lightning' not found") await release_payment_claim(payment.payment_hash) return - # Query for unsettled entries to link this settlement back to them - # Net settlement can settle both expenses and receivables + # Link the source entries this settlement reconciles — but only + # when the payment clears the full open balance. On a partial + # payment we can't know which entries are covered, and linking + # them would make get_unsettled_entries_bql treat them as + # settled. Only same-currency entries qualify either way. settled_links = [] - try: - unsettled_expenses = await fava.get_unsettled_entries_bql(user_id, "expense") - settled_links.extend([e["link"] for e in unsettled_expenses if e.get("link")]) - unsettled_receivables = await fava.get_unsettled_entries_bql(user_id, "receivable") - settled_links.extend([e["link"] for e in unsettled_receivables if e.get("link")]) - except Exception as e: - logger.warning(f"Could not query unsettled entries for settlement links: {e}") - # Continue without links - settlement will still be recorded + if open_receivable > 0 and fiat_amount + tolerance >= open_receivable: + try: + unsettled_expenses = await fava.get_unsettled_entries_bql(user_id, "expense") + unsettled_receivables = await fava.get_unsettled_entries_bql(user_id, "receivable") + settled_links.extend( + e["link"] + for e in unsettled_expenses + unsettled_receivables + if e.get("link") and e.get("fiat_currency") == fiat_currency + ) + except Exception as e: + logger.warning(f"Could not query unsettled entries for settlement links: {e}") + # Continue without links - settlement will still be recorded # Format as net settlement transaction entry = format_net_settlement_entry( @@ -338,7 +359,9 @@ async def on_invoice_paid(payment: Payment) -> None: entry_date=datetime.now().date(), payment_hash=payment.payment_hash, reference=payment.payment_hash, - settled_entry_links=settled_links if settled_links else None + settled_entry_links=settled_links if settled_links else None, + credit_account=user_credit.name if user_credit else None, + credit_overflow_fiat=credit_overflow, ) # Submit to Fava using idempotent method to prevent duplicates diff --git a/tests/test_settlement_api.py b/tests/test_settlement_api.py index 442a01e..f7de090 100644 --- a/tests/test_settlement_api.py +++ b/tests/test_settlement_api.py @@ -10,6 +10,7 @@ Underpay without explicit entry-picks returns 400 with diff details so the operator can either pay the exact net or specify `settled_entry_links`. """ import importlib +from decimal import Decimal from uuid import uuid4 import pytest @@ -268,10 +269,12 @@ async def test_underpay_without_explicit_links_returns_400( assert r.status_code == 400, f"expected 400, got {r.status_code}: {r.text}" payload = r.json().get("detail") assert isinstance(payload, dict), f"expected structured detail, got {payload!r}" - assert payload.get("cash_paid") == 30.0 - assert payload.get("net_obligation") == 100.0 - assert payload.get("receivable_total") == 100.0 - assert payload.get("payable_total") == 0.0 + # Amounts are exact Decimal strings (not floats) so the operator can + # act on them without precision loss. + assert Decimal(payload.get("cash_paid")) == Decimal("30.00") + assert Decimal(payload.get("net_obligation")) == Decimal("100.00") + assert Decimal(payload.get("receivable_total")) == Decimal("100.00") + assert Decimal(payload.get("payable_total")) == Decimal("0") @pytest.mark.anyio diff --git a/tests/test_unit.py b/tests/test_unit.py index 1c7dabc..11779e9 100644 --- a/tests/test_unit.py +++ b/tests/test_unit.py @@ -553,11 +553,6 @@ def test_validate_metadata_fiat_amount_without_currency_raises(): val.validate_metadata({"fiat_amount": "10.00"}) -@pytest.mark.xfail( - reason="libra/issues/38 — except clause doesn't catch decimal.InvalidOperation, " - "so the raw exception leaks instead of becoming ValidationError. Flip when fixed.", - strict=True, -) def test_validate_metadata_fiat_amount_invalid_decimal_raises(): with pytest.raises(val.ValidationError) as exc: val.validate_metadata({"fiat_amount": "not-a-number", "fiat_currency": "EUR"}) @@ -570,3 +565,111 @@ def test_validate_metadata_both_present_passes(): def test_validate_metadata_neither_present_passes(): val.validate_metadata({"source": "api"}) + + +# --------------------------------------------------------------------------- +# format_net_settlement_entry — balance guard + credit overflow +# --------------------------------------------------------------------------- + + +def _net_settlement(**overrides): + kwargs = dict( + user_id="abc12345", + payment_account="Assets:Bitcoin:Lightning", + receivable_account="Assets:Receivable:User-abc12345", + payable_account="Liabilities:Payable:User-abc12345", + amount_sats=565251, + net_fiat_amount=Decimal("517.00"), + total_receivable_fiat=Decimal("555.00"), + total_payable_fiat=Decimal("38.00"), + fiat_currency="EUR", + description="test settlement", + entry_date=date(2026, 7, 12), + payment_hash="ff" * 32, + ) + kwargs.update(overrides) + return bf.format_net_settlement_entry(**kwargs) + + +def test_net_settlement_balanced_passes(): + entry = _net_settlement() + amounts = [p["amount"] for p in entry["postings"]] + assert any("@@ 517.00 EUR" in a for a in amounts) + assert "-555.00 EUR" in amounts + assert "38.00 EUR" in amounts + + +def test_net_settlement_unbalanced_partial_payment_raises(): + # Payment of 300 can't clear a 555 receivable net of 38 payable — + # this is the pre-fix partial-payment shape that shipped unbalanced + # postings to the ledger. + with pytest.raises(ValueError, match="unbalanced"): + _net_settlement(net_fiat_amount=Decimal("300.00")) + + +def test_net_settlement_negative_amount_raises(): + with pytest.raises(ValueError, match="non-negative"): + _net_settlement(total_receivable_fiat=Decimal("-1.00")) + + +def test_net_settlement_credit_overflow_adds_leg(): + entry = _net_settlement( + net_fiat_amount=Decimal("600.00"), + credit_account="Liabilities:Credit:User-abc12345", + credit_overflow_fiat=Decimal("83.00"), + ) + amounts = [p["amount"] for p in entry["postings"]] + assert "-83.00 EUR" in amounts + + +def test_net_settlement_credit_overflow_without_account_raises(): + with pytest.raises(ValueError, match="credit_account"): + _net_settlement( + net_fiat_amount=Decimal("600.00"), + credit_overflow_fiat=Decimal("83.00"), + ) + + +# --------------------------------------------------------------------------- +# format_posting_at_average_cost — no empty cost braces +# --------------------------------------------------------------------------- + + +def test_average_cost_posting_without_currency_omits_braces(): + posting = bf.format_posting_at_average_cost( + account="Assets:Receivable:User-abc", amount_sats=-996896, + ) + assert posting["amount"] == "-996896 SATS" + assert "{" not in posting["amount"] + + +def test_average_cost_posting_with_currency_keeps_braces(): + posting = bf.format_posting_at_average_cost( + account="Assets:Receivable:User-abc", + amount_sats=-996896, + cost_currency="EUR", + ) + assert posting["amount"] == "-996896 SATS {EUR}" + + +# --------------------------------------------------------------------------- +# fiat_rate_metadata — Decimal-exact rate strings +# --------------------------------------------------------------------------- + + +def test_fiat_rate_metadata_is_exact(): + meta = bf.fiat_rate_metadata(107419, Decimal("100.00")) + assert meta["fiat_rate"] == "1074.190000" + assert meta["btc_rate"] == "93093.40" + # No float artifacts like 1074.1899999999998 + Decimal(meta["fiat_rate"]) + Decimal(meta["btc_rate"]) + + +def test_fiat_rate_metadata_zero_amounts(): + assert bf.fiat_rate_metadata(0, Decimal("100.00")) == { + "fiat_rate": "0", "btc_rate": "0", + } + assert bf.fiat_rate_metadata(1000, Decimal("0")) == { + "fiat_rate": "0", "btc_rate": "0", + } diff --git a/views_api.py b/views_api.py index 4c36427..2188d7a 100644 --- a/views_api.py +++ b/views_api.py @@ -13,6 +13,7 @@ from lnbits.decorators import ( ) from lnbits.utils.exchange_rates import allowed_currencies, fiat_amount_as_satoshis +from .beancount_format import fiat_rate_metadata from .crud import ( approve_manual_payment_request, check_balance_assertion, @@ -1074,8 +1075,7 @@ async def api_create_expense_entry( metadata = { "fiat_currency": data.currency.upper(), "fiat_amount": str(data.amount.quantize(Decimal("0.001"))), # Store as string with 3 decimal places - "fiat_rate": float(amount_sats) / float(data.amount) if data.amount > 0 else 0, - "btc_rate": float(data.amount) / float(amount_sats) * 100_000_000 if amount_sats > 0 else 0, + **fiat_rate_metadata(amount_sats, data.amount), } # Get or create expense account @@ -1272,8 +1272,7 @@ async def api_create_income_entry( metadata = { "fiat_currency": fiat_currency, "fiat_amount": str(data.amount.quantize(Decimal("0.001"))), - "fiat_rate": float(amount_sats) / float(data.amount) if data.amount > 0 else 0, - "btc_rate": float(data.amount) / float(amount_sats) * 100_000_000 if amount_sats > 0 else 0, + **fiat_rate_metadata(amount_sats, data.amount), } # Submit to Fava @@ -1371,8 +1370,7 @@ async def api_create_receivable_entry( metadata = { "fiat_currency": data.currency.upper(), "fiat_amount": str(data.amount.quantize(Decimal("0.001"))), # Store as string with 3 decimal places - "fiat_rate": float(amount_sats) / float(data.amount) if data.amount > 0 else 0, - "btc_rate": float(data.amount) / float(amount_sats) * 100_000_000 if amount_sats > 0 else 0, + **fiat_rate_metadata(amount_sats, data.amount), } # Get or create revenue account @@ -1760,15 +1758,10 @@ async def api_generate_payment_invoice( proportion = Decimal(data.amount) / Decimal(total_sat_balance) invoice_fiat_amount = abs(total_fiat_balance) * proportion - # Calculate fiat rate (sats per fiat unit) - fiat_rate = float(data.amount) / float(invoice_fiat_amount) if invoice_fiat_amount > 0 else 0 - btc_rate = float(invoice_fiat_amount) / float(data.amount) * 100_000_000 if data.amount > 0 else 0 - invoice_extra.update({ "fiat_currency": fiat_currency, "fiat_amount": str(invoice_fiat_amount.quantize(Decimal("0.001"))), - "fiat_rate": fiat_rate, - "btc_rate": btc_rate, + **fiat_rate_metadata(data.amount, invoice_fiat_amount), }) logger.info(f"Invoice extra metadata: {invoice_extra}") @@ -2064,6 +2057,19 @@ async def api_settle_receivable( unsettled_payables = await fava.get_unsettled_entries_bql(data.user_id, "expense") unsettled_receivables = await fava.get_unsettled_entries_bql(data.user_id, "receivable") + # Net only entries denominated in the settlement currency — summing + # mixed currencies as if they were one silently mis-states the net + # obligation and links entries this settlement doesn't actually clear. + settle_currency = data.currency.upper() + unsettled_payables = [ + e for e in unsettled_payables + if e.get("fiat_currency") == settle_currency + ] + unsettled_receivables = [ + e for e in unsettled_receivables + if e.get("fiat_currency") == settle_currency + ] + payable_total = sum( (Decimal(str(e["fiat_amount"])) for e in unsettled_payables), Decimal(0), @@ -2107,10 +2113,10 @@ async def api_settle_receivable( "net to clear all open entries, or pass " "`settled_entry_links` to settle a specific subset." ), - "cash_paid": float(cash_paid), - "net_obligation": float(net_obligation), - "receivable_total": float(receivable_total), - "payable_total": float(payable_total), + "cash_paid": str(cash_paid), + "net_obligation": str(net_obligation), + "receivable_total": str(receivable_total), + "payable_total": str(payable_total), "currency": data.currency.upper(), }, ) From c0c8acbe30ec167ee078f415e263d01a2cb26ac0 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 15:42:44 +0200 Subject: [PATCH 5/8] fix(assertions): send Balance directives in Fava's JSON shape (libra-#39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit format_balance returned a Beancount source string, but fava.add_entry feeds PUT /add_entries whose deserialiser expects {"t": "Balance", "amount": {"number", "currency"}, ...} — every assertion create 500'd. Returns the dict shape now. The seven strict-xfail reconciliation tests tracking this flip to regular passing tests. Co-Authored-By: Claude Fable 5 --- beancount_format.py | 25 +++++++++++++++---------- tests/test_reconciliation_api.py | 20 -------------------- 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/beancount_format.py b/beancount_format.py index 4cbc5c7..fbdfba9 100644 --- a/beancount_format.py +++ b/beancount_format.py @@ -115,13 +115,18 @@ def format_balance( account: str, amount: int, currency: str = "SATS" -) -> str: +) -> Dict[str, Any]: """ - Format a balance assertion directive for Beancount. + Format a balance assertion directive for Fava's JSON API. Balance assertions verify that an account has an expected balance on a specific date. They are checked automatically by Beancount when the file is loaded. + Fava's `deserialise` (fava/serialisation.py) expects + `{"t": "Balance", "amount": {"number", "currency"}, ...}` — the + previous source-string return 500'd on every assertion create + (libra-#39). + Args: date_val: Date of the balance assertion account: Account name (e.g., "Assets:Bitcoin:Lightning") @@ -129,15 +134,15 @@ def format_balance( currency: Currency code (default: "SATS") Returns: - Beancount balance directive as a string - - Example: - >>> format_balance(date(2025, 11, 10), "Assets:Bitcoin:Lightning", 1500000, "SATS") - '2025-11-10 balance Assets:Bitcoin:Lightning 1500000 SATS' + Fava API Balance entry dict ready for `fava.add_entry`. """ - date_str = date_val.strftime('%Y-%m-%d') - # Two spaces between account and amount (Beancount convention) - return f"{date_str} balance {account} {amount} {currency}" + return { + "t": "Balance", + "date": date_val.strftime('%Y-%m-%d'), + "account": account, + "amount": {"number": str(amount), "currency": currency}, + "meta": {}, + } def format_posting_with_cost( diff --git a/tests/test_reconciliation_api.py b/tests/test_reconciliation_api.py index 66757be..a168ff8 100644 --- a/tests/test_reconciliation_api.py +++ b/tests/test_reconciliation_api.py @@ -18,19 +18,6 @@ from uuid import uuid4 import pytest -# Tests that try to actually create + check an assertion all hit issue #39: -# `format_balance` returns a Beancount source string but `fava.add_entry` -# expects a dict, so Fava 500s on every assertion-create call. The contract -# violation is on libra's side; mark these strict-xfail so they go green -# automatically once #39 lands and the format_balance return shape is fixed. -ASSERTION_CREATE_BROKEN = pytest.mark.xfail( - reason="libra/issues/39 — POST /assertions submits a Beancount source string " - "to Fava's JSON API and 500s. Drop this marker when the format_balance " - "return type is changed to a dict.", - strict=True, -) - - # --------------------------------------------------------------------------- # helpers (local — assertion endpoints don't have wrapper helpers yet) # --------------------------------------------------------------------------- @@ -58,7 +45,6 @@ async def _create_assertion( # --------------------------------------------------------------------------- -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_assertion_against_empty_account_passes( client, super_user_headers, standard_accounts, @@ -79,7 +65,6 @@ async def test_assertion_against_empty_account_passes( assert body.get("difference_sats", 0) == 0 -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_assertion_with_wrong_balance_returns_409( client, super_user_headers, standard_accounts, @@ -102,7 +87,6 @@ async def test_assertion_with_wrong_balance_returns_409( assert detail.get("difference_sats") == 999_999 or detail.get("difference_sats") == -999_999 -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_assertion_with_tolerance_accepts_small_diff( client, super_user_headers, standard_accounts, @@ -119,7 +103,6 @@ async def test_assertion_with_tolerance_accepts_small_diff( assert r.json().get("status") == "passed" -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_list_assertions_returns_created( client, super_user_headers, standard_accounts, @@ -145,7 +128,6 @@ async def test_list_assertions_returns_created( assert assertion_id in ids, f"created assertion {assertion_id} missing from list {ids}" -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_get_assertion_by_id( client, super_user_headers, standard_accounts, @@ -167,7 +149,6 @@ async def test_get_assertion_by_id( assert r.json().get("id") == assertion_id -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_recheck_assertion_via_check_endpoint( client, super_user_headers, standard_accounts, @@ -190,7 +171,6 @@ async def test_recheck_assertion_via_check_endpoint( assert r.json().get("status") == "passed" -@ASSERTION_CREATE_BROKEN @pytest.mark.anyio async def test_delete_assertion_removes_it( client, super_user_headers, standard_accounts, From 4d63e08a692a045c69a3e0a445a8f8a285e8c384 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 15:42:44 +0200 Subject: [PATCH 6/8] fix(fava): serialize source mutations, share HTTP client, validate BQL input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fava-client hardening cluster (CODE-REVIEW-2026-06 #7, #14, #15, #19 + libra-#23, libra-#53): - New FavaClient.transform_source_line does the whole read-checksum-modify-write under the global write lock and maps Fava 409/412 to ChecksumConflictError. The approve and reject endpoints used to do this dance with raw httpx and no lock — two concurrent mutations raced each other and every other ledger writer (libra-#23). They now route through the new method and translate conflicts to HTTP 409. - update_entry_source / delete_entry raise ChecksumConflictError on 409/412 instead of leaking raw HTTPStatusError. - One shared httpx.AsyncClient per FavaClient (12 per-call instantiations removed — no more TCP handshake per request); closed via libra_stop. Health probes keep their 2s timeout per-request. - Account names/patterns are validated against ^[A-Za-z0-9:_-]+$ before interpolation into BQL string literals. - The posting amount regexes are consolidated into module-level compiled patterns, all decimal-tolerant — the old integer-only SATS pattern silently dropped decimal-SATS postings (Fava's @@->@ normalisation emits them) from balances. - add-account no longer verifies its own write with a second serialized get_all_accounts round-trip (libra-#53): sync_single_account_from_beancount grows an assume_exists path. New test: concurrent approve+reject must both land (was lost-update/412 before the lock). Co-Authored-By: Claude Fable 5 --- __init__.py | 11 ++ account_sync.py | 29 +++++- fava_client.py | 182 ++++++++++++++++++++++++++++------ tests/test_void_reject_api.py | 66 ++++++++++++ views_api.py | 140 +++++++++----------------- 5 files changed, 305 insertions(+), 123 deletions(-) diff --git a/__init__.py b/__init__.py index 614a8ba..42a2648 100644 --- a/__init__.py +++ b/__init__.py @@ -30,6 +30,17 @@ def libra_stop(): except Exception as ex: logger.warning(ex) + # Close the Fava client's shared HTTP connection pool. libra_stop is + # synchronous, so schedule the close; if the loop is already gone the + # sockets die with the process anyway. + from .fava_client import _fava_client + + if _fava_client is not None: + try: + asyncio.get_event_loop().create_task(_fava_client.aclose()) + except Exception as ex: + logger.warning(f"Could not close Fava HTTP client: {ex}") + def libra_start(): """Initialize Libra extension background tasks""" diff --git a/account_sync.py b/account_sync.py index 3d82381..870d1d7 100644 --- a/account_sync.py +++ b/account_sync.py @@ -285,7 +285,11 @@ async def sync_accounts_from_beancount(force_full_sync: bool = False) -> dict: return stats -async def sync_single_account_from_beancount(account_name: str) -> bool: +async def sync_single_account_from_beancount( + account_name: str, + description: Optional[str] = None, + assume_exists: bool = False, +) -> bool: """ Sync a single account from Beancount to Libra DB. @@ -294,6 +298,13 @@ async def sync_single_account_from_beancount(account_name: str) -> bool: Args: account_name: Hierarchical account name (e.g., "Expenses:Food") + description: Description for the Libra DB row (only used with + assume_exists — otherwise read from Beancount metadata) + assume_exists: Skip the Fava existence lookup. Pass when the + caller just wrote the Open directive itself — verifying via + a second serialized get_all_accounts round-trip doubles the + latency of every account create for no information gain + (libra-#53). Returns: True if account was created/updated, False if it already existed or failed @@ -306,6 +317,22 @@ async def sync_single_account_from_beancount(account_name: str) -> bool: logger.debug(f"Account already exists: {account_name}") return False + if assume_exists: + try: + await create_account( + CreateAccount( + name=account_name, + account_type=infer_account_type_from_name(account_name), + description=description, + user_id=extract_user_id_from_account_name(account_name), + ) + ) + logger.info(f"Created account (writer-asserted): {account_name}") + return True + except Exception as e: + logger.error(f"Failed to sync account {account_name}: {e}") + return False + # Get from Beancount fava = get_fava_client() try: diff --git a/fava_client.py b/fava_client.py index a7d1703..faad8f7 100644 --- a/fava_client.py +++ b/fava_client.py @@ -20,7 +20,8 @@ See: https://github.com/beancount/fava/blob/main/src/fava/json_api.py import asyncio import re import httpx -from typing import Any, Dict, List, Optional +from contextlib import asynccontextmanager +from typing import Any, AsyncIterator, Callable, Dict, List, Optional from decimal import Decimal from datetime import date, datetime from loguru import logger @@ -44,6 +45,34 @@ def _infer_target_file(account_name: str) -> str: return "accounts/chart.beancount" +# Posting amount-string patterns shared by the balance parsers. Fava's +# @@ → @ normalisation can emit decimal SATS values, so every SATS group +# must tolerate decimals (the old integer-only pattern silently dropped +# those postings from balances). +_TOTAL_PRICE_RE = re.compile(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@@\s+(-?[\d.]+)\s+SATS$') +_UNIT_PRICE_RE = re.compile(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@\s+([\d.]+)\s+SATS$') +_FIAT_AMOUNT_RE = re.compile(r'^(-?[\d.]+)\s+([A-Z]{3})$') +_SATS_AMOUNT_RE = re.compile(r'^(-?[\d.]+)\s+SATS') + + +def _sats_to_int(value: str) -> int: + """Parse a (possibly decimal) SATS amount string to whole sats.""" + return int(Decimal(value)) + + +# Account names/patterns are interpolated into BQL string literals; restrict +# them to Beancount account characters so caller-supplied input can't break +# out of the quoted literal. +_BQL_ACCOUNT_RE = re.compile(r'^[A-Za-z0-9:_-]+$') + + +def _validate_bql_account(value: str) -> str: + """Validate a value bound for interpolation into a BQL string literal.""" + if not _BQL_ACCOUNT_RE.match(value): + raise ValueError(f"Invalid account name for BQL query: {value!r}") + return value + + def _escape_beancount_string(value: str) -> str: """Escape a value for safe inclusion in a Beancount string literal. @@ -136,6 +165,27 @@ class FavaClient: self._main_dir_cache: Optional[str] = None self._main_dir_lock = asyncio.Lock() + # Shared HTTP client, created lazily on first use. One client + # means one connection pool instead of a TCP handshake per call. + self._http: Optional[httpx.AsyncClient] = None + + @asynccontextmanager + async def _client(self) -> AsyncIterator[httpx.AsyncClient]: + """Yield the shared HTTP client (lazily created). + + Kept as a context manager so call sites read the same as the + per-call clients they replace; the client itself is NOT closed on + exit — call `aclose()` at extension shutdown. + """ + if self._http is None or self._http.is_closed: + self._http = httpx.AsyncClient(timeout=self.timeout) + yield self._http + + async def aclose(self) -> None: + """Close the shared HTTP client (extension shutdown).""" + if self._http is not None and not self._http.is_closed: + await self._http.aclose() + async def _resolve_target_file(self, target_file: str) -> str: """ Turn a relative include path into the absolute path fava expects. @@ -160,7 +210,7 @@ class FavaClient: if self._main_dir_cache is None: async with self._main_dir_lock: if self._main_dir_cache is None: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: resp = await client.get(f"{self.base_url}/options") resp.raise_for_status() main_file = resp.json()["data"]["beancount_options"]["filename"] @@ -236,7 +286,7 @@ class FavaClient: # Acquire global write lock to serialize ledger modifications async with self._write_lock: try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.put( f"{self.base_url}/add_entries", json={"entries": [entry]}, @@ -350,10 +400,11 @@ class FavaClient: # Use sum(weight) for SATS and sum(number) for fiat # Note: BQL doesn't support != operator, so use flag = '*' to exclude pending + _validate_bql_account(account_name) query = f"SELECT sum(number), sum(weight) WHERE account = '{account_name}' AND flag = '*'" try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.get( f"{self.base_url}/query", params={"query_string": query} @@ -446,14 +497,14 @@ class FavaClient: import re # Try total price notation: "50.00 EUR @@ 50000 SATS" - total_price_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@@\s+(-?\d+)\s+SATS$', amount_str) + total_price_match = _TOTAL_PRICE_RE.match(amount_str) # Try per-unit price notation: "50.00 EUR @ 1000.5 SATS" - unit_price_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@\s+([\d.]+)\s+SATS$', amount_str) + unit_price_match = _UNIT_PRICE_RE.match(amount_str) if total_price_match: fiat_amount = Decimal(total_price_match.group(1)) fiat_currency = total_price_match.group(2) - sats_amount = int(total_price_match.group(3)) + sats_amount = _sats_to_int(total_price_match.group(3)) if fiat_currency not in fiat_balances: fiat_balances[fiat_currency] = Decimal(0) @@ -480,8 +531,8 @@ class FavaClient: accounts_dict[account_name]["sats"] += sats_amount # Try simple fiat format: "50.00 EUR" (check metadata for sats) - elif re.match(r'^(-?[\d.]+)\s+([A-Z]{3})$', amount_str): - fiat_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})$', amount_str) + elif _FIAT_AMOUNT_RE.match(amount_str): + fiat_match = _FIAT_AMOUNT_RE.match(amount_str) if fiat_match and fiat_match.group(2) in ('EUR', 'USD', 'GBP'): fiat_amount = Decimal(fiat_match.group(1)) fiat_currency = fiat_match.group(2) @@ -502,9 +553,9 @@ class FavaClient: else: # Old format: SATS with cost/price notation - extract SATS amount - sats_match = re.match(r'^(-?\d+)\s+SATS', amount_str) + sats_match = _SATS_AMOUNT_RE.match(amount_str) if sats_match: - sats_amount = int(sats_match.group(1)) + sats_amount = _sats_to_int(sats_match.group(1)) total_sats += sats_amount # Track per account @@ -603,14 +654,14 @@ class FavaClient: import re # Try total price notation: "50.00 EUR @@ 50000 SATS" - total_price_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@@\s+(-?\d+)\s+SATS$', amount_str) + total_price_match = _TOTAL_PRICE_RE.match(amount_str) # Try per-unit price notation: "50.00 EUR @ 1000.5 SATS" - unit_price_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})\s+@\s+([\d.]+)\s+SATS$', amount_str) + unit_price_match = _UNIT_PRICE_RE.match(amount_str) if total_price_match: fiat_amount = Decimal(total_price_match.group(1)) fiat_currency = total_price_match.group(2) - sats_amount = int(total_price_match.group(3)) + sats_amount = _sats_to_int(total_price_match.group(3)) if fiat_currency not in user_data[user_id]["fiat_balances"]: user_data[user_id]["fiat_balances"][fiat_currency] = Decimal(0) @@ -629,8 +680,8 @@ class FavaClient: user_data[user_id]["balance"] += sats_amount # Try simple fiat format: "50.00 EUR" (check metadata for sats) - elif re.match(r'^(-?[\d.]+)\s+([A-Z]{3})$', amount_str): - fiat_match = re.match(r'^(-?[\d.]+)\s+([A-Z]{3})$', amount_str) + elif _FIAT_AMOUNT_RE.match(amount_str): + fiat_match = _FIAT_AMOUNT_RE.match(amount_str) if fiat_match and fiat_match.group(2) in ('EUR', 'USD', 'GBP'): fiat_amount = Decimal(fiat_match.group(1)) fiat_currency = fiat_match.group(2) @@ -648,9 +699,9 @@ class FavaClient: else: # Old format: SATS with cost/price notation - sats_match = re.match(r'^(-?\d+)\s+SATS', amount_str) + sats_match = _SATS_AMOUNT_RE.match(amount_str) if sats_match: - sats_amount = int(sats_match.group(1)) + sats_amount = _sats_to_int(sats_match.group(1)) user_data[user_id]["balance"] += sats_amount # Extract fiat from cost syntax or metadata (backward compatibility) @@ -683,9 +734,12 @@ class FavaClient: True if Fava responds, False otherwise """ try: - async with httpx.AsyncClient(timeout=2.0) as client: + async with self._client() as client: + # Health probes stay fast regardless of the configured + # request timeout. response = await client.get( - f"{self.base_url}/changed" + f"{self.base_url}/changed", + timeout=2.0 ) return response.status_code == 200 except Exception as e: @@ -721,12 +775,13 @@ class FavaClient: """ # Build Beancount query if account_pattern: + _validate_bql_account(account_pattern) query = f"SELECT * WHERE account ~ '{account_pattern}' ORDER BY date DESC LIMIT {limit}" else: query = f"SELECT * ORDER BY date DESC LIMIT {limit}" try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.get( f"{self.base_url}/query", params={"query_string": query} @@ -807,7 +862,7 @@ class FavaClient: https://beancount.github.io/docs/beancount_query_language.html """ try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.get( f"{self.base_url}/query", params={"query_string": query_string} @@ -1341,7 +1396,7 @@ class FavaClient: # (BQL's SELECT DISTINCT account only returns accounts with postings) account_names: set[str] = set() - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: for endpoint in ("balance_sheet", "income_statement"): try: response = await client.get(f"{self.base_url}/{endpoint}") @@ -1440,7 +1495,7 @@ class FavaClient: params["time"] = f"{cutoff_date.isoformat()} - {today.isoformat()}" logger.info(f"Querying journal for last {days} days (from {cutoff_date})") - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.get(f"{self.base_url}/journal", params=params) response.raise_for_status() result = response.json() @@ -1482,7 +1537,7 @@ class FavaClient: sha256sum = context["sha256sum"] """ try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.get( f"{self.base_url}/context", params={"entry_hash": entry_hash} @@ -1529,7 +1584,7 @@ class FavaClient: # Acquire global write lock to serialize ledger modifications async with self._write_lock: try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.put( f"{self.base_url}/source_slice", json={ @@ -1544,11 +1599,78 @@ class FavaClient: except httpx.HTTPStatusError as e: logger.error(f"Fava update error: {e.response.status_code} - {e.response.text}") + if e.response.status_code in (409, 412): + raise ChecksumConflictError( + f"Entry {entry_hash} changed concurrently" + ) from e raise except httpx.RequestError as e: logger.error(f"Fava connection error: {e}") raise + async def transform_source_line( + self, + filename: str, + lineno: int, + transform: Callable[[str], str], + ) -> bool: + """Atomically read-modify-write one line of a ledger source file. + + Holds the global write lock across the whole read-modify-write, so + another writer can't slip in between the checksum read and the + write (libra-#23: the approve/reject endpoints used to do this + read-then-write with raw httpx and no lock). + + The transform receives the current line and returns the new one; + returning it unchanged skips the write. + + Returns: + True when the line was changed and written, False on a no-op. + + Raises: + ValueError: lineno is outside the file. + ChecksumConflictError: an out-of-process writer changed the + file between read and write (409/412 from Fava). + """ + async with self._write_lock: + async with self._client() as client: + response = await client.get( + f"{self.base_url}/source", + params={"filename": filename}, + ) + response.raise_for_status() + source_data = response.json()["data"] + sha256sum = source_data["sha256sum"] + lines = source_data["source"].split("\n") + + idx = lineno - 1 + if idx < 0 or idx >= len(lines): + raise ValueError(f"Line {lineno} not found in {filename}") + + new_line = transform(lines[idx]) + if new_line == lines[idx]: + return False + lines[idx] = new_line + + try: + update = await client.put( + f"{self.base_url}/source", + json={ + "file_path": filename, + "source": "\n".join(lines), + "sha256sum": sha256sum, + }, + headers={"Content-Type": "application/json"}, + ) + update.raise_for_status() + except httpx.HTTPStatusError as e: + if e.response.status_code in (409, 412): + raise ChecksumConflictError( + f"{filename} changed concurrently" + ) from e + raise + return True + async def delete_entry(self, entry_hash: str, sha256sum: str) -> str: """ Delete an entry from the Beancount file. @@ -1571,7 +1693,7 @@ class FavaClient: # Acquire global write lock to serialize ledger modifications async with self._write_lock: try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: response = await client.delete( f"{self.base_url}/source_slice", params={ @@ -1585,6 +1707,10 @@ class FavaClient: except httpx.HTTPStatusError as e: logger.error(f"Fava delete error: {e.response.status_code} - {e.response.text}") + if e.response.status_code in (409, 412): + raise ChecksumConflictError( + f"Entry {entry_hash} changed concurrently" + ) from e raise except httpx.RequestError as e: logger.error(f"Fava connection error: {e}") @@ -1664,7 +1790,7 @@ class FavaClient: # Acquire global write lock to serialize ledger modifications async with self._write_lock: try: - async with httpx.AsyncClient(timeout=self.timeout) as client: + async with self._client() as client: # Step 1: Get current source file (fresh read on each attempt) response = await client.get( f"{self.base_url}/source", diff --git a/tests/test_void_reject_api.py b/tests/test_void_reject_api.py index 66e2180..0f2b817 100644 --- a/tests/test_void_reject_api.py +++ b/tests/test_void_reject_api.py @@ -210,3 +210,69 @@ async def test_double_reject_returns_404_on_second_call( assert r.status_code in (200, 404), ( f"second reject should be deterministic, got {r.status_code}: {r.text}" ) + + +@pytest.mark.anyio +async def test_concurrent_approve_and_reject_are_serialized( + client, super_user_headers, configured_user, standard_accounts, +): + """Two mutations of the same ledger source file fired concurrently must + BOTH land. Before libra-#23 each endpoint did its own read-modify-write + with raw httpx and no lock, so one writer overwrote the other's change + (or 412'd on the stale checksum). Now both route through + FavaClient.transform_source_line under the global write lock. + """ + import asyncio + + _, wallet = configured_user + approve_tag = f"conc-approve-{uuid4().hex[:6]}" + reject_tag = f"conc-reject-{uuid4().hex[:6]}" + + posted = {} + for tag in (approve_tag, reject_tag): + posted[tag] = await post_expense( + client, + wallet_inkey=wallet.inkey, + user_wallet_id=wallet.id, + amount="10.00", + currency="EUR", + description=tag, + expense_account=standard_accounts["expense_food"]["name"], + ) + + # Force a Fava reload so the approve/reject lookups see both fresh + # pending entries (see #37). + await list_user_entries(client, wallet_inkey=wallet.inkey) + + r_approve, r_reject = await asyncio.gather( + client.post( + f"/libra/api/v1/entries/{posted[approve_tag]['id']}/approve", + headers=super_user_headers, + ), + client.post( + f"/libra/api/v1/entries/{posted[reject_tag]['id']}/reject", + headers=super_user_headers, + ), + ) + assert r_approve.status_code == 200, f"approve: {r_approve.text}" + assert r_reject.status_code == 200, f"reject: {r_reject.text}" + + # Both mutations must be visible: one entry voided, the other cleared + # (a cleared entry no longer matches the pending-only reject lookup). + listing = await list_user_entries(client, wallet_inkey=wallet.inkey) + entries = listing.get("entries", []) + rejected = next( + (e for e in entries if reject_tag in (e.get("description") or "")), None, + ) + assert rejected is not None and "voided" in rejected.get("tags", []), ( + f"rejected entry lost its #voided tag: {rejected}" + ) + + second_reject = await client.post( + f"/libra/api/v1/entries/{posted[approve_tag]['id']}/reject", + headers=super_user_headers, + ) + assert second_reject.status_code == 404, ( + "approved entry should no longer match the pending-only reject " + f"lookup, got {second_reject.status_code}" + ) diff --git a/views_api.py b/views_api.py index 2188d7a..1c3269f 100644 --- a/views_api.py +++ b/views_api.py @@ -2903,8 +2903,7 @@ async def api_approve_expense_entry( This updates the transaction in the Beancount file via Fava API. """ - import httpx - from .fava_client import get_fava_client + from .fava_client import ChecksumConflictError, get_fava_client fava = get_fava_client() @@ -2939,57 +2938,29 @@ async def api_approve_expense_entry( detail="Entry metadata missing filename or lineno" ) - # 3. Get the source file from Fava - async with httpx.AsyncClient(timeout=fava.timeout) as client: - response = await client.get( - f"{fava.base_url}/source", - params={"filename": filename} - ) - response.raise_for_status() - source_data = response.json()["data"] + # 3. Flip the flag under FavaClient's write lock — the whole + # read-modify-write is atomic against every other ledger writer. + old_pattern = f"{date_str} !" - sha256sum = source_data["sha256sum"] - source = source_data["source"] - lines = source.split('\n') - - # 4. Find and modify the entry at the specified line - # Line numbers are 1-indexed, list is 0-indexed - entry_line_idx = lineno - 1 - - if entry_line_idx >= len(lines): + def _approve(line: str) -> str: + if old_pattern not in line: raise HTTPException( status_code=HTTPStatus.INTERNAL_SERVER_ERROR, - detail=f"Line {lineno} not found in source file" + detail=f"Line {lineno} does not contain expected pattern '{old_pattern}'. Found: {line}" ) + return line.replace(old_pattern, f"{date_str} *", 1) - entry_line = lines[entry_line_idx] - - # Check if the line contains the pending flag pattern - old_pattern = f"{date_str} !" - if old_pattern not in entry_line: - raise HTTPException( - status_code=HTTPStatus.INTERNAL_SERVER_ERROR, - detail=f"Line {lineno} does not contain expected pattern '{old_pattern}'. Found: {entry_line}" - ) - - # Replace the flag - new_pattern = f"{date_str} *" - new_line = entry_line.replace(old_pattern, new_pattern, 1) - lines[entry_line_idx] = new_line - - # 5. Write back the modified source - new_source = '\n'.join(lines) - - update_response = await client.put( - f"{fava.base_url}/source", - json={ - "file_path": filename, - "source": new_source, - "sha256sum": sha256sum - }, - headers={"Content-Type": "application/json"} + try: + await fava.transform_source_line(filename, lineno, _approve) + except ValueError as e: + raise HTTPException( + status_code=HTTPStatus.INTERNAL_SERVER_ERROR, detail=str(e) + ) + except ChecksumConflictError: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Ledger changed concurrently; retry the approval", ) - update_response.raise_for_status() logger.info(f"Entry {entry_id} approved (flag changed to *)") @@ -3012,8 +2983,7 @@ async def api_reject_expense_entry( Adds #voided tag for audit trail while keeping the '!' flag. Voided transactions are excluded from balances but preserved in the ledger. """ - import httpx - from .fava_client import get_fava_client + from .fava_client import ChecksumConflictError, get_fava_client fava = get_fava_client() @@ -3048,50 +3018,26 @@ async def api_reject_expense_entry( detail="Entry metadata missing filename or lineno" ) - # 3. Get the source file from Fava - async with httpx.AsyncClient(timeout=fava.timeout) as client: - response = await client.get( - f"{fava.base_url}/source", - params={"filename": filename} + # 3. Add the #voided tag under FavaClient's write lock — the whole + # read-modify-write is atomic against every other ledger writer. + def _void(line: str) -> str: + if "#voided" in line: + return line # already voided — no-op + return line.rstrip() + ' #voided' + + try: + changed = await fava.transform_source_line(filename, lineno, _void) + except ValueError as e: + raise HTTPException( + status_code=HTTPStatus.INTERNAL_SERVER_ERROR, detail=str(e) ) - response.raise_for_status() - source_data = response.json()["data"] - - sha256sum = source_data["sha256sum"] - source = source_data["source"] - lines = source.split('\n') - - # 4. Find and modify the entry at the specified line - add #voided tag - entry_line_idx = lineno - 1 - - if entry_line_idx >= len(lines): - raise HTTPException( - status_code=HTTPStatus.INTERNAL_SERVER_ERROR, - detail=f"Line {lineno} not found in source file" - ) - - entry_line = lines[entry_line_idx] - - # Add #voided tag if not already present - if "#voided" not in entry_line: - # Add #voided tag to the transaction line - new_line = entry_line.rstrip() + ' #voided' - lines[entry_line_idx] = new_line - - # 5. Write back the modified source - new_source = '\n'.join(lines) - - update_response = await client.put( - f"{fava.base_url}/source", - json={ - "file_path": filename, - "source": new_source, - "sha256sum": sha256sum - }, - headers={"Content-Type": "application/json"} - ) - update_response.raise_for_status() - logger.info(f"Entry {entry_id} rejected (added #voided tag)") + except ChecksumConflictError: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Ledger changed concurrently; retry the rejection", + ) + if changed: + logger.info(f"Entry {entry_id} rejected (added #voided tag)") return { "message": f"Entry {entry_id} rejected (marked as voided)", @@ -3820,8 +3766,14 @@ async def api_admin_add_chart_account( "already_existed": True, } - # Mirror into libra DB so permissions / metadata layer sees it. - synced = await sync_single_account_from_beancount(payload.name) + # Mirror into libra DB so permissions / metadata layer sees it. We just + # wrote the Open directive ourselves, so skip the verification + # round-trip through Fava (libra-#53). + synced = await sync_single_account_from_beancount( + payload.name, + description=payload.description, + assume_exists=True, + ) return { "success": True, From c0d371036ba05c96a1457126bdc356dfe24629f7 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 15:52:29 +0200 Subject: [PATCH 7/8] fix(auth): exact-match authorization; guard reviews; centralize name validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auth + input-validation cluster (CODE-REVIEW-2026-06 #5, #6, #16, #17 + libra-#36, libra-#51, libra-#52): - can_access_user_data compares full user ids only. The 8-char prefix comparison was a 32-bit space: any prefix collision (or a crafted short target id) let one user read another's data. - can_access_account matches the User-{short} SEGMENT exactly; the substring test also matched accounts merely containing it (Expenses:Misc-User-deadbeef). - Manual-payment approve/reject are status-guarded (UPDATE ... WHERE status='pending' + rowcount): concurrent admins can't double-book. The approve endpoint claims the request BEFORE writing the ledger entry and reverts the claim if the write fails, so at most one journal entry can exist per request. - Account-name validation centralized into account_utils.validate_account_name (libra-#51) — called from crud.create_account (the choke point for every creation path, virtual parents allowed a bare root), the admin add-account endpoint, and fava_client.add_account at the writer boundary (libra-#52). - crud.create_account translates backend unique-violations into AccountExistsError instead of leaking sqlalchemy internals (libra-#36); POST /accounts returns 409 on duplicates and 400 on malformed names. get_or_create_user_account catches the domain error instead of string-matching the SQLite message (which never matched on Postgres). Co-Authored-By: Claude Fable 5 --- account_utils.py | 52 +++++++++ auth.py | 18 +-- crud.py | 142 +++++++++++++++-------- fava_client.py | 7 ++ tests/test_auth_validation.py | 207 ++++++++++++++++++++++++++++++++++ views_api.py | 99 ++++++++-------- 6 files changed, 422 insertions(+), 103 deletions(-) create mode 100644 tests/test_auth_validation.py diff --git a/account_utils.py b/account_utils.py index 8520d59..d797796 100644 --- a/account_utils.py +++ b/account_utils.py @@ -17,6 +17,58 @@ ACCOUNT_TYPE_ROOTS = { AccountType.EXPENSE: "Expenses", } +VALID_ACCOUNT_PREFIXES = ("Assets:", "Liabilities:", "Equity:", "Income:", "Expenses:") + + +def is_valid_account_component(component: str, *, is_root: bool) -> bool: + """Validate one ':'-separated account component against Beancount's grammar. + + Mirrors core/account.py: a root component matches ``[\\p{Lu}][\\p{L}\\p{Nd}-]*`` + (must start with an uppercase letter); a sub component matches + ``[\\p{Lu}\\p{Nd}][\\p{L}\\p{Nd}-]*`` (may also start with a digit). Body + chars are letters, decimal digits, or hyphen. Implemented with Unicode-aware + str methods (libra's runtime has no beancount — Fava is a separate service), + so non-ASCII letters are accepted exactly as Beancount accepts them. + """ + if not component: + return False + first, rest = component[0], component[1:] + first_ok = (first.isalpha() and first.isupper()) or ( + not is_root and first.isdecimal() + ) + if not first_ok: + return False + return all(ch == "-" or ch.isalpha() or ch.isdecimal() for ch in rest) + + +def validate_account_name(name: str, *, allow_root_only: bool = False) -> None: + """Raise ValueError if ``name`` is not a syntactically valid Beancount account. + + The single source of truth for account-name validation (libra-#51): + every path that writes an account name — admin add-account, direct + account create, user-account derivation — funnels through here before + the name can reach the ledger source. + + Args: + name: Hierarchical account name (e.g. "Expenses:Food"). + allow_root_only: Accept a bare root component ("Expenses") — + only virtual parent accounts are allowed this shape. + """ + parts = name.split(":") + min_parts = 1 if allow_root_only else 2 + valid = ( + len(parts) >= min_parts + and is_valid_account_component(parts[0], is_root=True) + and all(is_valid_account_component(p, is_root=False) for p in parts[1:]) + ) + if not valid: + raise ValueError( + f"Invalid account name {name!r}: each ':'-separated part must be " + "letters/digits/hyphens, the root starting with an uppercase " + "letter (sub-accounts may start with a digit), with at least one " + "sub-account (e.g. Expenses:Food)." + ) + def format_hierarchical_account_name( account_type: AccountType, diff --git a/auth.py b/auth.py index 8cc9c17..34ba8c6 100644 --- a/auth.py +++ b/auth.py @@ -172,11 +172,14 @@ async def can_access_account( if auth.is_super_user: return True - # Check if this is the user's own account + # Check if this is the user's own account. Match the User-{short} + # segment exactly — a substring test also matched account names that + # merely CONTAIN it (e.g. "Expenses:Misc-User-deadbeef"), granting + # access to unrelated accounts. account = await get_account(account_id) if account: - user_short = auth.user_id[:8] - if f"User-{user_short}" in account.name: + user_segment = f"User-{auth.user_id[:8]}" + if user_segment in account.name.split(":"): return True # Check explicit permissions @@ -242,14 +245,13 @@ async def can_access_user_data(auth: AuthContext, target_user_id: str) -> bool: if auth.is_super_user: return True - # Users can access their own data - compare full ID or short ID + # Users can access their own data. Full-ID equality ONLY: an 8-char + # prefix comparison is a 32-bit space, and any prefix collision (or a + # deliberately crafted short target id) let one user read another's + # data. Callers must pass full user ids. if auth.user_id == target_user_id: return True - # Also allow if short IDs match (8 char prefix) - if auth.user_id[:8] == target_user_id[:8]: - return True - return False diff --git a/crud.py b/crud.py index b49bb9b..5ea93f0 100644 --- a/crud.py +++ b/crud.py @@ -66,7 +66,21 @@ PERMISSION_CACHE_TTL = 60 # 1 minute # ===== ACCOUNT OPERATIONS ===== +class AccountExistsError(Exception): + """Raised when creating an account whose name is already taken.""" + + def __init__(self, name: str): + super().__init__(f"Account already exists: {name}") + self.name = name + + async def create_account(data: CreateAccount) -> Account: + # Single validation choke point for every account-creation path + # (libra-#51). Virtual parents may be a bare root ("Expenses"). + from .account_utils import validate_account_name + + validate_account_name(data.name, allow_root_only=data.is_virtual) + account_id = urlsafe_short_hash() account = Account( id=account_id, @@ -77,7 +91,17 @@ async def create_account(data: CreateAccount) -> Account: is_virtual=data.is_virtual, created_at=datetime.now(), ) - await db.insert("accounts", account) + try: + await db.insert("accounts", account) + except Exception as e: + # Translate backend-specific unique-violation errors (SQLite: + # "UNIQUE constraint failed", Postgres: "duplicate key value") + # into a domain error instead of leaking sqlalchemy internals + # (libra-#36). + msg = str(e).lower() + if "unique" in msg or "duplicate" in msg: + raise AccountExistsError(data.name) from e + raise # Invalidate cache for this account (Cache class doesn't have delete method, use pop) account_cache._values.pop(f"account:id:{account_id}", None) @@ -305,44 +329,39 @@ async def get_or_create_user_account( user_id=user_id, ) ) - except Exception as e: - # Handle UNIQUE constraint error - account already exists - if "UNIQUE constraint failed" in str(e) and "accounts.name" in str(e): - logger.warning(f"[LIBRA DB] Account already exists (UNIQUE constraint), fetching by name: {account_name}") - # Fetch existing account by name only (ignore user_id in query) - account = await db.fetchone( - """ - SELECT * FROM accounts - WHERE name = :name - """, - {"name": account_name}, - Account, - ) - if account: - logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})") - # Update user_id if it's NULL or different - if account.user_id != user_id: - logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}") - await db.execute( - """ - UPDATE accounts - SET user_id = :user_id - WHERE name = :name - """, - {"user_id": user_id, "name": account_name} - ) - # Refresh account from DB - account = await db.fetchone( - """ - SELECT * FROM accounts - WHERE name = :name - """, - {"name": account_name}, - Account, - ) - else: - # Re-raise if it's a different error - raise + except AccountExistsError: + logger.warning(f"[LIBRA DB] Account already exists, fetching by name: {account_name}") + # Fetch existing account by name only (ignore user_id in query) + account = await db.fetchone( + """ + SELECT * FROM accounts + WHERE name = :name + """, + {"name": account_name}, + Account, + ) + if account: + logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})") + # Update user_id if it's NULL or different + if account.user_id != user_id: + logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}") + await db.execute( + """ + UPDATE accounts + SET user_id = :user_id + WHERE name = :name + """, + {"user_id": user_id, "name": account_name} + ) + # Refresh account from DB + account = await db.fetchone( + """ + SELECT * FROM accounts + WHERE name = :name + """, + {"name": account_name}, + Account, + ) else: logger.info(f"[LIBRA DB] Account already exists in Libra DB: {account_name}") @@ -563,14 +582,20 @@ async def get_all_manual_payment_requests( async def approve_manual_payment_request( request_id: str, reviewed_by: str, journal_entry_id: str ) -> Optional["ManualPaymentRequest"]: - """Approve a manual payment request""" - from .models import ManualPaymentRequest + """Approve a manual payment request. - await db.execute( + Status-guarded: only a 'pending' request can be approved, so two + concurrent admins can't both win (the loser gets None and must not + create a second journal entry). + + Returns: + The approved request, or None if it wasn't pending anymore. + """ + result = await db.execute( """ UPDATE manual_payment_requests SET status = 'approved', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by, journal_entry_id = :journal_entry_id - WHERE id = :id + WHERE id = :id AND status = 'pending' """, { "id": request_id, @@ -579,21 +604,42 @@ async def approve_manual_payment_request( "journal_entry_id": journal_entry_id, }, ) + if result.rowcount == 0: + return None return await get_manual_payment_request(request_id) +async def revert_manual_payment_request(request_id: str) -> None: + """Roll an approved request back to pending. + + Compensation for the approve flow: the status is claimed BEFORE the + journal entry is written (so concurrent admins can't double-book); + if the ledger write then fails, the claim must be released. + """ + await db.execute( + """ + UPDATE manual_payment_requests + SET status = 'pending', reviewed_at = NULL, reviewed_by = NULL, journal_entry_id = NULL + WHERE id = :id AND status = 'approved' + """, + {"id": request_id}, + ) + + async def reject_manual_payment_request( request_id: str, reviewed_by: str ) -> Optional["ManualPaymentRequest"]: - """Reject a manual payment request""" - from .models import ManualPaymentRequest + """Reject a manual payment request. - await db.execute( + Status-guarded like approve_manual_payment_request; returns None when + the request wasn't pending anymore. + """ + result = await db.execute( """ UPDATE manual_payment_requests SET status = 'rejected', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by - WHERE id = :id + WHERE id = :id AND status = 'pending' """, { "id": request_id, @@ -601,6 +647,8 @@ async def reject_manual_payment_request( "reviewed_by": reviewed_by, }, ) + if result.rowcount == 0: + return None return await get_manual_payment_request(request_id) diff --git a/fava_client.py b/fava_client.py index faad8f7..057562a 100644 --- a/fava_client.py +++ b/fava_client.py @@ -1775,6 +1775,13 @@ class FavaClient: """ from datetime import date as date_type + # Defense in depth at the writer boundary (libra-#52): the name is + # written verbatim into ledger source below, so validate it HERE, + # not only in the endpoints that happen to call this today. + from .account_utils import validate_account_name + + validate_account_name(account_name, allow_root_only=True) + if opening_date is None: opening_date = date_type.today() diff --git a/tests/test_auth_validation.py b/tests/test_auth_validation.py new file mode 100644 index 0000000..90d278f --- /dev/null +++ b/tests/test_auth_validation.py @@ -0,0 +1,207 @@ +"""Auth narrowing + input validation. + +Covers the PR-5 fixes: + - `can_access_user_data`: full-ID equality only (an 8-char prefix + comparison let prefix-colliding users read each other's data). + - `can_access_account`: exact User-{short} SEGMENT match (a substring + test also matched accounts merely containing it). + - Manual-payment approve/reject: status-guarded claim — concurrent + admins can't double-book (CODE-REVIEW-2026-06 #16). + - POST /accounts: duplicate → 409 instead of a leaked + IntegrityError 500 (libra-#36); malformed name → 400 (libra-#51). +""" +import asyncio +import importlib +from uuid import uuid4 + +import pytest + +from .helpers import submit_manual_payment_request + +pytestmark = pytest.mark.anyio + + +def _module(name: str): + for prefix in ("lnbits.extensions.libra", "libra"): + try: + return importlib.import_module(f"{prefix}.{name}") + except ModuleNotFoundError: + continue + raise ModuleNotFoundError(f"libra.{name}: tried both import paths") + + +auth = _module("auth") +libra_crud = _module("crud") +mdl = _module("models") + + +def _ctx(user_id: str) -> "auth.AuthContext": + return auth.AuthContext( + user_id=user_id, wallet_id="w", is_super_user=False, wallet=None, + ) + + +# --------------------------------------------------------------------------- +# can_access_user_data — full-ID equality +# --------------------------------------------------------------------------- + + +async def test_prefix_colliding_user_cannot_access_other_users_data(client): + caller = "deadbeef" + uuid4().hex[8:] + victim = "deadbeef" + uuid4().hex[8:] # same 8-char prefix, different id + assert victim != caller + + assert await auth.can_access_user_data(_ctx(caller), caller) is True + assert await auth.can_access_user_data(_ctx(caller), victim) is False + # A crafted short target id must not match either. + assert await auth.can_access_user_data(_ctx(caller), caller[:8]) is False + + +# --------------------------------------------------------------------------- +# can_access_account — exact segment match +# --------------------------------------------------------------------------- + + +async def test_account_access_requires_exact_user_segment(client): + user_id = "deadbeef" + uuid4().hex[8:] + suffix = uuid4().hex[:6] + + # An account whose LAST SEGMENT merely contains "User-deadbeef". + lookalike = await libra_crud.create_account( + mdl.CreateAccount( + name=f"Expenses:Misc-User-deadbeef-{suffix}", + account_type=mdl.AccountType.EXPENSE, + description="substring-match bait", + ) + ) + owned = await libra_crud.create_account( + mdl.CreateAccount( + name=f"Assets:Receivable-{suffix}:User-deadbeef", + account_type=mdl.AccountType.ASSET, + description="genuinely owned", + user_id=user_id, + ) + ) + + ctx = _ctx(user_id) + assert await auth.can_access_account( + ctx, lookalike.id, mdl.PermissionType.READ + ) is False, "substring-only match must not grant access" + assert await auth.can_access_account( + ctx, owned.id, mdl.PermissionType.READ + ) is True + + +# --------------------------------------------------------------------------- +# Manual payment approve/reject — status-guarded claim +# --------------------------------------------------------------------------- + + +async def test_concurrent_approvals_create_exactly_one_entry( + client, super_user_headers, configured_user, +): + _, wallet = configured_user + submitted = await submit_manual_payment_request( + client, + wallet_inkey=wallet.inkey, + amount_sats=10_000, + description=f"race {uuid4().hex[:6]}", + ) + + r1, r2 = await asyncio.gather( + client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ), + client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ), + ) + statuses = sorted([r1.status_code, r2.status_code]) + assert statuses[0] == 200, f"one approval must win: {statuses} {r1.text} {r2.text}" + assert statuses[1] in (400, 409), ( + f"the losing approval must fail cleanly, got {statuses}" + ) + + # Exactly one ledger entry references this request. + listing = await client.get( + "/libra/api/v1/entries/user", + headers={"X-Api-Key": wallet.inkey}, + ) + assert listing.status_code == 200 + link = f"MPR-{submitted['id']}" + matching = [ + e for e in listing.json()["entries"] if link in (e.get("links") or []) + ] + assert len(matching) == 1, ( + f"expected exactly one journal entry for {link}, got {len(matching)}" + ) + + +async def test_reject_after_approve_conflicts( + client, super_user_headers, configured_user, +): + _, wallet = configured_user + submitted = await submit_manual_payment_request( + client, + wallet_inkey=wallet.inkey, + amount_sats=5_000, + description=f"approve-then-reject {uuid4().hex[:6]}", + ) + + r = await client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/approve", + headers=super_user_headers, + ) + assert r.status_code == 200, r.text + + r = await client.post( + f"/libra/api/v1/manual-payment-requests/{submitted['id']}/reject", + headers=super_user_headers, + ) + assert r.status_code in (400, 409), ( + f"rejecting an approved request must fail, got {r.status_code}" + ) + + +# --------------------------------------------------------------------------- +# POST /accounts — duplicate and malformed names +# --------------------------------------------------------------------------- + + +async def test_create_duplicate_account_returns_409( + client, super_user_headers, +): + name = f"Expenses:DupTest-{uuid4().hex[:6]}" + body = {"name": name, "account_type": "expense", "description": "dup test"} + + r = await client.post( + "/libra/api/v1/accounts", headers=super_user_headers, json=body, + ) + assert r.status_code == 201, r.text + + r = await client.post( + "/libra/api/v1/accounts", headers=super_user_headers, json=body, + ) + assert r.status_code == 409, ( + f"duplicate create must 409, not leak an IntegrityError: " + f"{r.status_code} {r.text}" + ) + + +async def test_create_account_with_invalid_name_returns_400( + client, super_user_headers, +): + r = await client.post( + "/libra/api/v1/accounts", + headers=super_user_headers, + json={ + "name": 'Expenses:bad"name\nfoo', + "account_type": "expense", + }, + ) + assert r.status_code == 400, ( + f"malformed account name must 400 before reaching the ledger: " + f"{r.status_code} {r.text}" + ) diff --git a/views_api.py b/views_api.py index 1c3269f..b41e6b6 100644 --- a/views_api.py +++ b/views_api.py @@ -13,6 +13,7 @@ from lnbits.decorators import ( ) from lnbits.utils.exchange_rates import allowed_currencies, fiat_amount_as_satoshis +from .account_utils import VALID_ACCOUNT_PREFIXES, validate_account_name from .beancount_format import fiat_rate_metadata from .crud import ( approve_manual_payment_request, @@ -295,7 +296,19 @@ async def api_create_account( auth: AuthContext = Depends(require_super_user), ) -> Account: """Create a new account (super user only)""" - return await create_account(data) + from .crud import AccountExistsError + + try: + return await create_account(data) + except ValueError as e: + raise HTTPException( + status_code=HTTPStatus.BAD_REQUEST, detail=str(e) + ) + except AccountExistsError: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail=f"Account {data.name} already exists", + ) @libra_api_router.get("/api/v1/accounts/{account_id}") @@ -2856,15 +2869,30 @@ async def api_approve_manual_payment_request( settled_entry_links=settled_links ) - # Submit to Fava - result = await fava.add_entry(entry) - logger.info(f"Manual payment entry submitted to Fava: {result.get('data', 'Unknown')}") - - # Approve the request with Fava entry reference - entry_id = f"fava-{datetime.now().timestamp()}" - return await approve_manual_payment_request( - request_id, auth.user_id, entry_id + # Claim the request BEFORE writing the ledger entry — the + # status-guarded UPDATE makes exactly one concurrent admin win, so + # only one journal entry can ever be created for this request. + approved = await approve_manual_payment_request( + request_id, auth.user_id, f"MPR-{request.id}" ) + if approved is None: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Request was already reviewed by another admin", + ) + + try: + result = await fava.add_entry(entry) + logger.info(f"Manual payment entry submitted to Fava: {result.get('data', 'Unknown')}") + except BaseException: + # Ledger write failed — release the claim so the request can be + # approved again. + from .crud import revert_manual_payment_request + + await revert_manual_payment_request(request_id) + raise + + return approved @libra_api_router.post("/api/v1/manual-payment-requests/{request_id}/reject") @@ -2887,7 +2915,13 @@ async def api_reject_manual_payment_request( detail=f"Request already {request.status}", ) - return await reject_manual_payment_request(request_id, auth.user_id) + rejected = await reject_manual_payment_request(request_id, auth.user_id) + if rejected is None: + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail="Request was already reviewed by another admin", + ) + return rejected # ===== EXPENSE APPROVAL ENDPOINTS ===== @@ -3650,52 +3684,21 @@ async def api_get_account_hierarchy( # ===== ACCOUNT SYNC ENDPOINTS ===== -_VALID_ACCOUNT_PREFIXES = ("Assets:", "Liabilities:", "Equity:", "Income:", "Expenses:") - - -def _is_valid_account_component(component: str, *, is_root: bool) -> bool: - """Validate one ':'-separated account component against Beancount's grammar. - - Mirrors core/account.py: a root component matches ``[\\p{Lu}][\\p{L}\\p{Nd}-]*`` - (must start with an uppercase letter); a sub component matches - ``[\\p{Lu}\\p{Nd}][\\p{L}\\p{Nd}-]*`` (may also start with a digit). Body - chars are letters, decimal digits, or hyphen. Implemented with Unicode-aware - str methods (libra's runtime has no beancount — Fava is a separate service), - so non-ASCII letters are accepted exactly as Beancount accepts them. - """ - if not component: - return False - first, rest = component[0], component[1:] - first_ok = (first.isalpha() and first.isupper()) or ( - not is_root and first.isdecimal() - ) - if not first_ok: - return False - return all(ch == "-" or ch.isalpha() or ch.isdecimal() for ch in rest) +_VALID_ACCOUNT_PREFIXES = VALID_ACCOUNT_PREFIXES def _validate_account_name(name: str) -> None: """Raise HTTP 400 if ``name`` is not a syntactically valid Beancount account. - The UI guards this client-side, but the endpoint is reachable directly via - API, so this is the load-bearing check before the name is written into the - ledger source. Requires a root plus at least one sub-component. + Thin HTTP wrapper around account_utils.validate_account_name — the + single source of truth for account-name syntax (libra-#51). """ - parts = name.split(":") - valid = ( - len(parts) >= 2 - and _is_valid_account_component(parts[0], is_root=True) - and all(_is_valid_account_component(p, is_root=False) for p in parts[1:]) - ) - if not valid: + try: + validate_account_name(name) + except ValueError as e: raise HTTPException( status_code=HTTPStatus.BAD_REQUEST, - detail=( - f"Invalid account name {name!r}: each ':'-separated part must be " - "letters/digits/hyphens, the root starting with an uppercase " - "letter (sub-accounts may start with a digit), with at least one " - "sub-account (e.g. Expenses:Food)." - ), + detail=str(e), ) From ec6cac51f06fcee62b01081e3771145ef813cb89 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 16:01:04 +0200 Subject: [PATCH 8/8] =?UTF-8?q?chore:=20hygiene=20sweep=20=E2=80=94=20dead?= =?UTF-8?q?=20code,=20role=20race,=20user=20lookup,=20stale=20files?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One pass over the LOW-tier review items plus two folded issues: - Delete validate_journal_entry (dead since the Fava migration; it validated the pre-string-amount model) with its exports, unused crud imports, and tests. Beancount validates entries now. - Migration m006: UNIQUE index on user_roles(user_id, role_id) after deduping; assign_user_role inserts with ON CONFLICT DO NOTHING and returns the existing assignment — closes the auto-assign check-then-act race on concurrent logins. - Extract _get_username_from_user_id (110 lines in views_api, fresh LNbits Database per call inside per-row hot paths) into user_lookup.py with one shared core-DB handle, a 60s TTL cache and a batch get_usernames API (review #18). - Receivable-entry responses report CLEARED, matching the flag the formatter actually writes; PENDING misled the UI (libra-#35). - Replace the remaining print() calls in tasks.py with logger. - get_all_accounts derives valid roots from account_utils.ACCOUNT_TYPE_ROOTS instead of a hardcoded tuple, and the no-op per-test rate-limit reset is gone (libra-#54). - Delete migrations_old.py.bak, MIGRATION_SQUASH_SUMMARY.md, docs/PHASE*_COMPLETE.md and the rendered .html; .gitignore data/ (it holds the runtime .lnbits_auth_key secret). - Track docs/CODE-REVIEW-2026-06.md with finding statuses updated for the PR #55-#59 + chore/hygiene series. - CLAUDE.md notes LNbits pins Pydantic v1: keep .dict(), don't "modernize" to .model_dump(). Note: format_payment_entry's is_payable docstring (flagged in review follow-up) turned out to be consistent with the body — no change. Co-Authored-By: Claude Fable 5 --- .gitignore | 1 + CLAUDE.md | 6 + MIGRATION_SQUASH_SUMMARY.md | 218 ----- core/__init__.py | 3 +- core/validation.py | 75 -- crud.py | 30 +- docs/ACCOUNTING-ANALYSIS-NET-SETTLEMENT.html | 953 ------------------- docs/CODE-REVIEW-2026-06.md | 260 +++++ docs/PHASE1_COMPLETE.md | 200 ---- docs/PHASE2_COMPLETE.md | 273 ------ docs/PHASE3_COMPLETE.md | 365 ------- fava_client.py | 5 +- migrations.py | 27 + migrations_old.py.bak | 651 ------------- tasks.py | 16 +- tests/conftest.py | 3 - tests/test_unit.py | 60 -- user_lookup.py | 126 +++ views_api.py | 124 +-- 19 files changed, 454 insertions(+), 2942 deletions(-) delete mode 100644 MIGRATION_SQUASH_SUMMARY.md delete mode 100644 docs/ACCOUNTING-ANALYSIS-NET-SETTLEMENT.html create mode 100644 docs/CODE-REVIEW-2026-06.md delete mode 100644 docs/PHASE1_COMPLETE.md delete mode 100644 docs/PHASE2_COMPLETE.md delete mode 100644 docs/PHASE3_COMPLETE.md delete mode 100644 migrations_old.py.bak create mode 100644 user_lookup.py diff --git a/.gitignore b/.gitignore index e68ab2e..1ff904f 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ __pycache__ node_modules .venv .mypy_cache +data/ diff --git a/CLAUDE.md b/CLAUDE.md index 97e546f..1016821 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -169,6 +169,12 @@ User-specific accounts are created automatically with format: Use `get_or_create_user_account()` in crud.py to ensure consistency. +### Pydantic version + +LNbits pins **Pydantic v1** (`pydantic~=1.10`) — keep `.dict()` / +`.parse_obj()` v1 APIs. Do NOT "modernize" to `.model_dump()` etc.; +it would crash at runtime until upstream migrates. + ### Currency Handling **CRITICAL**: Use `Decimal` for all fiat amounts, never `float`. diff --git a/MIGRATION_SQUASH_SUMMARY.md b/MIGRATION_SQUASH_SUMMARY.md deleted file mode 100644 index 4b03ed0..0000000 --- a/MIGRATION_SQUASH_SUMMARY.md +++ /dev/null @@ -1,218 +0,0 @@ -# Libra Migration Squash Summary - -**Date:** November 10, 2025 -**Action:** Squashed 16 incremental migrations into a single clean initial migration - -## Overview - -The Libra extension had accumulated 16 migrations (m001-m016) during development. Since the software has not been released yet, we safely squashed all migrations into a single clean `m001_initial` migration. - -## Files Changed - -- **migrations.py** - Replaced with squashed single migration (651 → 327 lines) -- **migrations_old.py.bak** - Backup of original 16 migrations for reference - -## Final Database Schema - -The squashed migration creates **7 tables**: - -### 1. libra_accounts -- Core chart of accounts with hierarchical Beancount-style names -- Examples: "Assets:Bitcoin:Lightning", "Expenses:Food:Groceries" -- User-specific accounts: "Assets:Receivable:User-af983632" -- Includes comprehensive default account set (40+ accounts) - -### 2. libra_extension_settings -- Libra-wide configuration -- Stores libra_wallet_id for Lightning payments - -### 3. libra_user_wallet_settings -- Per-user wallet configuration -- Allows users to have separate wallet preferences - -### 4. libra_manual_payment_requests -- User-submitted payment requests to Libra -- Reviewed by admins before processing -- Includes notes field for additional context - -### 5. libra_balance_assertions -- Reconciliation and balance checking at specific dates -- Multi-currency support (satoshis + fiat) -- Tolerance checking for small discrepancies -- Includes notes field for reconciliation comments - -### 6. libra_user_equity_status -- Manages equity contribution eligibility -- Equity-eligible users can convert expenses to equity -- Creates dynamic user-specific equity accounts: Equity:User-{user_id} - -### 7. libra_account_permissions -- Granular access control for accounts -- Permission types: read, submit_expense, manage -- Supports hierarchical inheritance (parent permissions cascade) -- Time-based expiration support - -## What Was Removed - -The following tables were **intentionally NOT included** in the final schema (they were dropped in m016): - -- **libra_journal_entries** - Journal entries now managed by Fava/Beancount (external source of truth) -- **libra_entry_lines** - Entry lines now managed by Fava/Beancount - -Libra now uses Fava as the single source of truth for accounting data. Journal operations: -- **Write:** Submit to Fava via FavaClient.add_entry() -- **Read:** Query Fava via FavaClient.get_entries() - -## Key Schema Decisions - -1. **Hierarchical Account Names** - Beancount-style colon-separated hierarchy (e.g., "Assets:Bitcoin:Lightning") -2. **No Journal Tables** - Fava/Beancount is the source of truth for journal entries -3. **Dynamic User Accounts** - User-specific accounts created on-demand (Assets:Receivable:User-xxx, Equity:User-xxx) -4. **No Parent-Only Accounts** - Hierarchy is implicit in names (no "Assets:Bitcoin" parent account needed) -5. **Multi-Currency Support** - Balance assertions support both satoshis and fiat currencies -6. **Notes Fields** - Added notes to balance_assertions and manual_payment_requests for better documentation - -## Migration History (Original 16 Migrations) - -For reference, the original migration sequence (preserved in migrations_old.py.bak): - -1. **m001** - Initial accounts, journal_entries, entry_lines tables -2. **m002** - Extension settings table -3. **m003** - User wallet settings table -4. **m004** - Manual payment requests table -5. **m005** - Added flag/meta columns to journal_entries -6. **m006** - Migrated to hierarchical account names -7. **m007** - Balance assertions table -8. **m008** - Renamed Lightning account (Assets:Lightning:Balance → Assets:Bitcoin:Lightning) -9. **m009** - Added OnChain Bitcoin account (Assets:Bitcoin:OnChain) -10. **m010** - User equity status table -11. **m011** - Account permissions table -12. **m012** - Updated default accounts with detailed hierarchy (40+ accounts) -13. **m013** - Removed parent-only accounts (Assets:Bitcoin, Equity) -14. **m014** - Removed legacy equity accounts (MemberEquity, RetainedEarnings) -15. **m015** - Converted entry_lines from debit/credit to single amount field -16. **m016** - Dropped journal_entries and entry_lines tables (Fava integration) - -## Benefits of Squashing - -1. **Cleaner Codebase** - Single 327-line migration vs 651 lines across 16 functions -2. **Easier to Understand** - New developers see final schema immediately -3. **Faster Fresh Installs** - One migration run instead of 16 -4. **Better Documentation** - Comprehensive comments explain design decisions -5. **No Migration Artifacts** - No intermediate states, data conversions, or temporary columns - -## Fresh Install Process - -For new installations: - -```bash -# Libra's migration system will run m001_initial automatically -# No manual intervention needed -``` - -The migration will: -1. Create all 7 tables with proper indexes and foreign keys -2. Insert 40+ default accounts with hierarchical names -3. Set up proper constraints and defaults -4. Complete in a single transaction - -## Default Accounts Created - -The migration automatically creates a comprehensive chart of accounts: - -**Assets (12 accounts):** -- Assets:Bank -- Assets:Bitcoin:Lightning -- Assets:Bitcoin:OnChain -- Assets:Cash -- Assets:FixedAssets:Equipment -- Assets:FixedAssets:FarmEquipment -- Assets:FixedAssets:Network -- Assets:FixedAssets:ProductionFacility -- Assets:Inventory -- Assets:Livestock -- Assets:Receivable -- Assets:Tools - -**Liabilities (1 account):** -- Liabilities:Payable - -**Income (3 accounts):** -- Income:Accommodation:Guests -- Income:Service -- Income:Other - -**Expenses (24 accounts):** -- Expenses:Administrative -- Expenses:Construction:Materials -- Expenses:Furniture -- Expenses:Garden -- Expenses:Gas:Kitchen -- Expenses:Gas:Vehicle -- Expenses:Groceries -- Expenses:Hardware -- Expenses:Housewares -- Expenses:Insurance -- Expenses:Kitchen -- Expenses:Maintenance:Car -- Expenses:Maintenance:Garden -- Expenses:Maintenance:Property -- Expenses:Membership -- Expenses:Supplies -- Expenses:Tools -- Expenses:Utilities:Electric -- Expenses:Utilities:Internet -- Expenses:WebHosting:Domain -- Expenses:WebHosting:Wix - -**Equity:** -- Created dynamically as Equity:User-{user_id} when granting equity eligibility - -## Testing - -After squashing, verify the migration works: - -```bash -# 1. Backup existing database (if any) -cp libra.sqlite3 libra.sqlite3.backup - -# 2. Drop and recreate database to test fresh install -rm libra.sqlite3 - -# 3. Start LNbits - migration should run automatically -poetry run lnbits - -# 4. Verify tables created -sqlite3 libra.sqlite3 ".tables" -# Should show: libra_accounts, libra_extension_settings, etc. - -# 5. Verify default accounts -sqlite3 libra.sqlite3 "SELECT COUNT(*) FROM libra_accounts;" -# Should show: 40 (default accounts) -``` - -## Rollback Plan - -If issues are discovered: - -```bash -# Restore original migrations -cp migrations_old.py.bak migrations.py - -# Restore database -cp libra.sqlite3.backup libra.sqlite3 -``` - -## Notes - -- This squash is safe because Libra has not been released yet -- No existing production databases need migration -- Historical migrations preserved in migrations_old.py.bak -- All functionality preserved in final schema -- No data loss concerns (no production data exists) - ---- - -**Signed off by:** Claude Code -**Reviewed by:** Human operator -**Status:** Complete diff --git a/core/__init__.py b/core/__init__.py index 10c362c..e8b8fa3 100644 --- a/core/__init__.py +++ b/core/__init__.py @@ -16,10 +16,9 @@ Note: Balance calculation and inventory tracking have been migrated to Fava/Bean All accounting calculations are now performed via Fava's query API. """ -from .validation import ValidationError, validate_journal_entry, validate_balance +from .validation import ValidationError, validate_balance __all__ = [ "ValidationError", - "validate_journal_entry", "validate_balance", ] diff --git a/core/validation.py b/core/validation.py index 913bb8a..04416fc 100644 --- a/core/validation.py +++ b/core/validation.py @@ -18,81 +18,6 @@ class ValidationError(Exception): self.details = details or {} -def validate_journal_entry( - entry: Dict[str, Any], - entry_lines: List[Dict[str, Any]] -) -> None: - """ - Validate a journal entry and its lines (Beancount-style with single amount field). - - Checks: - 1. Entry must have at least 2 lines (double-entry requirement) - 2. Entry must be balanced (sum of amounts = 0) - 3. All lines must have account_id - 4. No line should have amount = 0 (would serve no purpose) - - Args: - entry: Journal entry dict with keys: - - id: str - - description: str - - entry_date: datetime - entry_lines: List of entry line dicts with keys: - - account_id: str - - amount: int (positive = debit, negative = credit) - - Raises: - ValidationError: If validation fails - """ - # Check minimum number of lines - if len(entry_lines) < 2: - raise ValidationError( - "Journal entry must have at least 2 lines", - { - "entry_id": entry.get("id"), - "line_count": len(entry_lines), - } - ) - - # Validate each line - for i, line in enumerate(entry_lines): - # Check account_id exists - if not line.get("account_id"): - raise ValidationError( - f"Entry line {i + 1} missing account_id", - { - "entry_id": entry.get("id"), - "line_index": i, - } - ) - - # Get amount (Beancount-style: positive = debit, negative = credit) - amount = line.get("amount", 0) - - # Check that amount is non-zero (zero amounts serve no purpose) - if amount == 0: - raise ValidationError( - f"Entry line {i + 1} has amount = 0 (serves no purpose)", - { - "entry_id": entry.get("id"), - "line_index": i, - } - ) - - # Check entry is balanced (sum of amounts must equal 0) - # Beancount-style: positive amounts cancel out negative amounts - total_amount = sum(line.get("amount", 0) for line in entry_lines) - - if total_amount != 0: - raise ValidationError( - "Journal entry is not balanced (sum of amounts must equal 0)", - { - "entry_id": entry.get("id"), - "total_amount": total_amount, - "line_count": len(entry_lines), - } - ) - - def validate_balance( account_id: str, expected_balance_sats: int, diff --git a/crud.py b/crud.py index 5ea93f0..c28d685 100644 --- a/crud.py +++ b/crud.py @@ -1,4 +1,3 @@ -import json from datetime import datetime from typing import Optional @@ -18,13 +17,9 @@ from .models import ( CreateAccount, CreateAccountPermission, CreateBalanceAssertion, - CreateEntryLine, - CreateJournalEntry, CreateRole, CreateRolePermission, CreateUserEquityStatus, - EntryLine, - JournalEntry, PermissionType, Role, RolePermission, @@ -39,16 +34,6 @@ from .models import ( UserWithRoles, ) -# Import core accounting logic -from .core.validation import ( - ValidationError, - validate_journal_entry, - validate_balance, - validate_receivable_entry, - validate_expense_entry, - validate_payment_entry, -) - db = Database("ext_libra") # ===== CACHING ===== @@ -1540,10 +1525,15 @@ async def assign_user_role(data: AssignUserRole, granted_by: str) -> UserRole: notes=data.notes, ) - await db.execute( + # The unique index on (user_id, role_id) makes this insert the + # arbiter against concurrent assignments (e.g. two simultaneous + # logins both auto-assigning the default role). rowcount 0 means + # the assignment already exists — return it (idempotent). + result = await db.execute( """ INSERT INTO user_roles (id, user_id, role_id, granted_by, granted_at, expires_at, notes) VALUES (:id, :user_id, :role_id, :granted_by, :granted_at, :expires_at, :notes) + ON CONFLICT (user_id, role_id) DO NOTHING """, { "id": user_role.id, @@ -1555,6 +1545,14 @@ async def assign_user_role(data: AssignUserRole, granted_by: str) -> UserRole: "notes": user_role.notes, }, ) + if result.rowcount == 0: + existing = await db.fetchone( + "SELECT * FROM user_roles WHERE user_id = :user_id AND role_id = :role_id", + {"user_id": data.user_id, "role_id": data.role_id}, + UserRole, + ) + if existing: + return existing return user_role diff --git a/docs/ACCOUNTING-ANALYSIS-NET-SETTLEMENT.html b/docs/ACCOUNTING-ANALYSIS-NET-SETTLEMENT.html deleted file mode 100644 index 6271865..0000000 --- a/docs/ACCOUNTING-ANALYSIS-NET-SETTLEMENT.html +++ /dev/null @@ -1,953 +0,0 @@ - - - - - - - ACCOUNTING-ANALYSIS-NET-SETTLEMENT - - - - - -

Accounting -Analysis: Net Settlement Entry Pattern

-

Date: 2025-01-12 Prepared By: -Senior Accounting Review Subject: Libra Extension - -Lightning Payment Settlement Entries Status: Technical -Review

-
-

Executive Summary

-

This document provides a professional accounting assessment of -Libra’s net settlement entry pattern used for recording Lightning -Network payments that settle fiat-denominated receivables. The analysis -identifies areas where the implementation deviates from traditional -accounting best practices and provides specific recommendations for -improvement.

-

Key Findings: - ✅ Double-entry integrity maintained -- ✅ Functional for intended purpose - ❌ Zero-amount postings violate -accounting principles - ❌ Redundant satoshi tracking - ❌ No exchange -gain/loss recognition - ⚠️ Mixed currency approach lacks clear -hierarchy

-
-

Background: The Technical -Challenge

-

Libra operates as a Lightning Network-integrated accounting system -for collectives (co-living spaces, makerspaces). It faces a unique -accounting challenge:

-

Scenario: User creates a receivable in EUR (e.g., -€200 for room rent), then pays via Lightning Network in satoshis -(225,033 sats).

-

Challenge: Record the payment while: 1. Clearing the -exact EUR receivable amount 2. Recording the exact satoshi amount -received 3. Handling cases where users have both receivables (owe -Libra) and payables (Libra owes them) 4. Maintaining Beancount -double-entry balance

-
-

Current Implementation

-

Transaction Example

-
; Step 1: Receivable Created
-2025-11-12 * "room (200.00 EUR)" #receivable-entry
-  user-id: "375ec158"
-  source: "libra-api"
-  sats-amount: "225033"
-  Assets:Receivable:User-375ec158     200.00 EUR
-    sats-equivalent: "225033"
-  Income:Accommodation:Guests        -200.00 EUR
-    sats-equivalent: "225033"
-
-; Step 2: Lightning Payment Received
-2025-11-12 * "Lightning payment settlement from user 375ec158"
-  #lightning-payment #net-settlement
-  user-id: "375ec158"
-  source: "lightning_payment"
-  payment-type: "net-settlement"
-  payment-hash: "8d080ec4cc4301715535004156085dd50c159185..."
-  Assets:Bitcoin:Lightning            225033 SATS @ 0.0008887585... EUR
-    payment-hash: "8d080ec4cc4301715535004156085dd50c159185..."
-  Assets:Receivable:User-375ec158    -200.00 EUR
-    sats-equivalent: "225033"
-  Liabilities:Payable:User-375ec158     0.00 EUR
-

Code Implementation

-

Location: -beancount_format.py:739-760

-
# Build postings for net settlement
-postings = [
-    {
-        "account": payment_account,
-        "amount": f"{abs(amount_sats)} SATS @@ {abs(net_fiat_amount):.2f} {fiat_currency}",
-        "meta": {"payment-hash": payment_hash} if payment_hash else {}
-    },
-    {
-        "account": receivable_account,
-        "amount": f"-{abs(total_receivable_fiat):.2f} {fiat_currency}",
-        "meta": {"sats-equivalent": str(abs(amount_sats))}
-    },
-    {
-        "account": payable_account,
-        "amount": f"{abs(total_payable_fiat):.2f} {fiat_currency}",
-        "meta": {}
-    }
-]
-

Three-Posting Structure: 1. Lightning -Account: Records SATS received with @@ total price -notation 2. Receivable Account: Clears EUR receivable -with sats-equivalent metadata 3. Payable Account: -Clears any outstanding EUR payables (often 0.00)

-
-

Accounting Issues Identified

-

Issue 1: Zero-Amount Postings

-

Problem: The third posting often records -0.00 EUR when no payable exists.

-
Liabilities:Payable:User-375ec158     0.00 EUR
-

Why This Is Wrong: - Zero-amount postings have no -economic substance - Clutters the journal with non-events - Violates the -principle of materiality (GAAP Concept Statement 2) - Makes auditing -more difficult (reviewers must verify why zero amounts exist)

-

Accounting Principle Violated: > “Transactions -should only include postings that represent actual economic events or -changes in account balances.”

-

Impact: Low severity, but unprofessional -presentation

-

Recommendation:

-
# Make payable posting conditional
-postings = [
-    {"account": payment_account, "amount": ...},
-    {"account": receivable_account, "amount": ...}
-]
-
-# Only add payable posting if there's actually a payable
-if total_payable_fiat > 0:
-    postings.append({
-        "account": payable_account,
-        "amount": f"{abs(total_payable_fiat):.2f} {fiat_currency}",
-        "meta": {}
-    })
-
-

Issue 2: Redundant Satoshi -Tracking

-

Problem: Satoshis are tracked in TWO places in the -same transaction:

-
    -

  1. Position Amount (via @@ -notation):

    -
    Assets:Bitcoin:Lightning  225033 SATS @@ 200.00 EUR
    2. -

  2. Metadata (sats-equivalent):

    -
    Assets:Receivable:User-375ec158  -200.00 EUR
-  sats-equivalent: "225033"
    3. -
-

Why This Is Problematic: - The @@ -notation already records the exact satoshi amount - Beancount’s price -database stores this relationship - Metadata becomes redundant for this -specific posting - Increases storage and potential for inconsistency

-

Technical Detail:

-

The @@ notation means “total price” and Beancount -converts it to per-unit price:

-
; You write:
-Assets:Bitcoin:Lightning  225033 SATS @@ 200.00 EUR
-
-; Beancount stores:
-Assets:Bitcoin:Lightning  225033 SATS @ 0.0008887585... EUR
-; (where 200.00 / 225033 = 0.0008887585...)
-

Beancount can query this:

-
SELECT account, sum(convert(position, SATS))
-WHERE account = 'Assets:Bitcoin:Lightning'
-

Recommendation:

-

Choose ONE approach consistently:

-

Option A - Use @ notation (Beancount standard):

-
Assets:Bitcoin:Lightning           225033 SATS @@ 200.00 EUR
-  payment-hash: "8d080ec4..."
-Assets:Receivable:User-375ec158   -200.00 EUR
-  ; No sats-equivalent needed here
-

Option B - Use EUR positions with metadata (Libra’s -current approach):

-
Assets:Bitcoin:Lightning           200.00 EUR
-  sats-received: "225033"
-  payment-hash: "8d080ec4..."
-Assets:Receivable:User-375ec158   -200.00 EUR
-  sats-cleared: "225033"
-

Don’t: Mix both in the same transaction (current -implementation)

-
-

Issue 3: No Exchange -Gain/Loss Recognition

-

Problem: When receivables are denominated in one -currency (EUR) and paid in another (SATS), exchange rate fluctuations -create gains or losses that should be recognized.

-

Example Scenario:

-
Day 1 - Receivable Created:
-  200 EUR = 225,033 SATS (rate: 1,125.165 sats/EUR)
-
-Day 5 - Payment Received:
-  225,033 SATS = 199.50 EUR (rate: 1,127.682 sats/EUR)
-  Exchange rate moved unfavorably
-
-Economic Reality: 0.50 EUR LOSS
-

Current Implementation: Forces balance by -calculating the @ rate to make it exactly 200 EUR:

-
Assets:Bitcoin:Lightning  225033 SATS @ 0.000888... EUR  ; = exactly 200.00 EUR
-

This hides the exchange variance by treating the -payment as if it was worth exactly the receivable amount.

-

GAAP/IFRS Requirement:

-

Under both US GAAP (ASC 830) and IFRS (IAS 21), exchange gains and -losses on monetary items (like receivables) should be recognized in the -period they occur.

-

Proper Accounting Treatment:

-
2025-11-12 * "Lightning payment with exchange loss"
-  Assets:Bitcoin:Lightning           225033 SATS @ 0.000886... EUR
-    ; Market rate at payment time = 199.50 EUR
-  Expenses:Foreign-Exchange-Loss     0.50 EUR
-  Assets:Receivable:User-375ec158   -200.00 EUR
-

Impact: Moderate severity - affects financial -statement accuracy

-

Why This Matters: - Tax reporting may require -exchange gain/loss recognition - Financial statements misstate true -economic results - Auditors would flag this as a compliance issue - -Cannot accurately calculate ROI or performance metrics

-
-

Issue 4: Semantic -Misuse of Price Notation

-

Problem: The @ notation in Beancount -represents acquisition cost, not settlement -value.

-

Current Usage:

-
Assets:Bitcoin:Lightning  225033 SATS @ 0.000888... EUR
-

What this notation means in accounting: “We -purchased 225,033 satoshis at a cost of 0.000888 EUR -per satoshi”

-

What actually happened: “We -received 225,033 satoshis as payment for a debt”

-

Economic Difference: - Purchase: -You exchange cash for an asset (buying Bitcoin) - Payment -Receipt: You receive an asset in settlement of a receivable

-

Accounting Substance vs. Form: - -Form: The transaction looks like a Bitcoin purchase - -Substance: The transaction is actually a receivable -collection

-

GAAP Principle (ASC 105-10-05): > “Accounting -should reflect the economic substance of transactions, not merely their -legal form.”

-

Why This Creates Issues:

-
    -
  1. Cost Basis Tracking: For tax purposes, the “cost” -of Bitcoin received as payment should be its fair market value at -receipt, not the receivable amount
    2. -
  2. Price Database Pollution: Beancount’s price -database now contains “prices” that aren’t real market prices
    3. -
  3. Auditor Confusion: An auditor reviewing this would -question why purchase prices don’t match market rates
    4. -
-

Proper Accounting Approach:

-
; Approach 1: Record at fair market value
-Assets:Bitcoin:Lightning           225033 SATS @ 0.000886... EUR
-  ; Using actual market price at time of receipt
-  acquisition-type: "payment-received"
-Revenue:Exchange-Gain              0.50 EUR
-Assets:Receivable:User-375ec158   -200.00 EUR
-
-; Approach 2: Don't use @ notation at all
-Assets:Bitcoin:Lightning           200.00 EUR
-  sats-received: "225033"
-  fmv-at-receipt: "199.50 EUR"
-Assets:Receivable:User-375ec158   -200.00 EUR
-
-

Issue 5: Misnamed -Function and Incorrect Usage

-

Problem: Function is called -format_net_settlement_entry, but it’s used for simple -payments that aren’t true net settlements.

-

Example from User’s Transaction: - Receivable: -200.00 EUR - Payable: 0.00 EUR - Net: 200.00 EUR (this is just a -payment, not a settlement)

-

Accounting Terminology:

-
    -
  • Payment: Settling a single obligation (receivable -OR payable)
    • -
  • Net Settlement: Offsetting multiple obligations -(receivable AND payable)
    • -
-

When Net Settlement is Appropriate:

-
User owes Libra:    555.00 EUR (receivable)
-Libra owes User:     38.00 EUR (payable)
-Net amount due:      517.00 EUR (true settlement)
-

Proper three-posting entry:

-
Assets:Bitcoin:Lightning           565251 SATS @@ 517.00 EUR
-Assets:Receivable:User            -555.00 EUR
-Liabilities:Payable:User            38.00 EUR
-; Net: 517.00 = -555.00 + 38.00 ✓
-

When Two Postings Suffice:

-
User owes Libra:    200.00 EUR (receivable)
-Libra owes User:      0.00 EUR (no payable)
-Amount due:          200.00 EUR (simple payment)
-

Simpler two-posting entry:

-
Assets:Bitcoin:Lightning           225033 SATS @@ 200.00 EUR
-Assets:Receivable:User            -200.00 EUR
-

Best Practice: Use the simplest journal entry -structure that accurately represents the transaction.

-

Recommendation: 1. Rename function to -format_payment_entry or -format_receivable_payment_entry 2. Create separate -format_net_settlement_entry for true netting scenarios 3. -Use conditional logic to choose 2-posting vs 3-posting based on whether -both receivables AND payables exist

-
-

Traditional Accounting -Approaches

-

Approach -1: Record Bitcoin at Fair Market Value (Tax Compliant)

-
2025-11-12 * "Bitcoin payment from user 375ec158"
-  Assets:Bitcoin:Lightning           199.50 EUR
-    sats-received: "225033"
-    fmv-per-sat: "0.000886 EUR"
-    cost-basis: "199.50 EUR"
-    payment-hash: "8d080ec4..."
-  Revenue:Exchange-Gain              0.50 EUR
-    source: "cryptocurrency-receipt"
-  Assets:Receivable:User-375ec158   -200.00 EUR
-

Pros: - ✅ Tax compliant (establishes cost basis) - -✅ Recognizes exchange gain/loss - ✅ Uses actual market rates - ✅ -Audit trail for cryptocurrency receipts

-

Cons: - ❌ Requires real-time price feeds - ❌ -Creates taxable events

-
-

Approach 2: -Simplified EUR-Only Ledger (No SATS Positions)

-
2025-11-12 * "Bitcoin payment from user 375ec158"
-  Assets:Bitcoin:Lightning           200.00 EUR
-    sats-received: "225033"
-    sats-rate: "1125.165"
-    payment-hash: "8d080ec4..."
-  Assets:Receivable:User-375ec158   -200.00 EUR
-

Pros: - ✅ Simple and clean - ✅ EUR positions match -accounting reality - ✅ SATS tracked in metadata for reference - ✅ No -artificial price notation

-

Cons: - ❌ SATS not queryable via Beancount -positions - ❌ Requires metadata parsing for SATS balances

-
-

Approach -3: True Net Settlement (When Both Obligations Exist)

-
2025-11-12 * "Net settlement via Lightning"
-  ; User owes 555 EUR, Libra owes 38 EUR, net: 517 EUR
-  Assets:Bitcoin:Lightning           517.00 EUR
-    sats-received: "565251"
-  Assets:Receivable:User-375ec158   -555.00 EUR
-  Liabilities:Payable:User-375ec158   38.00 EUR
-

When to Use: Only when both -receivables and payables exist and you’re truly netting them.

-
-

Recommendations

-

Priority 1: Immediate -Fixes (Easy Wins)

-

1.1 Remove Zero-Amount -Postings

-

File: beancount_format.py:739-760

-

Current Code:

-
postings = [
-    {...},  # Lightning
-    {...},  # Receivable
-    {       # Payable (always included, even if 0.00)
-        "account": payable_account,
-        "amount": f"{abs(total_payable_fiat):.2f} {fiat_currency}",
-        "meta": {}
-    }
-]
-

Fixed Code:

-
postings = [
-    {
-        "account": payment_account,
-        "amount": f"{abs(amount_sats)} SATS @@ {abs(net_fiat_amount):.2f} {fiat_currency}",
-        "meta": {"payment-hash": payment_hash} if payment_hash else {}
-    },
-    {
-        "account": receivable_account,
-        "amount": f"-{abs(total_receivable_fiat):.2f} {fiat_currency}",
-        "meta": {"sats-equivalent": str(abs(amount_sats))}
-    }
-]
-
-# Only add payable posting if there's actually a payable to clear
-if total_payable_fiat > 0:
-    postings.append({
-        "account": payable_account,
-        "amount": f"{abs(total_payable_fiat):.2f} {fiat_currency}",
-        "meta": {}
-    })
-

Impact: Cleaner journal, professional presentation, -easier auditing

-
-

1.2 Choose One SATS Tracking -Method

-

Decision Required: Select either position-based OR -metadata-based satoshi tracking.

-

Option A - Keep Metadata Approach (recommended for -Libra):

-
# In format_net_settlement_entry()
-postings = [
-    {
-        "account": payment_account,
-        "amount": f"{abs(net_fiat_amount):.2f} {fiat_currency}",  # EUR only
-        "meta": {
-            "sats-received": str(abs(amount_sats)),
-            "payment-hash": payment_hash
-        }
-    },
-    {
-        "account": receivable_account,
-        "amount": f"-{abs(total_receivable_fiat):.2f} {fiat_currency}",
-        "meta": {"sats-cleared": str(abs(amount_sats))}
-    }
-]
-

Option B - Use Position-Based Tracking:

-
# Remove sats-equivalent metadata entirely
-postings = [
-    {
-        "account": payment_account,
-        "amount": f"{abs(amount_sats)} SATS @@ {abs(net_fiat_amount):.2f} {fiat_currency}",
-        "meta": {"payment-hash": payment_hash}
-    },
-    {
-        "account": receivable_account,
-        "amount": f"-{abs(total_receivable_fiat):.2f} {fiat_currency}",
-        # No sats-equivalent needed - queryable via price database
-    }
-]
-

Recommendation: Choose Option A (metadata) for -consistency with Libra’s architecture.

-
-

1.3 Rename Function for -Clarity

-

File: beancount_format.py

-

Current: -format_net_settlement_entry()

-

New: format_receivable_payment_entry() -or format_payment_settlement_entry()

-

Rationale: More accurately describes what the -function does (processes payments, not always net settlements)

-
-

Priority 2: -Medium-Term Improvements (Compliance)

-

2.1 Add Exchange Gain/Loss -Tracking

-

File: tasks.py:259-276 (get balance and -calculate settlement)

-

New Logic:

-
# Get user's current balance
-balance = await fava.get_user_balance(user_id)
-fiat_balances = balance.get("fiat_balances", {})
-total_fiat_balance = fiat_balances.get(fiat_currency, Decimal(0))
-
-# Calculate expected fiat value of SATS payment at current market rate
-market_rate = await get_current_sats_eur_rate()  # New function needed
-market_value = Decimal(amount_sats) * market_rate
-
-# Calculate exchange variance
-receivable_amount = abs(total_fiat_balance) if total_fiat_balance > 0 else Decimal(0)
-exchange_variance = market_value - receivable_amount
-
-# If variance is material (> 1 cent), create exchange gain/loss posting
-if abs(exchange_variance) > Decimal("0.01"):
-    # Add exchange gain/loss to postings
-    if exchange_variance > 0:
-        # Gain: payment worth more than receivable
-        exchange_account = "Revenue:Foreign-Exchange-Gain"
-    else:
-        # Loss: payment worth less than receivable
-        exchange_account = "Expenses:Foreign-Exchange-Loss"
-
-    # Include in entry creation
-    exchange_posting = {
-        "account": exchange_account,
-        "amount": f"{abs(exchange_variance):.2f} {fiat_currency}",
-        "meta": {
-            "sats-amount": str(amount_sats),
-            "market-rate": str(market_rate),
-            "receivable-amount": str(receivable_amount)
-        }
-    }
-

Benefits: - ✅ Tax compliance - ✅ Accurate -financial reporting - ✅ Audit trail for cryptocurrency gains/losses - -✅ Regulatory compliance (GAAP/IFRS)

-
-

2.2 -Implement True Net Settlement vs. Simple Payment Logic

-

File: tasks.py or new -payment_logic.py

-
async def create_payment_entry(
-    user_id: str,
-    amount_sats: int,
-    fiat_amount: Decimal,
-    fiat_currency: str,
-    payment_hash: str
-):
-    """
-    Create appropriate payment entry based on user's balance situation.
-    Uses 2-posting for simple payments, 3-posting for net settlements.
-    """
-    # Get user balance
-    balance = await fava.get_user_balance(user_id)
-    fiat_balances = balance.get("fiat_balances", {})
-    total_balance = fiat_balances.get(fiat_currency, Decimal(0))
-
-    receivable_amount = Decimal(0)
-    payable_amount = Decimal(0)
-
-    if total_balance > 0:
-        receivable_amount = total_balance
-    elif total_balance < 0:
-        payable_amount = abs(total_balance)
-
-    # Determine entry type
-    if receivable_amount > 0 and payable_amount > 0:
-        # TRUE NET SETTLEMENT: Both obligations exist
-        return await format_net_settlement_entry(
-            user_id=user_id,
-            amount_sats=amount_sats,
-            receivable_amount=receivable_amount,
-            payable_amount=payable_amount,
-            fiat_amount=fiat_amount,
-            fiat_currency=fiat_currency,
-            payment_hash=payment_hash
-        )
-    elif receivable_amount > 0:
-        # SIMPLE RECEIVABLE PAYMENT: Only receivable exists
-        return await format_receivable_payment_entry(
-            user_id=user_id,
-            amount_sats=amount_sats,
-            receivable_amount=receivable_amount,
-            fiat_amount=fiat_amount,
-            fiat_currency=fiat_currency,
-            payment_hash=payment_hash
-        )
-    else:
-        # PAYABLE PAYMENT: Libra paying user (different flow)
-        return await format_payable_payment_entry(...)
-
-

Priority 3: -Long-Term Architectural Decisions

-

3.1 Establish Primary -Currency Hierarchy

-

Current Issue: Mixed approach (EUR positions with -SATS metadata, but also SATS positions with @ notation)

-

Decision Required: Choose ONE of the following -architectures:

-

Architecture A - EUR Primary, SATS Secondary -(recommended):

-
; All positions in EUR, SATS in metadata
-2025-11-12 * "Payment"
-  Assets:Bitcoin:Lightning           200.00 EUR
-    sats-received: "225033"
-  Assets:Receivable:User            -200.00 EUR
-    sats-cleared: "225033"
-

Architecture B - SATS Primary, EUR Secondary:

-
; All positions in SATS, EUR in metadata
-2025-11-12 * "Payment"
-  Assets:Bitcoin:Lightning           225033 SATS
-    eur-value: "200.00"
-  Assets:Receivable:User            -225033 SATS
-    eur-cleared: "200.00"
-

Recommendation: Architecture A (EUR primary) -because: 1. Most receivables created in EUR 2. Financial reporting -requirements typically in fiat 3. Tax obligations calculated in fiat 4. -Aligns with current Libra metadata approach

-
-

3.2 -Consider Separate Ledger for Cryptocurrency Holdings

-

Advanced Approach: Separate cryptocurrency movements -from fiat accounting

-

Main Ledger (EUR-denominated):

-
2025-11-12 * "Payment received from user"
-  Assets:Bitcoin-Custody:User-375ec158  200.00 EUR
-  Assets:Receivable:User-375ec158      -200.00 EUR
-

Cryptocurrency Sub-Ledger (SATS-denominated):

-
2025-11-12 * "Lightning payment received"
-  Assets:Bitcoin:Lightning:Libra    225033 SATS
-  Assets:Bitcoin:Custody:User-375ec  225033 SATS
-

Benefits: - ✅ Clean separation of concerns - ✅ -Cryptocurrency movements tracked independently - ✅ Fiat accounting -unaffected by Bitcoin volatility - ✅ Can generate separate financial -statements

-

Drawbacks: - ❌ Increased complexity - ❌ -Reconciliation between ledgers required - ❌ Two sets of books to -maintain

-
-

Code Files Requiring Changes

-

High Priority (Immediate -Fixes)

-
    -
  1. beancount_format.py:739-760 -
      -
    • Remove zero-amount postings
      • -
    • Make payable posting conditional
      • -
    2. -
  2. beancount_format.py:692 -
      -
    • Rename function to format_receivable_payment_entry
      • -
    3. -
-

Medium Priority (Compliance)

-
    -
  1. tasks.py:235-310 -
      -
    • Add exchange gain/loss calculation
      • -
    • Implement payment vs. settlement logic
      • -
    2. -
  2. New file: exchange_rates.py -
      -
    • Create get_current_sats_eur_rate() function
      • -
    • Implement price feed integration
      • -
    3. -
  3. beancount_format.py -
      -
    • Create new format_net_settlement_entry() for true -netting
      • -
    • Create format_receivable_payment_entry() for simple -payments
      • -
    4. -
-
-

Testing Requirements

-

Test Case 1: -Simple Receivable Payment (No Payable)

-

Setup: - User has receivable: 200.00 EUR - User has -payable: 0.00 EUR - User pays: 225,033 SATS

-

Expected Entry (after fixes):

-
2025-11-12 * "Lightning payment from user"
-  Assets:Bitcoin:Lightning           200.00 EUR
-    sats-received: "225033"
-    payment-hash: "8d080ec4..."
-  Assets:Receivable:User            -200.00 EUR
-    sats-cleared: "225033"
-

Verify: - ✅ Only 2 postings (no zero-amount -payable) - ✅ Entry balances - ✅ SATS tracked in metadata - ✅ User -balance becomes 0 (both EUR and SATS)

-
-

Test Case 2: True Net -Settlement

-

Setup: - User has receivable: 555.00 EUR - User has -payable: 38.00 EUR - Net owed: 517.00 EUR - User pays: 565,251 SATS -(worth 517.00 EUR)

-

Expected Entry:

-
2025-11-12 * "Net settlement via Lightning"
-  Assets:Bitcoin:Lightning           517.00 EUR
-    sats-received: "565251"
-    payment-hash: "abc123..."
-  Assets:Receivable:User            -555.00 EUR
-    sats-portion: "565251"
-  Liabilities:Payable:User            38.00 EUR
-

Verify: - ✅ 3 postings (receivable + payable -cleared) - ✅ Net amount = receivable - payable - ✅ Both balances -become 0 - ✅ Mathematically balanced

-
-

Test Case 3: Exchange -Gain/Loss (Future)

-

Setup: - User has receivable: 200.00 EUR (created at -1,125 sats/EUR) - User pays: 225,033 SATS (now worth 199.50 EUR at -market) - Exchange loss: 0.50 EUR

-

Expected Entry (with exchange tracking):

-
2025-11-12 * "Lightning payment with exchange loss"
-  Assets:Bitcoin:Lightning           199.50 EUR
-    sats-received: "225033"
-    market-rate: "0.000886"
-  Expenses:Foreign-Exchange-Loss     0.50 EUR
-  Assets:Receivable:User            -200.00 EUR
-

Verify: - ✅ Bitcoin recorded at fair market value - -✅ Exchange loss recognized - ✅ Receivable cleared at book value - ✅ -Entry balances

-
-

Conclusion

-

Summary of Issues

- ------ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
IssueSeverityAccounting ImpactRecommended Action
Zero-amount postingsLowPresentation onlyRemove immediately
Redundant SATS trackingLowStorage/efficiencyChoose one method
No exchange gain/lossHighFinancial accuracyImplement for compliance
Semantic misuse of @MediumAudit clarityConsider EUR-only positions
Misnamed functionLowCode clarityRename function
-

Professional Assessment

-

Is this “best practice” accounting? -No, this implementation deviates from traditional -accounting standards in several ways.

-

Is it acceptable for Libra’s use case? Yes, -with modifications, it’s a reasonable pragmatic solution for a -novel problem (cryptocurrency payments of fiat debts).

-

Critical improvements needed: 1. ✅ Remove -zero-amount postings (easy fix, professional presentation) 2. ✅ -Implement exchange gain/loss tracking (required for compliance) 3. ✅ -Separate payment vs. settlement logic (accuracy and clarity)

-

The fundamental challenge: Traditional accounting -wasn’t designed for this scenario. There is no established “standard” -for recording cryptocurrency payments of fiat-denominated receivables. -Libra’s approach is functional, but should be refined to align better -with accounting principles where possible.

-

Next Steps

-
    -
  1. Week 1: Implement Priority 1 fixes (remove zero -postings, rename function)
    2. -
  2. Week 2-3: Design and implement exchange gain/loss -tracking
    3. -
  3. Week 4: Add payment vs. settlement logic
    4. -
  4. Ongoing: Monitor regulatory guidance on -cryptocurrency accounting
    5. -
-
-

References

-
    -
  • FASB ASC 830: Foreign Currency Matters
    • -
  • IAS 21: The Effects of Changes in Foreign Exchange -Rates
    • -
  • FASB Concept Statement No. 2: Qualitative -Characteristics of Accounting Information
    • -
  • ASC 105-10-05: Substance Over Form
    • -
  • Beancount Documentation: -http://furius.ca/beancount/doc/index
    • -
  • Libra Extension: -docs/SATS-EQUIVALENT-METADATA.md
    • -
  • BQL Analysis: -docs/BQL-BALANCE-QUERIES.md
    • -
-
-

Document Version: 1.0 Last Updated: -2025-01-12 Next Review: After Priority 1 fixes -implemented

-
-

This analysis was prepared for internal review and development -planning. It represents a professional accounting assessment of the -current implementation and should be used to guide improvements to -Libra’s payment recording system.

- - diff --git a/docs/CODE-REVIEW-2026-06.md b/docs/CODE-REVIEW-2026-06.md new file mode 100644 index 0000000..a200c5c --- /dev/null +++ b/docs/CODE-REVIEW-2026-06.md @@ -0,0 +1,260 @@ +# Code review — 2026-06-05 + +Findings from a deep review of the Libra LNbits extension (12k LOC, +14 files). Each finding has `file:line` references, a one-line fix +proposal, and a status tag: + +- ✅ **fixed** — merged in commit listed +- ⏳ **outstanding** — still needs work +- 🚫 **downgraded** — initially flagged, verified not a bug on closer read + +Triage order at the bottom prioritises blast radius over file location. + +> **2026-07-12 refactor series:** findings #2–#19 fixed across PRs +> #55–#59 + the chore/hygiene branch (stacked; merge in order). LOW +> items fixed in chore/hygiene except `parse_legacy_account_name` +> fragility (documented assumption, internal input only) and the +> `is_active`/`is_virtual` filter inconsistency (still open). + + +--- + +## CRITICAL + +### ✅ #1 — Mass `require_admin_key` mis-use → cross-user privilege escalation +**Status:** fixed in `1201557` (`aiolabs/libra` main, 2026-06-05) + +`4c704e5` (`aiolabs/webapp` dev). + +27 endpoints documented "(admin only)" used `require_admin_key`, which +only checks the caller owns *some* wallet with its admin key — i.e. +any authenticated user. Cluster included `receivable`/`revenue` +creation, equity-eligibility grant/revoke, account-permission CRUD +(grant yourself MANAGE on any account → ledger god mode), role and +user-role CRUD, account-sync admin, cross-user reports. + +Also deleted the duplicate `api_pay_user` at `views_api.py:1937` (the +correctly-gated `/api/v1/payables/pay` at L2144 replaces it). + +Webapp side: deleted orphaned `PermissionManager.vue` + +`GrantPermissionDialog.vue` admin components that were never imported +or routed and whose backing API methods pointed at non-existent paths. + +### ✅ #2 — `format_net_settlement_entry` ships unbalanced postings on partial payments +`beancount_format.py:761-777` emits three postings whose weights sum to +`net_fiat − total_receivable + total_payable`. The docstring example +assumes `net_fiat == total_receivable − total_payable`. But +`tasks.py:251-258` sets `total_receivable = total_prior_balance` and +`net_fiat = invoice_fiat_amount` — only equal when the user paid the +full balance. Any partial payment ships unbalanced postings; Beancount +will reject or apply tolerance silently. + +**Fix:** add `assert abs(net_fiat − (receivable − payable)) <= 0.005` +inside the formatter and raise on violation. Then fix the caller in +`tasks.py:218-322` to settle only what the payment covers (likely a +two-posting `DR Lightning / CR Receivable`-for-payment-amount, not +net-settlement). + +### ✅ #3 — Migrations not idempotent (violates fork-migrations contract) +`migrations.py:347, 377` (`ALTER TABLE accounts ADD COLUMN +is_active/is_virtual`) and `migrations.py:441, 472, 510` (`CREATE TABLE +roles/role_permissions/user_roles`) lack idempotency guards. Seed +`INSERT`s at `migrations.py:319-330, 400-412, 587-597` have no `ON +CONFLICT DO NOTHING`. Per CLAUDE.md, the cross-DB write between +`ext_libra` and core `dbversions` is non-atomic — a failed version-bump +leaves the migration to re-run on boot and crash with `duplicate +column` / `table exists` / UNIQUE violation. Bricks the extension until +manual `dbversions` surgery. + +**Fix:** wrap ALTERs with `_alter_add_column_safe`, switch CREATEs to +`CREATE TABLE IF NOT EXISTS`, gate seed INSERTs with `INSERT ... ON +CONFLICT DO NOTHING`. + +### ✅ #4 — Lightning payment recording has no local idempotency gate +`tasks.py:218-322` relies entirely on `fava.add_entry_idempotent` for +dedup, which itself does a read-then-write race on the Fava ledger. On +lnbits restart with a persisted invoice queue, the same `payment_hash` +can re-fire; the per-user lock is in-process only and doesn't survive +restart. Webhook + poller hitting concurrently both pass the +"not present" check and both insert. + +**Fix:** add a `processed_payments(payment_hash TEXT PRIMARY KEY)` +table; `INSERT OR IGNORE` at the top of `on_invoice_paid`; only +proceed if `rowcount == 1`. + +--- + +## HIGH + +### ✅ #5 — Auth prefix-match in `can_access_user_data` +`auth.py:248-251` uses `caller.user_id[:8] == target.user_id[:8]`. +Eight hex chars = 32 bits; birthday collision at ~65k users. + +**Fix:** require full UUID equality; never resolve by prefix in an +authorisation decision. + +### ✅ #6 — `can_access_account` substring match +`auth.py:178-180` does `f"User-{short}" in account.name` — matches +`Expenses:Misc-User-deadbeef` too. + +**Fix:** split on `:`, require segment equality. + +### ✅ #7 — `ChecksumConflictError` never raised by update/delete +`fava_client.py:1392-1482`: Fava 409/412 propagates as raw +`HTTPStatusError`. The `ChecksumConflictError` type exists but isn't +raised by these methods; callers see stack-trace 500s instead of a +clean retry path. + +**Fix:** `if status in (409, 412): raise ChecksumConflictError(...)`. + +### ✅ #8 — `float()` arithmetic in fiat-rate metadata +`views_api.py:1061-1062, 1263-1264, 1364-1365, 1759-1760` compute +`fiat_rate` / `btc_rate` via `float()`, then persist into Beancount +metadata as the cost basis for that entry. Float drift cascades +through reporting. + +**Fix:** keep `Decimal` end-to-end; only stringify at JSON-serialise +time. + +### ✅ #9 — Background loop swallows `raise` in `on_invoice_paid` +`tasks.py:320-322` does `logger.error(...); raise`. +`wait_for_paid_invoices` at `tasks.py:178-180` has no surrounding +try/except, so one unhandled exception kills the listener for the +rest of the process lifetime — no further Lightning payments get +recorded, no alarm. + +**Fix:** wrap the iteration body in +`try/except Exception: logger.exception(...)`; never `raise` from +`on_invoice_paid`. + +### ✅ #10 — `record-payment` dedup is non-atomic AND exception-swallowing +`views_api.py:1841-1924` (per subagent report — needs verification) +catches all exceptions in the dedup window with a 5-second timeout +and treats Fava errors as "not duplicate", producing double-entries +on transient Fava blips. + +**Fix:** fail closed on transport error; narrow the exception type +catch to `httpx.HTTPError` only. + +### ✅ #11 — `validate_journal_entry` is stale +`core/validation.py:21-93` validates the pre-string-amount model — +sums one bag of integers, doesn't balance per currency. Doesn't match +production data shape post-Fava migration. + +**Fix:** rewrite to parse `"X CCY"` strings and balance per currency, +or delete if Beancount-side validation is now considered sufficient. + +--- + +## MEDIUM + +### ✅ #12 — `m001_initial` seed `INSERT` into `accounts` non-idempotent +`migrations.py:319-330` — same shape as #3. + +### ✅ #13 — `format_posting_at_average_cost` emits `{}` when `cost_currency=None` +`beancount_format.py:256` — ` SATS {}` isn't valid Beancount +syntax; drop the braces when cost is unset. + +### ✅ #14 — Per-call `httpx.AsyncClient` instantiation in fava_client +~30 sites build a new `httpx.AsyncClient` per call. TCP handshake every +time. + +**Fix:** construct once on `FavaClient.__init__`, expose `aclose()`. + +### ✅ #15 — BQL string interpolation without quoting +`fava_client.py:250, 621, 770-775, 853-858` interpolate +`account_name` / user-id-prefix raw into BQL `WHERE account = '{...}'`. +The 8-char hex prefix is safe in practice; arbitrary `account_name` +input is not. + +**Fix:** validate against `^[A-Za-z0-9:_-]+$` before interpolation. + +### ✅ #16 — `approve_manual_payment_request` not status-guarded +`crud.py:559-579` overwrites `status='approved'` regardless of current +state. Two concurrent admins → two journal entries. + +**Fix:** `UPDATE ... WHERE id=:id AND status='pending'`, check +`rowcount == 1`. + +### ✅ #17 — Account name not validated on receivable/revenue/expense +`views_api.py:1066-1074, 1224-1236, 1369-1376, 1477-1494` accept +free-string `data.expense_account` (etc.) with no Beancount-syntax +check before lookup. + +**Fix:** enforce `^[A-Z][A-Za-z0-9:-]*$`. + +### ✅ #18 — `_get_username_from_user_id` creates a fresh LNbits DB per call +`views_api.py:697-708` opens an LNbits DB inside a per-row hot path. + +**Fix:** cache the Database instance at module load; batch-load +usernames once per request via single `IN (…)` query. + +### ✅ #19 — `get_user_balance` regex rejects decimal SATS +`fava_client.py:346, 503` patterns require `(-?\d+)` SATS. Fava's +`@@→@` normalisation can emit decimal SATS. + +**Fix:** `(-?[\d.]+)`. + +### ⏳ #20 — `fava_url` default `http://localhost:3333` and sandboxed lnbits +Loopback breaks if the lnbits service unit gains `PrivateNetwork=true`. +Not a code bug — worth documenting in deploy assumptions. + +--- + +## LOW + +- ⏳ `tasks.py` mixes `print()` with `logger.*` (`:61, 65-69, 81, 89, + 94, 162`). +- ⏳ Dead model imports in `crud.py:21-27` (`JournalEntry`, + `EntryLine`) after `entry_lines` table dropped. +- ⏳ `auto_assign_default_role` check-then-act race + (`crud.py:1609-1641`) — add UNIQUE constraint on + `user_roles(user_id, role_id)`. +- ⏳ Pydantic v1 `.dict()` calls (`crud.py:381, 400, 411, 450`) if + upstream is on v2. +- ⏳ `account_utils.parse_legacy_account_name` splits on ` - ` — + fragile if ever called on user input. +- ⏳ `Account.is_active` vs `is_virtual` default-filter inconsistency + hides virtual parents in permission-grant UI (`crud.py:146-167`). + +--- + +## 🚫 Downgraded (initially flagged, verified not a bug) + +### Subagent A — "inverted balance sign in tasks.py:251-258" +`fava_client.py:303` docstring says positive = user owes libra +(bookkeeper perspective). `tasks.py:249-250` matches. CLAUDE.md +describes the *user's* perspective (positive = libra owes user), which +is consistent at the UI layer. Both representations are internally +coherent — no bug, just a doc-vs-code perspective collision. + +### Subagent A — "fava_ledger_slug default doesn't match deploy" +Verified empirically by user: Fava in single-ledger mode appears to +accept arbitrary slugs against the JSON API, and the deploy's seed +title `"Libra Ledger"` → slugify → `libra-ledger` matches the +extension default in `models.py:159` anyway. False alarm. + +--- + +## Triage order (when picking the next item) + +1. **#2 (unbalanced net settlement) + #4 (idempotency)** — silently + corrupts the ledger on every partial Lightning payment + every + restart with a persisted invoice queue. Real-money blast radius. +2. **#3 (migrations) + #12** — guaranteed boot crash on the documented + failure mode; bricks the extension. +3. **#8 (float in fiat metadata)** — every entry written today carries + float-drift cost basis into Beancount. +4. **#9 (silent listener death)** — operational; the kind of bug + discovered when nobody can pay for a week. +5. **#5, #6 (auth narrowing)** — residual privilege risk; smaller blast + than #1 (already fixed) but worth closing. +6. Everything else, in arbitrary order; mostly hygiene. + +--- + +## Commits applied + +| Commit | Repo / branch | What | +|---|---|---| +| `1201557` | `aiolabs/libra` `main` | Gate cross-user admin endpoints behind `require_super_user`; delete duplicate `api_pay_user` | +| `4c704e5` | `aiolabs/webapp` `dev` | Delete orphaned `PermissionManager.vue` + `GrantPermissionDialog.vue` + 3 API methods + 4 dead types | diff --git a/docs/PHASE1_COMPLETE.md b/docs/PHASE1_COMPLETE.md deleted file mode 100644 index c152ef1..0000000 --- a/docs/PHASE1_COMPLETE.md +++ /dev/null @@ -1,200 +0,0 @@ -# Phase 1 Implementation - Complete ✅ - -## Summary - -We've successfully implemented the core improvements from Phase 1 of the Beancount patterns adoption: - -## ✅ Completed - -### 1. **Decimal Instead of Float for Fiat Amounts** -- **Files Changed:** - - `models.py`: Changed all fiat amount fields from `float` to `Decimal` - - `ExpenseEntry.amount` - - `ReceivableEntry.amount` - - `RevenueEntry.amount` - - `UserBalance.fiat_balances` dictionary values - - `crud.py`: Updated fiat balance calculations to use `Decimal` - - `views_api.py`: Store fiat amounts as strings with `str(amount.quantize(Decimal("0.001")))` - -- **Benefits:** - - Prevents floating point rounding errors - - Exact decimal arithmetic - - Financial-grade precision - -### 2. **Meta Field for Journal Entries** -- **Database Migration:** `m005_add_flag_and_meta` - - Added `meta TEXT DEFAULT '{}'` column to `journal_entries` table - -- **Model Changes:** - - Added `meta: dict = {}` to `JournalEntry` and `CreateJournalEntry` - - Meta stores: source, created_via, user_id, payment_hash, etc. - -- **CRUD Updates:** - - `create_journal_entry()` now stores meta as JSON - - `get_journal_entries_by_user()` parses meta from JSON - -- **API Integration:** - - Expense entries: `{"source": "api", "created_via": "expense_entry", "user_id": "...", "is_equity": false}` - - Receivable entries: `{"source": "api", "created_via": "receivable_entry", "debtor_user_id": "..."}` - - Payment entries: `{"source": "lightning_payment", "created_via": "record_payment", "payment_hash": "...", "payer_user_id": "..."}` - -- **Benefits:** - - Full audit trail for every transaction - - Source tracking (where did this entry come from?) - - Can add tags, links, notes in future - - Essential for compliance and debugging - -### 3. **Flag Field for Transaction Status** -- **Database Migration:** `m005_add_flag_and_meta` - - Added `flag TEXT DEFAULT '*'` column to `journal_entries` table - -- **Model Changes:** - - Created `JournalEntryFlag` enum: - - `*` = CLEARED (confirmed/reconciled) - - `!` = PENDING (awaiting confirmation) - - `#` = FLAGGED (needs review) - - `x` = VOID (cancelled) - - Added `flag: JournalEntryFlag` to `JournalEntry` and `CreateJournalEntry` - -- **CRUD Updates:** - - `create_journal_entry()` stores flag as string value - - `get_journal_entries_by_user()` converts string to enum - -- **API Logic:** - - Expense entries: Default to CLEARED (immediately confirmed) - - Receivable entries: Start as PENDING (unpaid debt) - - Payment entries: Mark as CLEARED (payment received) - -- **Benefits:** - - Visual indication of transaction status in UI - - Filter transactions by status - - Supports reconciliation workflows - - Standard accounting practice (Beancount-style) - -## 📊 Migration Details - -**Migration `m005_add_flag_and_meta`:** -```sql -ALTER TABLE journal_entries ADD COLUMN flag TEXT DEFAULT '*'; -ALTER TABLE journal_entries ADD COLUMN meta TEXT DEFAULT '{}'; -``` - -**To Apply:** -1. Stop LNbits server (if running) -2. Restart LNbits - migration runs automatically -3. Check logs for "m005_add_flag_and_meta" success message - -## 🔧 Technical Implementation Details - -### Decimal Handling -```python -# Store as string for precision -metadata = { - "fiat_amount": str(data.amount.quantize(Decimal("0.001"))), -} - -# Parse back to Decimal -fiat_decimal = Decimal(str(fiat_amount)) -``` - -### Flag Handling -```python -# Set flag on creation -entry_data = CreateJournalEntry( - flag=JournalEntryFlag.PENDING, # or CLEARED - # ... -) - -# Parse from database -flag = JournalEntryFlag(entry_data.get("flag", "*")) -``` - -### Meta Handling -```python -# Create with meta -entry_meta = { - "source": "api", - "created_via": "expense_entry", - "user_id": wallet.wallet.user, -} - -entry_data = CreateJournalEntry( - meta=entry_meta, - # ... -) - -# Parse from database -meta = json.loads(entry_data.get("meta", "{}")) if entry_data.get("meta") else {} -``` - -## 🎯 What's Next (Remaining Phase 1 Items) - -### Hierarchical Account Naming (In Progress) -Implement Beancount-style account hierarchy: -- Current: `"Accounts Receivable - af983632"` -- Better: `"Assets:Receivable:User-af983632"` - -### UI Updates for Flags -Display flag icons in transaction list: -- ✅ `*` = Green checkmark (cleared) -- ⚠️ `!` = Yellow/Orange badge (pending) -- 🚩 `#` = Red flag (needs review) -- ❌ `x` = Strikethrough (voided) - -## 🧪 Testing Recommendations - -1. **Test Decimal Precision:** - ```python - # Create expense with fiat amount - POST /api/v1/entries/expense - {"amount": "36.93", "currency": "EUR", ...} - - # Verify stored as exact string - SELECT metadata FROM entry_lines WHERE ... - # Should see: {"fiat_amount": "36.930", ...} - ``` - -2. **Test Flag Workflow:** - ```python - # Create receivable (should be PENDING) - POST /api/v1/entries/receivable - # Check: flag = '!' - - # Pay receivable (creates CLEARED entry) - POST /api/v1/record-payment - # Check: payment entry flag = '*' - ``` - -3. **Test Meta Audit Trail:** - ```python - # Create any entry - # Check database: - SELECT meta FROM journal_entries WHERE ... - # Should see: {"source": "api", "created_via": "...", ...} - ``` - -## 🎉 Success Metrics - -- ✅ No more floating point errors in fiat calculations -- ✅ Every transaction has source tracking -- ✅ Transaction status is visible (pending vs cleared) -- ✅ Database migration successful -- ✅ All API endpoints updated -- ✅ CRUD operations handle new fields - -## 📝 Notes - -- **Backward Compatibility:** Old entries will have default values (`flag='*'`, `meta='{}'`) -- **Performance:** No impact - added columns have defaults and indexes not needed yet -- **Storage:** Minimal increase (meta typically < 200 bytes per entry) - -## ✅ Phase 1 Complete! - -All Phase 1 tasks have been completed: -1. ✅ Decimal instead of float for fiat amounts -2. ✅ Meta field for journal entries (audit trail) -3. ✅ Flag field for transaction status -4. ✅ Hierarchical account naming (Beancount-style) -5. ✅ UI updated to display flags and metadata - -**Next:** Move to Phase 2 (Core logic refactoring) when ready. diff --git a/docs/PHASE2_COMPLETE.md b/docs/PHASE2_COMPLETE.md deleted file mode 100644 index cc45614..0000000 --- a/docs/PHASE2_COMPLETE.md +++ /dev/null @@ -1,273 +0,0 @@ -# Phase 2: Reconciliation - COMPLETE ✅ - -## Summary - -Phase 2 of the Beancount-inspired refactor focused on **reconciliation and automated balance checking**. This phase builds on Phase 1's foundation to provide robust reconciliation tools that ensure accounting accuracy and catch discrepancies early. - -## Completed Features - -### 1. Balance Assertions ✅ - -**Purpose**: Verify account balances match expected values at specific points in time (like Beancount's `balance` directive) - -**Implementation**: -- **Models** (`models.py:184-219`): - - `AssertionStatus` enum (pending, passed, failed) - - `BalanceAssertion` model with sats and optional fiat checks - - `CreateBalanceAssertion` request model - -- **Database** (`migrations.py:275-320`): - - `balance_assertions` table with expected/actual balance tracking - - Tolerance levels for flexible matching - - Status tracking and timestamps - - Indexes for performance - -- **CRUD** (`crud.py:773-981`): - - `create_balance_assertion()` - Create and store assertion - - `get_balance_assertion()` - Fetch single assertion - - `get_balance_assertions()` - List with filters - - `check_balance_assertion()` - Compare expected vs actual - - `delete_balance_assertion()` - Remove assertion - -- **API Endpoints** (`views_api.py:1067-1230`): - - `POST /api/v1/assertions` - Create and check assertion - - `GET /api/v1/assertions` - List assertions with filters - - `GET /api/v1/assertions/{id}` - Get specific assertion - - `POST /api/v1/assertions/{id}/check` - Re-check assertion - - `DELETE /api/v1/assertions/{id}` - Delete assertion - -- **UI** (`templates/libra/index.html:254-378`): - - Balance Assertions card (super user only) - - Failed assertions prominently displayed with red banner - - Passed assertions in collapsible panel - - Create assertion dialog with validation - - Re-check and delete buttons - -- **Frontend** (`static/js/index.js:70-79, 602-726`): - - Data properties and computed values - - CRUD methods for assertions - - Automatic loading on page load - -### 2. Reconciliation API Endpoints ✅ - -**Purpose**: Provide comprehensive reconciliation tools and reporting - -**Implementation**: -- **Summary Endpoint** (`views_api.py:1236-1287`): - - `GET /api/v1/reconciliation/summary` - - Returns counts of assertions by status - - Returns counts of journal entries by flag - - Total accounts count - - Last checked timestamp - -- **Check All Endpoint** (`views_api.py:1290-1325`): - - `POST /api/v1/reconciliation/check-all` - - Re-checks all balance assertions - - Returns summary of results (passed/failed/errors) - - Useful for manual reconciliation runs - -- **Discrepancies Endpoint** (`views_api.py:1328-1357`): - - `GET /api/v1/reconciliation/discrepancies` - - Returns all failed assertions - - Returns all flagged journal entries - - Returns all pending entries - - Total discrepancy count - -### 3. Reconciliation UI Dashboard ✅ - -**Purpose**: Visual dashboard for reconciliation status and quick access to reconciliation tools - -**Implementation** (`templates/libra/index.html:380-499`): -- **Summary Cards**: - - Balance Assertions stats (total, passed, failed, pending) - - Journal Entries stats (total, cleared, pending, flagged) - - Total Accounts count with last checked timestamp - -- **Discrepancies Alert**: - - Warning banner when discrepancies found - - Shows count of failed assertions and flagged entries - - "View Details" button to expand discrepancy list - -- **Discrepancy Details**: - - Failed assertions list with expected vs actual balances - - Flagged entries list - - Quick access to problematic transactions - -- **Actions**: - - "Check All" button to run full reconciliation - - Loading states during checks - - Success message when all accounts reconciled - -**Frontend** (`static/js/index.js:80-85, 727-779, 933-934`): -- Reconciliation data properties -- Methods to load summary and discrepancies -- `runFullReconciliation()` method with notifications -- Automatic loading on page load for super users - -### 4. Automated Daily Balance Checks ✅ - -**Purpose**: Run balance checks automatically on a schedule to catch discrepancies early - -**Implementation**: - -- **Tasks Module** (`tasks.py`): - - `check_all_balance_assertions()` - Core checking logic - - `scheduled_daily_reconciliation()` - Scheduled wrapper - - Results logging and reporting - - Error handling - -- **API Endpoint** (`views_api.py:1363-1390`): - - `POST /api/v1/tasks/daily-reconciliation` - - Can be triggered manually or via cron - - Returns detailed results - - Super user only - -- **Documentation** (`DAILY_RECONCILIATION.md`): - - Comprehensive setup guide - - Multiple scheduling options (cron, systemd, k8s) - - Monitoring and troubleshooting - - Best practices - - Example scripts - -## Benefits - -### Accounting Accuracy -- ✅ Catch data entry errors early -- ✅ Verify balances at critical checkpoints -- ✅ Build confidence in accounting accuracy -- ✅ Required for external audits - -### Operational Excellence -- ✅ Automated daily checks reduce manual work -- ✅ Dashboard provides at-a-glance reconciliation status -- ✅ Discrepancies are immediately visible -- ✅ Historical tracking of assertions - -### Developer Experience -- ✅ Clean API for programmatic reconciliation -- ✅ Well-documented scheduling options -- ✅ Flexible tolerance levels -- ✅ Comprehensive error reporting - -## File Changes - -### New Files Created -1. `tasks.py` - Background tasks for automated reconciliation -2. `DAILY_RECONCILIATION.md` - Setup and scheduling documentation -3. `PHASE2_COMPLETE.md` - This file - -### Modified Files -1. `models.py` - Added `BalanceAssertion`, `CreateBalanceAssertion`, `AssertionStatus` -2. `migrations.py` - Added `m007_balance_assertions` migration -3. `crud.py` - Added balance assertion CRUD operations -4. `views_api.py` - Added assertion, reconciliation, and task endpoints -5. `templates/libra/index.html` - Added assertions and reconciliation UI -6. `static/js/index.js` - Added assertion and reconciliation functionality -7. `BEANCOUNT_PATTERNS.md` - Updated roadmap to mark Phase 2 complete - -## API Endpoints Summary - -### Balance Assertions -- `POST /api/v1/assertions` - Create assertion -- `GET /api/v1/assertions` - List assertions -- `GET /api/v1/assertions/{id}` - Get assertion -- `POST /api/v1/assertions/{id}/check` - Re-check assertion -- `DELETE /api/v1/assertions/{id}` - Delete assertion - -### Reconciliation -- `GET /api/v1/reconciliation/summary` - Get reconciliation summary -- `POST /api/v1/reconciliation/check-all` - Check all assertions -- `GET /api/v1/reconciliation/discrepancies` - Get discrepancies - -### Automated Tasks -- `POST /api/v1/tasks/daily-reconciliation` - Run daily reconciliation check - -## Usage Examples - -### Create a Balance Assertion -```bash -curl -X POST http://localhost:5000/libra/api/v1/assertions \ - -H "X-Api-Key: ADMIN_KEY" \ - -H "Content-Type: application/json" \ - -d '{ - "account_id": "lightning", - "expected_balance_sats": 268548, - "tolerance_sats": 100 - }' -``` - -### Get Reconciliation Summary -```bash -curl http://localhost:5000/libra/api/v1/reconciliation/summary \ - -H "X-Api-Key: ADMIN_KEY" -``` - -### Run Full Reconciliation -```bash -curl -X POST http://localhost:5000/libra/api/v1/reconciliation/check-all \ - -H "X-Api-Key: ADMIN_KEY" -``` - -### Schedule Daily Reconciliation (Cron) -```bash -# Add to crontab -0 2 * * * curl -X POST http://localhost:5000/libra/api/v1/tasks/daily-reconciliation -H "X-Api-Key: ADMIN_KEY" -``` - -## Testing Checklist - -- [x] Create balance assertion (UI) -- [x] Create balance assertion (API) -- [x] Assertion passes when balance matches -- [x] Assertion fails when balance doesn't match -- [x] Tolerance levels work correctly -- [x] Fiat balance assertions work -- [x] Re-check assertion updates status -- [x] Delete assertion removes it -- [x] Reconciliation summary shows correct stats -- [x] Check all assertions endpoint works -- [x] Discrepancies endpoint returns correct data -- [x] Dashboard displays summary correctly -- [x] Discrepancy alert shows when issues exist -- [x] "Check All" button triggers reconciliation -- [x] Daily reconciliation task executes successfully -- [x] Failed assertions are logged -- [x] All endpoints require super user access - -## Next Steps - -**Phase 3: Core Logic Refactoring (Medium Priority)** -- Create `core/` module with pure accounting logic -- Implement `LibraInventory` for position tracking -- Move balance calculation to `core/balance.py` -- Add comprehensive validation in `core/validation.py` - -**Phase 4: Validation Plugins (Medium Priority)** -- Create plugin system architecture -- Implement `check_balanced` plugin -- Implement `check_receivables` plugin -- Add plugin configuration UI - -**Phase 5: Advanced Features (Low Priority)** -- Add tags and links to entries -- Implement query language -- Add lot tracking to inventory -- Support multi-currency in single entry - -## Conclusion - -Phase 2 successfully implements Beancount's reconciliation philosophy in the Libra extension. With balance assertions, comprehensive reconciliation APIs, a visual dashboard, and automated daily checks, users can: - -- **Trust their data** with automated verification -- **Catch errors early** through regular reconciliation -- **Save time** with automated daily checks -- **Gain confidence** in their accounting accuracy - -The implementation follows Beancount's best practices while adapting to LNbits' architecture and use case. All reconciliation features are admin-only, ensuring proper access control for sensitive accounting operations. - -**Phase 2 Status**: ✅ COMPLETE - ---- - -*Generated: 2025-10-23* -*Next: Phase 3 - Core Logic Refactoring* diff --git a/docs/PHASE3_COMPLETE.md b/docs/PHASE3_COMPLETE.md deleted file mode 100644 index b53625a..0000000 --- a/docs/PHASE3_COMPLETE.md +++ /dev/null @@ -1,365 +0,0 @@ -# Phase 3: Core Logic Refactoring - COMPLETE ✅ - -## Summary - -Phase 3 of the Beancount-inspired refactor focused on **separating business logic from database operations** and creating a clean, testable core module. This phase improves code quality, maintainability, and follows best practices from Beancount's architecture. - -## Completed Features - -### 1. Core Module Structure ✅ - -**Purpose**: Separate pure accounting logic from database and API concerns - -**Implementation** (`core/__init__.py`): -- Created `core/` module package -- Exports main classes and functions -- Clean separation of concerns - -**Benefits**: -- Testable without database -- Reusable across different storage backends -- Easier to audit and verify -- Clear architecture - -### 2. LibraInventory for Position Tracking ✅ - -**Purpose**: Track balances across multiple currencies with cost basis information (following Beancount's Inventory pattern) - -**Implementation** (`core/inventory.py`): - -**LibraPosition** (Lines 11-84): -- Immutable dataclass representing a single position -- Tracks currency, amount, cost basis, and metadata -- Supports addition and negation operations -- Automatic Decimal conversion in `__post_init__` - -```python -@dataclass(frozen=True) -class LibraPosition: - currency: str # "SATS", "EUR", "USD" - amount: Decimal - cost_currency: Optional[str] = None - cost_amount: Optional[Decimal] = None - date: Optional[datetime] = None - metadata: Dict[str, Any] = field(default_factory=dict) -``` - -**LibraInventory** (Lines 87-201): -- Container for multiple positions -- Positions keyed by `(currency, cost_currency)` tuple -- Methods for querying balances: - - `get_balance_sats()` - Total satoshis - - `get_balance_fiat(currency)` - Fiat balance for specific currency - - `get_all_fiat_balances()` - All fiat balances -- Utility methods: - - `is_empty()` - Check if no positions - - `is_zero()` - Check if all positions sum to zero - - `to_dict()` - Export to dictionary - -### 3. BalanceCalculator ✅ - -**Purpose**: Pure logic for calculating balances from journal entries - -**Implementation** (`core/balance.py`): - -**AccountType Enum** (Lines 13-19): -```python -class AccountType(str, Enum): - ASSET = "asset" - LIABILITY = "liability" - EQUITY = "equity" - REVENUE = "revenue" - EXPENSE = "expense" -``` - -**BalanceCalculator Class** (Lines 22-217): - -**Static Methods**: - -1. **`calculate_account_balance()`** (Lines 29-54): - - Calculate balance based on account type - - Normal balances: - - Assets/Expenses: Debit balance (debit - credit) - - Liabilities/Equity/Revenue: Credit balance (credit - debit) - -2. **`build_inventory_from_entry_lines()`** (Lines 56-117): - - Build LibraInventory from journal entry lines - - Handles both sats and fiat currency tracking - - Accounts for account type when determining sign - -3. **`calculate_user_balance()`** (Lines 119-168): - - Calculate user's total balance across all accounts - - Returns both sats balance and fiat balances by currency - - Properly handles asset (receivable) vs liability (payable) accounts - -4. **`check_balance_matches()`** (Lines 170-187): - - Verify balance assertion for sats - -5. **`check_fiat_balance_matches()`** (Lines 189-202): - - Verify balance assertion for fiat currency - -### 4. Comprehensive Validation ✅ - -**Purpose**: Validation rules for accounting operations - -**Implementation** (`core/validation.py`): - -**ValidationError Exception** (Lines 10-18): -- Custom exception for validation failures -- Includes detailed error information - -**Validation Functions**: - -1. **`validate_journal_entry()`** (Lines 21-124): - - Checks: - - At least 2 lines (double-entry requirement) - - Entry is balanced (debits = credits) - - Valid amounts (non-negative) - - No line has both debit and credit - - All lines have account_id - -2. **`validate_balance()`** (Lines 127-177): - - Validates balance assertions - - Checks both sats and fiat within tolerance - -3. **`validate_receivable_entry()`** (Lines 180-199): - - Validates receivable (user owes libra) entries - - Ensures positive amount - - Ensures revenue account type - -4. **`validate_expense_entry()`** (Lines 202-227): - - Validates expense entries - - Ensures positive amount - - Checks account type (expense or equity) - -5. **`validate_payment_entry()`** (Lines 230-245): - - Validates payment entries - - Ensures positive amount - -6. **`validate_metadata()`** (Lines 248-284): - - Validates entry line metadata - - Checks for required keys - - Validates fiat currency/amount consistency - - Validates Decimal conversion - -### 5. Refactored CRUD Operations ✅ - -**Purpose**: Use core logic in database operations - -**Modified Files**: `crud.py` - -**Changes**: - -1. **Imports** (Lines 26-36): - - Import core accounting logic - - Import validation functions - -2. **`get_account_balance()`** (Lines 347-377): - - Refactored to use `BalanceCalculator.calculate_account_balance()` - - Removed duplicate logic - -3. **`get_user_balance()`** (Lines 380-435): - - Completely refactored to use: - - `BalanceCalculator.build_inventory_from_entry_lines()` - - `BalanceCalculator.calculate_user_balance()` - - Cleaner separation of database queries vs business logic - -4. **`get_all_user_balances()`** (Lines 438-459): - - Simplified to call `get_user_balance()` for each user - - Eliminates code duplication - -## Architecture - -### Before Phase 3 - -``` -views_api.py → crud.py (mixed DB + logic) - ↓ - database -``` - -All accounting logic was embedded in crud.py alongside database operations. - -### After Phase 3 - -``` -views_api.py → crud.py → core/ - ↓ ↓ - database Pure Logic - (testable) -``` - -**Separation of Concerns**: -- `core/` - Pure accounting logic (no DB dependencies) -- `crud.py` - Database operations + orchestration -- `views_api.py` - HTTP API layer - -## Benefits - -### Code Quality -- ✅ **Testability**: Core logic can be tested without database -- ✅ **Maintainability**: Clear separation makes code easier to understand -- ✅ **Reusability**: Core logic can be used in different contexts -- ✅ **Consistency**: Centralized accounting rules - -### Developer Experience -- ✅ **Type Safety**: Immutable dataclasses with proper types -- ✅ **Documentation**: Well-documented core functions -- ✅ **Debugging**: Easier to trace accounting logic -- ✅ **Refactoring**: Safer to make changes - -### Reliability -- ✅ **Validation**: Comprehensive validation rules -- ✅ **Correctness**: Pure functions easier to verify -- ✅ **Auditability**: Clear accounting rules - -## File Structure - -``` -lnbits/extensions/libra/ -├── core/ -│ ├── __init__.py # Module exports -│ ├── inventory.py # LibraInventory, LibraPosition -│ ├── balance.py # BalanceCalculator -│ └── validation.py # Validation functions -├── crud.py # DB operations (refactored to use core/) -├── models.py # Pydantic models -├── views_api.py # API endpoints -└── PHASE3_COMPLETE.md # This file -``` - -## Usage Examples - -### Using LibraInventory - -```python -from decimal import Decimal -from libra.core.inventory import LibraInventory, LibraPosition - -# Create inventory -inv = LibraInventory() - -# Add positions -inv.add_position(LibraPosition( - currency="SATS", - amount=Decimal("100000") -)) - -inv.add_position(LibraPosition( - currency="SATS", - amount=Decimal("50000"), - cost_currency="EUR", - cost_amount=Decimal("25.00") -)) - -# Query balances -total_sats = inv.get_balance_sats() # Decimal("150000") -eur_balance = inv.get_balance_fiat("EUR") # Decimal("25.00") - -# Export -data = inv.to_dict() -# {"sats": 150000, "fiat": {"EUR": 25.00}} -``` - -### Using BalanceCalculator - -```python -from libra.core.balance import BalanceCalculator, AccountType - -# Calculate account balance -balance = BalanceCalculator.calculate_account_balance( - total_debit=100000, - total_credit=50000, - account_type=AccountType.ASSET -) -# Returns: 50000 (debit balance for asset) - -# Build inventory from entry lines -entry_lines = [ - {"amount": 100000, "metadata": '{"fiat_currency": "EUR", "fiat_amount": "50.00"}'}, # Positive = debit - {"amount": -50000, "metadata": "{}"} # Negative = credit -] - -inventory = BalanceCalculator.build_inventory_from_entry_lines( - entry_lines, - AccountType.ASSET -) - -# Check balance matches -is_valid = BalanceCalculator.check_balance_matches( - actual_balance_sats=100000, - expected_balance_sats=99900, - tolerance_sats=100 -) -# Returns: True (within tolerance) -``` - -### Using Validation - -```python -from libra.core.validation import validate_journal_entry, ValidationError - -entry = { - "id": "abc123", - "description": "Test entry", - "entry_date": datetime.now() -} - -entry_lines = [ - {"account_id": "acc1", "amount": 100000}, # Positive = debit - {"account_id": "acc2", "amount": -100000} # Negative = credit -] - -try: - validate_journal_entry(entry, entry_lines) - print("Valid!") -except ValidationError as e: - print(f"Invalid: {e.message}") - print(f"Details: {e.details}") -``` - -## Testing Checklist - -- [x] LibraInventory created and tested -- [x] LibraPosition addition works -- [x] Inventory balance calculations work -- [x] BalanceCalculator account balance calculation works -- [x] BalanceCalculator inventory building works -- [x] BalanceCalculator user balance calculation works -- [x] Validation functions work -- [x] crud.py refactored to use core logic -- [x] Existing balance calculations still work -- [ ] Unit tests for core module (future work) - -## Next Steps - -**Phase 4: Validation Plugins** (Medium Priority) -- Create plugin system architecture -- Implement `check_balanced` plugin -- Implement `check_receivables` plugin -- Add plugin configuration UI - -**Future Enhancements**: -- Add unit tests for core/ module -- Add integration tests -- Add lot tracking to inventory -- Support multi-currency in single entry -- Add more validation plugins - -## Conclusion - -Phase 3 successfully refactors Libra's accounting logic into a clean, testable core module. By following Beancount's architecture patterns, we've created: - -- **Pure accounting logic** separated from database concerns -- **LibraInventory** for position tracking across currencies -- **BalanceCalculator** for consistent balance calculations -- **Comprehensive validation** for data integrity - -The refactoring improves code quality, maintainability, and sets the foundation for Phase 4's plugin system. - -**Phase 3 Status**: ✅ COMPLETE - ---- - -*Generated: 2025-10-23* -*Next: Phase 4 - Validation Plugins* diff --git a/fava_client.py b/fava_client.py index 057562a..1cca139 100644 --- a/fava_client.py +++ b/fava_client.py @@ -1409,9 +1409,12 @@ class FavaClient: logger.warning(f"Failed to fetch {endpoint}: {e}") # Filter out synthetic entries like "Net Profit" + from .account_utils import ACCOUNT_TYPE_ROOTS + + valid_roots = set(ACCOUNT_TYPE_ROOTS.values()) account_names = { name for name in account_names - if ":" in name or name in ("Assets", "Liabilities", "Equity", "Income", "Expenses") + if ":" in name or name in valid_roots } if account_names: diff --git a/migrations.py b/migrations.py index d8a4bba..1e661c5 100644 --- a/migrations.py +++ b/migrations.py @@ -651,3 +651,30 @@ async def m005_add_processed_payments(db): ); """ ) + + +async def m006_unique_user_roles(db): + """ + Enforce one assignment per (user, role). + + auto_assign_default_role's check-then-act let two concurrent logins + both pass the "no roles yet" check and insert twice. The unique + index makes the insert itself the arbiter (assign_user_role now + uses ON CONFLICT DO NOTHING against it). + """ + # Remove duplicate assignments before creating the index (keep one + # deterministic row per pair). + await db.execute( + """ + DELETE FROM user_roles + WHERE id NOT IN ( + SELECT min(id) FROM user_roles GROUP BY user_id, role_id + ) + """ + ) + await db.execute( + """ + CREATE UNIQUE INDEX IF NOT EXISTS idx_user_roles_unique + ON user_roles (user_id, role_id) + """ + ) diff --git a/migrations_old.py.bak b/migrations_old.py.bak deleted file mode 100644 index a412e3e..0000000 --- a/migrations_old.py.bak +++ /dev/null @@ -1,651 +0,0 @@ -async def m001_initial(db): - """ - Initial migration for Castle accounting extension. - Creates tables for double-entry bookkeeping system. - """ - await db.execute( - f""" - CREATE TABLE accounts ( - id TEXT PRIMARY KEY, - name TEXT NOT NULL, - account_type TEXT NOT NULL, - description TEXT, - user_id TEXT, - created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now} - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_accounts_user_id ON accounts (user_id); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_accounts_type ON accounts (account_type); - """ - ) - - await db.execute( - f""" - CREATE TABLE journal_entries ( - id TEXT PRIMARY KEY, - description TEXT NOT NULL, - entry_date TIMESTAMP NOT NULL, - created_by TEXT NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}, - reference TEXT - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_journal_entries_created_by ON journal_entries (created_by); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_journal_entries_date ON journal_entries (entry_date); - """ - ) - - await db.execute( - f""" - CREATE TABLE entry_lines ( - id TEXT PRIMARY KEY, - journal_entry_id TEXT NOT NULL, - account_id TEXT NOT NULL, - debit INTEGER NOT NULL DEFAULT 0, - credit INTEGER NOT NULL DEFAULT 0, - description TEXT, - metadata TEXT DEFAULT '{{}}' - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_entry_lines_journal_entry ON entry_lines (journal_entry_id); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_entry_lines_account ON entry_lines (account_id); - """ - ) - - # Insert default chart of accounts - default_accounts = [ - # Assets - ("cash", "Cash", "asset", "Cash on hand"), - ("bank", "Bank Account", "asset", "Bank account"), - ("lightning", "Lightning Balance", "asset", "Lightning Network balance"), - ("accounts_receivable", "Accounts Receivable", "asset", "Money owed to the Castle"), - - # Liabilities - ("accounts_payable", "Accounts Payable", "liability", "Money owed by the Castle"), - - # Equity - ("member_equity", "Member Equity", "equity", "Member contributions"), - ("retained_earnings", "Retained Earnings", "equity", "Accumulated profits"), - - # Revenue - ("accommodation_revenue", "Accommodation Revenue", "revenue", "Revenue from stays"), - ("service_revenue", "Service Revenue", "revenue", "Revenue from services"), - ("other_revenue", "Other Revenue", "revenue", "Other revenue"), - - # Expenses - ("utilities", "Utilities", "expense", "Electricity, water, internet"), - ("food", "Food & Supplies", "expense", "Food and supplies"), - ("maintenance", "Maintenance", "expense", "Repairs and maintenance"), - ("other_expense", "Other Expenses", "expense", "Miscellaneous expenses"), - ] - - for acc_id, name, acc_type, desc in default_accounts: - await db.execute( - """ - INSERT INTO accounts (id, name, account_type, description) - VALUES (:id, :name, :type, :description) - """, - {"id": acc_id, "name": name, "type": acc_type, "description": desc} - ) - - -async def m002_extension_settings(db): - """ - Create extension_settings table for Castle configuration. - """ - await db.execute( - f""" - CREATE TABLE extension_settings ( - id TEXT NOT NULL PRIMARY KEY, - castle_wallet_id TEXT, - updated_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now} - ); - """ - ) - - -async def m003_user_wallet_settings(db): - """ - Create user_wallet_settings table for per-user wallet configuration. - """ - await db.execute( - f""" - CREATE TABLE user_wallet_settings ( - id TEXT NOT NULL PRIMARY KEY, - user_wallet_id TEXT, - updated_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now} - ); - """ - ) - - -async def m004_manual_payment_requests(db): - """ - Create manual_payment_requests table for user payment requests to Castle. - """ - await db.execute( - f""" - CREATE TABLE manual_payment_requests ( - id TEXT PRIMARY KEY, - user_id TEXT NOT NULL, - amount INTEGER NOT NULL, - description TEXT NOT NULL, - status TEXT NOT NULL DEFAULT 'pending', - created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}, - reviewed_at TIMESTAMP, - reviewed_by TEXT, - journal_entry_id TEXT - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_manual_payment_requests_user_id ON manual_payment_requests (user_id); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_manual_payment_requests_status ON manual_payment_requests (status); - """ - ) - - -async def m005_add_flag_and_meta(db): - """ - Add flag and meta columns to journal_entries table. - - flag: Transaction status (* = cleared, ! = pending, # = flagged, x = void) - - meta: JSON metadata for audit trail (source, tags, links, notes) - """ - await db.execute( - """ - ALTER TABLE journal_entries ADD COLUMN flag TEXT DEFAULT '*'; - """ - ) - - await db.execute( - """ - ALTER TABLE journal_entries ADD COLUMN meta TEXT DEFAULT '{}'; - """ - ) - - -async def m006_hierarchical_account_names(db): - """ - Migrate account names to hierarchical Beancount-style format. - - "Cash" → "Assets:Cash" - - "Accounts Receivable" → "Assets:Receivable" - - "Food & Supplies" → "Expenses:Food:Supplies" - - "Accounts Receivable - af983632" → "Assets:Receivable:User-af983632" - """ - from .account_utils import migrate_account_name - from .models import AccountType - - # Get all existing accounts - accounts = await db.fetchall("SELECT * FROM accounts") - - # Mapping of old names to new names - name_mappings = { - # Assets - "cash": "Assets:Cash", - "bank": "Assets:Bank", - "lightning": "Assets:Bitcoin:Lightning", - "accounts_receivable": "Assets:Receivable", - - # Liabilities - "accounts_payable": "Liabilities:Payable", - - # Equity - "member_equity": "Equity:MemberEquity", - "retained_earnings": "Equity:RetainedEarnings", - - # Revenue → Income - "accommodation_revenue": "Income:Accommodation", - "service_revenue": "Income:Service", - "other_revenue": "Income:Other", - - # Expenses - "utilities": "Expenses:Utilities", - "food": "Expenses:Food:Supplies", - "maintenance": "Expenses:Maintenance", - "other_expense": "Expenses:Other", - } - - # Update default accounts using ID-based mapping - for old_id, new_name in name_mappings.items(): - await db.execute( - """ - UPDATE accounts - SET name = :new_name - WHERE id = :old_id - """, - {"new_name": new_name, "old_id": old_id} - ) - - # Update user-specific accounts (those with user_id set) - user_accounts = await db.fetchall( - "SELECT * FROM accounts WHERE user_id IS NOT NULL" - ) - - for account in user_accounts: - # Parse account type - account_type = AccountType(account["account_type"]) - - # Migrate name - new_name = migrate_account_name(account["name"], account_type) - - await db.execute( - """ - UPDATE accounts - SET name = :new_name - WHERE id = :id - """, - {"new_name": new_name, "id": account["id"]} - ) - - -async def m007_balance_assertions(db): - """ - Create balance_assertions table for reconciliation. - Allows admins to assert expected balances at specific dates. - """ - await db.execute( - f""" - CREATE TABLE balance_assertions ( - id TEXT PRIMARY KEY, - date TIMESTAMP NOT NULL, - account_id TEXT NOT NULL, - expected_balance_sats INTEGER NOT NULL, - expected_balance_fiat TEXT, - fiat_currency TEXT, - tolerance_sats INTEGER DEFAULT 0, - tolerance_fiat TEXT DEFAULT '0', - checked_balance_sats INTEGER, - checked_balance_fiat TEXT, - difference_sats INTEGER, - difference_fiat TEXT, - status TEXT NOT NULL DEFAULT 'pending', - created_by TEXT NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}, - checked_at TIMESTAMP, - FOREIGN KEY (account_id) REFERENCES accounts (id) - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_balance_assertions_account_id ON balance_assertions (account_id); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_balance_assertions_status ON balance_assertions (status); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_balance_assertions_date ON balance_assertions (date); - """ - ) - - -async def m008_rename_lightning_account(db): - """ - Rename Lightning account from Assets:Lightning:Balance to Assets:Bitcoin:Lightning - for better naming consistency. - """ - await db.execute( - """ - UPDATE accounts - SET name = 'Assets:Bitcoin:Lightning' - WHERE name = 'Assets:Lightning:Balance' - """ - ) - - -async def m009_add_onchain_bitcoin_account(db): - """ - Add Assets:Bitcoin:OnChain account for on-chain Bitcoin transactions. - This allows tracking on-chain Bitcoin separately from Lightning Network payments. - """ - import uuid - - # Check if the account already exists - existing = await db.fetchone( - """ - SELECT id FROM accounts - WHERE name = 'Assets:Bitcoin:OnChain' - """ - ) - - if not existing: - # Create the on-chain Bitcoin asset account - await db.execute( - f""" - INSERT INTO accounts (id, name, account_type, description, created_at) - VALUES (:id, :name, :type, :description, {db.timestamp_now}) - """, - { - "id": str(uuid.uuid4()), - "name": "Assets:Bitcoin:OnChain", - "type": "asset", - "description": "On-chain Bitcoin wallet" - } - ) - - -async def m010_user_equity_status(db): - """ - Create user_equity_status table for managing equity contribution eligibility. - Only equity-eligible users can convert their expenses to equity contributions. - """ - await db.execute( - f""" - CREATE TABLE user_equity_status ( - user_id TEXT PRIMARY KEY, - is_equity_eligible BOOLEAN NOT NULL DEFAULT FALSE, - equity_account_name TEXT, - notes TEXT, - granted_by TEXT NOT NULL, - granted_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}, - revoked_at TIMESTAMP - ); - """ - ) - - await db.execute( - """ - CREATE INDEX idx_user_equity_status_eligible - ON user_equity_status (is_equity_eligible) - WHERE is_equity_eligible = TRUE; - """ - ) - - -async def m011_account_permissions(db): - """ - Create account_permissions table for granular account access control. - Allows admins to grant specific permissions (read, submit_expense, manage) to users for specific accounts. - Supports hierarchical permission inheritance (permissions on parent accounts cascade to children). - """ - await db.execute( - f""" - CREATE TABLE account_permissions ( - id TEXT PRIMARY KEY, - user_id TEXT NOT NULL, - account_id TEXT NOT NULL, - permission_type TEXT NOT NULL, - granted_by TEXT NOT NULL, - granted_at TIMESTAMP NOT NULL DEFAULT {db.timestamp_now}, - expires_at TIMESTAMP, - notes TEXT, - FOREIGN KEY (account_id) REFERENCES accounts (id) - ); - """ - ) - - # Index for looking up permissions by user - await db.execute( - """ - CREATE INDEX idx_account_permissions_user_id ON account_permissions (user_id); - """ - ) - - # Index for looking up permissions by account - await db.execute( - """ - CREATE INDEX idx_account_permissions_account_id ON account_permissions (account_id); - """ - ) - - # Composite index for checking specific user+account permissions - await db.execute( - """ - CREATE INDEX idx_account_permissions_user_account - ON account_permissions (user_id, account_id); - """ - ) - - # Index for finding permissions by type - await db.execute( - """ - CREATE INDEX idx_account_permissions_type ON account_permissions (permission_type); - """ - ) - - # Index for finding expired permissions - await db.execute( - """ - CREATE INDEX idx_account_permissions_expires - ON account_permissions (expires_at) - WHERE expires_at IS NOT NULL; - """ - ) - - -async def m012_update_default_accounts(db): - """ - Update default chart of accounts to include more detailed hierarchical structure. - Adds new accounts for fixed assets, livestock, equity contributions, and detailed expenses. - Only adds accounts that don't already exist. - """ - import uuid - from .account_utils import DEFAULT_HIERARCHICAL_ACCOUNTS - - for name, account_type, description in DEFAULT_HIERARCHICAL_ACCOUNTS: - # Check if account already exists - existing = await db.fetchone( - """ - SELECT id FROM accounts WHERE name = :name - """, - {"name": name} - ) - - if not existing: - # Create new account - await db.execute( - f""" - INSERT INTO accounts (id, name, account_type, description, created_at) - VALUES (:id, :name, :type, :description, {db.timestamp_now}) - """, - { - "id": str(uuid.uuid4()), - "name": name, - "type": account_type.value, - "description": description - } - ) - - -async def m013_remove_parent_only_accounts(db): - """ - Remove parent-only accounts from the database. - - Since Castle doesn't interface directly with Beancount (only exports to it), - we don't need parent accounts that exist only for organizational hierarchy. - The hierarchy is implicit in the colon-separated account names. - - When exporting to Beancount, the parent accounts will be inferred from the - hierarchical naming (e.g., "Assets:Bitcoin:Lightning" implies "Assets:Bitcoin" exists). - - This keeps our database clean and prevents accidentally posting to parent accounts. - - Removes: - - Assets:Bitcoin (parent of Lightning and OnChain) - - Equity (parent of user equity accounts like Equity:User-xxx) - """ - # Remove Assets:Bitcoin (parent account) - await db.execute( - "DELETE FROM accounts WHERE name = :name", - {"name": "Assets:Bitcoin"} - ) - - # Remove Equity (parent account) - await db.execute( - "DELETE FROM accounts WHERE name = :name", - {"name": "Equity"} - ) - - -async def m014_remove_legacy_equity_accounts(db): - """ - Remove legacy generic equity accounts that don't fit the user-specific equity model. - - The castle extension uses dynamic user-specific equity accounts (Equity:User-{user_id}) - created automatically when granting equity eligibility. Generic equity accounts like - MemberEquity and RetainedEarnings are not needed. - - Removes: - - Equity:MemberEquity - - Equity:RetainedEarnings - """ - # Remove Equity:MemberEquity - await db.execute( - "DELETE FROM accounts WHERE name = :name", - {"name": "Equity:MemberEquity"} - ) - - # Remove Equity:RetainedEarnings - await db.execute( - "DELETE FROM accounts WHERE name = :name", - {"name": "Equity:RetainedEarnings"} - ) - - -async def m015_convert_to_single_amount_field(db): - """ - Convert entry_lines from separate debit/credit columns to single amount field. - - This aligns Castle with Beancount's elegant design: - - Positive amount = debit (increase assets/expenses, decrease liabilities/equity/revenue) - - Negative amount = credit (decrease assets/expenses, increase liabilities/equity/revenue) - - Benefits: - - Simpler model (one field instead of two) - - Direct compatibility with Beancount import/export - - Eliminates invalid states (both debit and credit non-zero) - - More intuitive for programmers (positive/negative instead of accounting conventions) - - Migration formula: amount = debit - credit - - Examples: - - Expense transaction: - * Expenses:Food:Groceries amount=+100 (debit) - * Liabilities:Payable:User amount=-100 (credit) - - Payment transaction: - * Liabilities:Payable:User amount=+100 (debit) - * Assets:Bitcoin:Lightning amount=-100 (credit) - """ - from sqlalchemy.exc import OperationalError - - # Step 1: Add new amount column (nullable for migration) - try: - await db.execute( - "ALTER TABLE entry_lines ADD COLUMN amount INTEGER" - ) - except OperationalError: - # Column might already exist if migration was partially run - pass - - # Step 2: Populate amount from existing debit/credit - # Formula: amount = debit - credit - await db.execute( - """ - UPDATE entry_lines - SET amount = debit - credit - WHERE amount IS NULL - """ - ) - - # Step 3: Create new table with amount field as NOT NULL - # SQLite doesn't support ALTER COLUMN, so we need to recreate the table - await db.execute( - """ - CREATE TABLE entry_lines_new ( - id TEXT PRIMARY KEY, - journal_entry_id TEXT NOT NULL, - account_id TEXT NOT NULL, - amount INTEGER NOT NULL, - description TEXT, - metadata TEXT DEFAULT '{}' - ) - """ - ) - - # Step 4: Copy data from old table to new - await db.execute( - """ - INSERT INTO entry_lines_new (id, journal_entry_id, account_id, amount, description, metadata) - SELECT id, journal_entry_id, account_id, amount, description, metadata - FROM entry_lines - """ - ) - - # Step 5: Drop old table and rename new one - await db.execute("DROP TABLE entry_lines") - await db.execute("ALTER TABLE entry_lines_new RENAME TO entry_lines") - - # Step 6: Recreate indexes - await db.execute( - """ - CREATE INDEX idx_entry_lines_journal_entry ON entry_lines (journal_entry_id) - """ - ) - - await db.execute( - """ - CREATE INDEX idx_entry_lines_account ON entry_lines (account_id) - """ - ) - - -async def m016_drop_obsolete_journal_tables(db): - """ - Drop journal_entries and entry_lines tables. - - Castle now uses Fava/Beancount as the single source of truth for accounting data. - These tables are no longer written to or read from. - - All journal entry operations now: - - Write: Submit to Fava via FavaClient.add_entry() - - Read: Query Fava via FavaClient.get_entries() - - Migration completed as part of Castle extension cleanup (Nov 2025). - No backwards compatibility concerns - user explicitly approved. - """ - # Drop entry_lines first (has foreign key to journal_entries) - await db.execute("DROP TABLE IF EXISTS entry_lines") - - # Drop journal_entries - await db.execute("DROP TABLE IF EXISTS journal_entries") diff --git a/tasks.py b/tasks.py index 23dd0b5..0482708 100644 --- a/tasks.py +++ b/tasks.py @@ -58,15 +58,15 @@ async def check_all_balance_assertions() -> dict: }) except Exception as e: results["errors"] += 1 - print(f"Error checking assertion {assertion.id}: {e}") + logger.error(f"Error checking assertion {assertion.id}: {e}") # Log results if results["failed"] > 0: - print(f"[LIBRA] Daily reconciliation check: {results['failed']} FAILED assertions!") + logger.warning(f"[LIBRA] Daily reconciliation check: {results['failed']} FAILED assertions!") for failed in results["failed_assertions"]: - print(f" - Account {failed['account_id']}: expected {failed['expected_sats']}, got {failed['actual_sats']}") + logger.warning(f" - Account {failed['account_id']}: expected {failed['expected_sats']}, got {failed['actual_sats']}") else: - print(f"[LIBRA] Daily reconciliation check: All {results['passed']} assertions passed ✓") + logger.info(f"[LIBRA] Daily reconciliation check: All {results['passed']} assertions passed ✓") return results @@ -78,7 +78,7 @@ async def scheduled_daily_reconciliation(): This function is meant to be called by a scheduler (cron, systemd timer, etc.) or by LNbits background task system. """ - print(f"[LIBRA] Running scheduled daily reconciliation check at {datetime.now()}") + logger.info(f"[LIBRA] Running scheduled daily reconciliation check at {datetime.now()}") try: results = await check_all_balance_assertions() @@ -86,12 +86,12 @@ async def scheduled_daily_reconciliation(): # TODO: Send notifications if there are failures # This could send email, webhook, or in-app notification if results["failed"] > 0: - print(f"[LIBRA] WARNING: {results['failed']} balance assertions failed!") + logger.warning(f"[LIBRA] {results['failed']} balance assertions failed!") # Future: Send alert notification return results except Exception as e: - print(f"[LIBRA] Error in scheduled reconciliation: {e}") + logger.error(f"[LIBRA] Error in scheduled reconciliation: {e}") raise @@ -166,7 +166,7 @@ def start_daily_reconciliation_task(): # Run daily at 2 AM 0 2 * * * curl -X POST http://localhost:5000/libra/api/v1/tasks/daily-reconciliation -H "X-Api-Key: YOUR_ADMIN_KEY" """ - print("[LIBRA] Daily reconciliation task registered") + logger.info("[LIBRA] Daily reconciliation task registered") # In a production system, you would register this with LNbits task scheduler # For now, it can be triggered manually via API endpoint diff --git a/tests/conftest.py b/tests/conftest.py index 44b5c26..7b4bfe7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -108,9 +108,6 @@ def _settings_cleanup(settings: Settings) -> None: settings.lnbits_user_activation_by_invitation_code = False settings.lnbits_register_reusable_activation_code = "" settings.lnbits_register_one_time_activation_codes = [] - # Keep the rate limiter disabled across per-test settings resets (the - # limiter itself is fixed at app-creation time, but keep the value coherent). - settings.lnbits_rate_limit_no = 1_000_000 @pytest.fixture(scope="session") diff --git a/tests/test_unit.py b/tests/test_unit.py index 11779e9..a25b436 100644 --- a/tests/test_unit.py +++ b/tests/test_unit.py @@ -394,66 +394,6 @@ def test_migrate_account_name_expense_with_ampersand(): ) -# --------------------------------------------------------------------------- -# core.validation — validate_journal_entry -# --------------------------------------------------------------------------- - - -def test_validate_journal_entry_balanced_passes(): - val.validate_journal_entry( - {"id": "x"}, - [ - {"account_id": "a", "amount": 100}, - {"account_id": "b", "amount": -100}, - ], - ) - - -def test_validate_journal_entry_unbalanced_raises(): - with pytest.raises(val.ValidationError) as exc: - val.validate_journal_entry( - {"id": "x"}, - [ - {"account_id": "a", "amount": 100}, - {"account_id": "b", "amount": -50}, - ], - ) - assert "not balanced" in str(exc.value) - - -def test_validate_journal_entry_single_line_raises(): - with pytest.raises(val.ValidationError) as exc: - val.validate_journal_entry( - {"id": "x"}, - [{"account_id": "a", "amount": 100}], - ) - assert "at least 2 lines" in str(exc.value) - - -def test_validate_journal_entry_zero_amount_raises(): - with pytest.raises(val.ValidationError) as exc: - val.validate_journal_entry( - {"id": "x"}, - [ - {"account_id": "a", "amount": 0}, - {"account_id": "b", "amount": 0}, - ], - ) - assert "amount = 0" in str(exc.value) - - -def test_validate_journal_entry_missing_account_id_raises(): - with pytest.raises(val.ValidationError) as exc: - val.validate_journal_entry( - {"id": "x"}, - [ - {"amount": 100}, - {"account_id": "b", "amount": -100}, - ], - ) - assert "missing account_id" in str(exc.value) - - # --------------------------------------------------------------------------- # core.validation — validate_balance # --------------------------------------------------------------------------- diff --git a/user_lookup.py b/user_lookup.py new file mode 100644 index 0000000..040c09b --- /dev/null +++ b/user_lookup.py @@ -0,0 +1,126 @@ +"""Username resolution for UI display. + +Extracted from views_api (CODE-REVIEW-2026-06 #18): the old helper +constructed a fresh LNbits `Database` per call inside per-row hot paths +(entry listings, all-user balances). This module keeps one shared core-DB +handle and a short TTL cache, so listing N rows for the same few users +costs one lookup per unique user per TTL window instead of one per row. + +Accepted id shapes (they all occur in ledger data): +- Full UUID with dashes (36 chars): "375ec158-686c-4a21-b44d-a51cc90ef07d" +- Dashless UUID (32 chars): "375ec158686c4a21b44da51cc90ef07d" +- Partial id from account names (8 chars): "375ec158" +""" + +from typing import Dict, Iterable, Optional + +from lnbits.core.crud.users import get_user +from lnbits.db import Database +from lnbits.utils.cache import Cache +from loguru import logger + +# One shared handle to the LNbits core DB (username lives on core +# `accounts`, not in libra's extension DB). +_core_db = Database("database") + +_username_cache = Cache() +_USERNAME_CACHE_TTL = 60 # seconds — usernames change rarely + + +def _dashed(user_id: str) -> str: + return ( + f"{user_id[0:8]}-{user_id[8:12]}-{user_id[12:16]}" + f"-{user_id[16:20]}-{user_id[20:32]}" + ) + + +async def _resolve(user_id: str) -> str: + from .crud import get_all_user_wallet_settings + + # Case 1: full UUID with dashes + if len(user_id) == 36 and user_id.count('-') == 4: + user = await get_user(user_id) + return user.username if user and user.username else f"User-{user_id[:8]}" + + # Case 2: dashless 32-char UUID — libra user settings first, then + # the LNbits core DB directly + if len(user_id) == 32 and '-' not in user_id: + try: + user_id_with_dashes = _dashed(user_id) + + user_settings = await get_all_user_wallet_settings() + for setting in user_settings: + if setting.id == user_id_with_dashes: + user = await get_user(setting.id) + return ( + user.username + if user and user.username + else f"User-{user_id[:8]}" + ) + + async with _core_db.connect() as conn: + row = await conn.fetchone( + "SELECT id, username FROM accounts WHERE id = :user_id LIMIT 1", + {"user_id": user_id_with_dashes}, + ) + if row and row["username"]: + return row["username"] + + return f"User-{user_id[:8]}" + except Exception as e: + logger.error(f"Error looking up user by dashless UUID {user_id}: {e}") + return f"User-{user_id[:8]}" + + # Case 3: 8-char partial id from an account name — resolve to a full + # id via libra user settings + if len(user_id) == 8: + try: + user_settings = await get_all_user_wallet_settings() + for setting in user_settings: + if setting.id.startswith(user_id): + user = await get_user(setting.id) + return ( + user.username + if user and user.username + else f"User-{user_id}" + ) + return f"User-{user_id}" + except Exception as e: + logger.error(f"Error looking up user by partial ID {user_id}: {e}") + return f"User-{user_id}" + + # Case 4: unknown shape — try as-is, fall back + try: + user = await get_user(user_id) + return user.username if user and user.username else f"User-{user_id[:8]}" + except Exception: + return f"User-{user_id[:8]}" + + +async def get_username(user_id: str) -> Optional[str]: + """Resolve a user id (any accepted shape) to a display username. + + Returns a "User-{short}" fallback when no username exists, or None + for falsy input. + """ + if not user_id: + return None + + cache_key = f"username:{user_id}" + cached = _username_cache.get(cache_key) + if cached is not None: + return cached + + result = await _resolve(user_id) + _username_cache.set(cache_key, result, _USERNAME_CACHE_TTL) + return result + + +async def get_usernames(user_ids: Iterable[str]) -> Dict[str, str]: + """Resolve many user ids at once, deduplicated and cache-backed.""" + result: Dict[str, str] = {} + for user_id in {u for u in user_ids if u}: + username = await get_username(user_id) + if username is not None: + result[user_id] = username + return result diff --git a/views_api.py b/views_api.py index b41e6b6..65185e8 100644 --- a/views_api.py +++ b/views_api.py @@ -15,6 +15,7 @@ from lnbits.utils.exchange_rates import allowed_currencies, fiat_amount_as_satos from .account_utils import VALID_ACCOUNT_PREFIXES, validate_account_name from .beancount_format import fiat_rate_metadata +from .user_lookup import get_username from .crud import ( approve_manual_payment_request, check_balance_assertion, @@ -643,7 +644,7 @@ async def api_get_user_entries( break # Look up actual username using helper function - username = await _get_username_from_user_id(user_id_match) if user_id_match else None + username = await get_username(user_id_match) if user_id_match else None entry_data = { "id": entry_id or e.get("entry_hash", "unknown"), @@ -684,119 +685,6 @@ async def api_get_user_entries( } -async def _get_username_from_user_id(user_id: str) -> str: - """ - Helper function to get username from user_id, handling various formats. - - Supports: - - Full UUID with dashes (36 chars): "375ec158-686c-4a21-b44d-a51cc90ef07d" - - Dashless UUID (32 chars): "375ec158686c4a21b44da51cc90ef07d" - - Partial ID (8 chars from account names): "375ec158" - - Returns username or formatted fallback. - """ - from lnbits.core.crud.users import get_user - - logger.debug(f"[USERNAME] Called with: '{user_id}' (len={len(user_id) if user_id else 0})") - - if not user_id: - return None - - # Case 1: Already in standard UUID format (36 chars with dashes) - if len(user_id) == 36 and user_id.count('-') == 4: - logger.debug(f"[USERNAME] Case 1: Full UUID format") - user = await get_user(user_id) - result = user.username if user and user.username else f"User-{user_id[:8]}" - logger.debug(f"[USERNAME] Case 1 result: '{result}'") - return result - - # Case 2: Dashless 32-char UUID - lookup via Libra user settings, fallback to LNbits - elif len(user_id) == 32 and '-' not in user_id: - logger.debug(f"[USERNAME] Case 2: Dashless UUID format - looking up in Libra user settings") - try: - # Convert dashless to dashed format - user_id_with_dashes = f"{user_id[0:8]}-{user_id[8:12]}-{user_id[12:16]}-{user_id[16:20]}-{user_id[20:32]}" - logger.debug(f"[USERNAME] Converted to dashed format: {user_id_with_dashes}") - - # Try Libra settings first - user_settings = await get_all_user_wallet_settings() - for setting in user_settings: - if setting.id == user_id_with_dashes: - logger.debug(f"[USERNAME] Found matching user in Libra settings") - user = await get_user(setting.id) - result = user.username if user and user.username else f"User-{user_id[:8]}" - logger.debug(f"[USERNAME] Case 2 result (from Libra): '{result}'") - return result - - # Not in Libra settings - try LNbits database directly - logger.debug(f"[USERNAME] Not in Libra settings, querying LNbits database directly") - from lnbits.db import Database - db = Database("database") - async with db.connect() as conn: - row = await conn.fetchone( - "SELECT id, username FROM accounts WHERE id = :user_id LIMIT 1", - {"user_id": user_id_with_dashes} - ) - logger.debug(f"[USERNAME] Database query result: {row}") - if row and row["username"]: - result = row["username"] - logger.debug(f"[USERNAME] Case 2 result (from LNbits DB): '{result}'") - return result - - # User doesn't exist anywhere - logger.debug(f"[USERNAME] User not found in LNbits database either") - result = f"User-{user_id[:8]}" - logger.debug(f"[USERNAME] Case 2 result (not found): '{result}'") - return result - - except Exception as e: - logger.error(f"Error looking up user by dashless UUID {user_id}: {e}") - result = f"User-{user_id[:8]}" - return result - - # Case 3: Partial ID (8 chars from account name) - lookup via Libra user settings - elif len(user_id) == 8: - logger.debug(f"[USERNAME] Case 3: Partial ID format - looking up in Libra user settings") - try: - # Get all Libra users (which have full user_ids) - user_settings = await get_all_user_wallet_settings() - - # Find matching user by first 8 chars - for setting in user_settings: - if setting.id.startswith(user_id): - logger.debug(f"[USERNAME] Found full user_id: {setting.id}") - # Now get username from LNbits with full ID - user = await get_user(setting.id) - result = user.username if user and user.username else f"User-{user_id}" - logger.debug(f"[USERNAME] Case 3 result (found): '{result}'") - return result - - # No matching user found in Libra settings - logger.debug(f"[USERNAME] No matching user found in Libra settings") - result = f"User-{user_id}" - logger.debug(f"[USERNAME] Case 3 result (not found): '{result}'") - return result - - except Exception as e: - logger.error(f"Error looking up user by partial ID {user_id}: {e}") - result = f"User-{user_id}" - return result - - # Case 4: Unknown format - try as-is and fall back - else: - logger.debug(f"[USERNAME] Case 4: Unknown format - trying as-is") - try: - user = await get_user(user_id) - result = user.username if user and user.username else f"User-{user_id[:8]}" - logger.debug(f"[USERNAME] Case 4 result: '{result}'") - return result - except Exception as e: - logger.debug(f"[USERNAME] Case 4 exception: {e}") - result = f"User-{user_id[:8]}" - logger.debug(f"[USERNAME] Case 4 fallback result: '{result}'") - return result - - @libra_api_router.get("/api/v1/entries/pending") async def api_get_pending_entries( auth: AuthContext = Depends(require_super_user), @@ -844,7 +732,7 @@ async def api_get_pending_entries( break # Look up username using helper function - username = await _get_username_from_user_id(user_id) if user_id else None + username = await get_username(user_id) if user_id else None # Extract amount from postings (sum of absolute values / 2) amount_sats = 0 @@ -1453,7 +1341,9 @@ async def api_create_receivable_entry( created_by=auth.user_id, created_at=datetime.now(), reference=data.reference, - flag=JournalEntryFlag.PENDING, + # Receivables are written cleared (format_receivable_entry uses + # flag="*") — reporting PENDING here misled the UI (libra-#35). + flag=JournalEntryFlag.CLEARED, meta=entry_meta, lines=[ EntryLine( @@ -1682,7 +1572,7 @@ async def api_get_all_balances( # Enrich with username information using helper function result = [] for balance in balances: - username = await _get_username_from_user_id(balance["user_id"]) + username = await get_username(balance["user_id"]) result.append({ "user_id": balance["user_id"],