From ec6cac51f06fcee62b01081e3771145ef813cb89 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 12 Jul 2026 16:01:04 +0200 Subject: [PATCH] =?UTF-8?q?chore:=20hygiene=20sweep=20=E2=80=94=20dead=20c?= =?UTF-8?q?ode,=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:

- -

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

- -
-

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"], -- 2.53.0