Skip to content

Add portfolio allocation display and P&L tracking enhancements - #7

Open
IamJasonBian wants to merge 2 commits into
mainfrom
fix-btc-order-audit
Open

Add portfolio allocation display and P&L tracking enhancements#7
IamJasonBian wants to merge 2 commits into
mainfrom
fix-btc-order-audit

Conversation

@IamJasonBian

@IamJasonBian IamJasonBian commented Feb 8, 2026

Copy link
Copy Markdown
Owner

PR to add P&L stuff

IamJasonBian and others added 2 commits February 6, 2026 13:42
- Remove broken portfolio history API (404 error fix)
- Add dollar cost average (DCA) and P/L to position details in audit
- Add portfolio coverage percentage to order book display
- Enhance logging for portfolio history API calls with detailed debug info
- All tests passing (6/6)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Copy link
Copy Markdown
Owner Author

Code Review — PR #7

Overall: Adds P&L tracking and proportional coverage calculations to the audit system. The proportional equity coverage fix (from binary "has coverage" to actual coverage percentage) is a meaningful accuracy improvement.

Concerns

  1. Very stale — This PR has been open since Feb 8 (~7 weeks). The base SHA is significantly behind main. Likely needs a rebase and re-validation of tests.

  2. get_portfolio_history added to safe_cash_bot.py — This method imports logging at function scope and adds portfolio history fetching. It's unrelated to the audit/P&L coverage changes in the PR title. Consider splitting.

  3. Test coverage is good — The new test_proportional_coverage_btc_scenario with concrete numbers (3562 shares @ $31.54) validates the proportional math well. The existing test update (24000.0 vs 30000.0) correctly reflects the proportional calculation.

Recommendation

Given the age, consider rebasing and re-running tests. If too painful, it may be easier to cherry-pick the audit.py proportional coverage changes onto a fresh branch from current main.


Generated by Claude Code

@IamJasonBian IamJasonBian added the stale label Mar 27, 2026 — with Claude

Copy link
Copy Markdown
Owner Author

Stale PR notice — This PR has been open for 63 days (~9 weeks) with no new commits since Feb 8. The prior review noted significant base SHA drift and recommended cherry-picking the proportional-coverage audit changes onto a fresh branch rather than attempting a rebase. Please either rebase+revalidate or close and open a fresh PR for the audit.py changes.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Stale — 11 weeks, no activity since Feb 8. Two prior reviews recommended cherry-picking the proportional-coverage audit changes onto a fresh branch from current main. The base SHA drift is now extreme. Recommend closing this PR and opening a new one if the audit.py changes are still wanted.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Code Deletion & Redundancy — PR #7

This PR is now 16 weeks old with extreme base SHA drift. Here's what can be deleted to make it mergeable (or help decide to close it).

1. Delete get_portfolio_history() from this PR (51 lines → 0)

This method (safe_cash_bot.py:510-557) imports logging at function scope, fetches Robinhood historical portfolio data, and has nothing to do with the proportional coverage or P&L changes in the PR title. It was flagged in the initial review as unrelated scope. Remove it — if needed, submit as a standalone PR.

2. Consolidate duplicate order-display logic in get_portfolio_summary (~15 lines saved)

The buy-order and sell-order display blocks both compute order_value and coverage_pct, then print nearly identical formatted output. The sell path adds a position lookup, but the structure is the same. Extract a helper:

def _format_order_value(order, equity, positions=None):
    if order['side'] == 'buy':
        price = order['limit_price'] or 0
    else:
        pos = next((p for p in (positions or []) if p['symbol'] == order['symbol']), None)
        price = pos['current_price'] if pos else 0
    order_value = order['quantity'] * price
    coverage_pct = (order_value / equity * 100) if equity > 0 else 0
    return order_value, coverage_pct

This eliminates the duplicated computation and the 10-line if position: ... else: block in the sell path.

3. Recommendation: Cherry-pick the audit.py changes, close this PR

The valuable changes are ~50 lines in audit.py (proportional coverage math) and test_audit.py (new BTC scenario test). The unrelated additions (get_portfolio_history, order value display in get_portfolio_summary) inflate the diff and block review. Cherry-pick audit.py + test_audit.py onto a fresh branch from current main and close this.


Generated by Claude Code

@IamJasonBian IamJasonBian left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Stale — oldest open PR (5+ months). Consider closing.

Review notes if this is still relevant:

  1. get_portfolio_history creates a logging.getLogger(__name__) on every call. Move it to a module-level constant.
  2. Proportional coverage fix in audit.py is a genuine improvement — the old code counted full equity for any partial coverage, now it's proportional. This logic is worth extracting into its own focused PR.
  3. P&L fields added to check_coverage (profit_loss, profit_loss_pct, avg_buy_price) — useful, but they expand the audit's responsibility. Consider whether P&L belongs in the coverage audit or in a separate summary method.
  4. Order value / coverage % in get_portfolio_summary adds useful output but is print-only — not returned as data. If downstream consumers need this, it should be in the return value too.

Recommendation: The proportional coverage fix is the valuable core of this PR. The rest (P&L tracking, portfolio history, order value display) could be separate PRs. Given the age and likely merge conflicts with main, consider rebasing the coverage fix alone into a fresh branch and closing this.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant