Skip to content

Add paper trade approval gate#131

Merged
sjackson0109 merged 3 commits into
sjackson0109:mainfrom
rpelevin:feature/trade-proposal-approval-gate
Jun 10, 2026
Merged

Add paper trade approval gate#131
sjackson0109 merged 3 commits into
sjackson0109:mainfrom
rpelevin:feature/trade-proposal-approval-gate

Conversation

@rpelevin

@rpelevin rpelevin commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add a small in-memory approval gate for paper-trade proposals
  • bind approval to a canonical digest of the exact execution payload
  • add lifecycle audit entries and fail-closed checks before calling paper-mode execution

Scope

This first slice is intentionally limited to the approval / risk / audit path for paper trading.

It does not touch live exchange execution, credentials, MCP transport, GUI, or the broader order execution engine.

Tests

  • PYTHONDONTWRITEBYTECODE=1 python3 -m unittest test_trade_proposal_approval.py -v
  • PYTHONDONTWRITEBYTECODE=1 python3 -m unittest test_paper_trading_integration.py -v
  • PYTHONDONTWRITEBYTECODE=1 python3 -m unittest test_paper_mode.py -v
  • git diff --check

Strongly linked to #102

@rpelevin rpelevin requested a review from sjackson0109 as a code owner June 10, 2026 11:07
@rpelevin

Copy link
Copy Markdown
Collaborator Author

Thanks Simon — PR is up against main here.

It looks like GitHub has the forked-PR workflows waiting in action_required / maintainer-approval state:

  • Code Quality & Testing
  • Project Management
  • PowerTrader AI+ CI/CD

No rush at all. Once those are approved/run, I’ll watch for any failures and keep any fix scoped to the paper-mode approval/risk/audit path in this PR.

@rpelevin

Copy link
Copy Markdown
Collaborator Author

Thanks Simon — reviewed both.

The blocking issue is just Black formatting on the two approval-gate files:

  • app/trade_proposal_approval.py
  • app/test_trade_proposal_approval.py

The Python 3.11/3.12 unit tests and security scan passed. Notify Completion is only failing because it detects the upstream code-quality failure.

I pushed a formatting-only follow-up commit to the PR branch and will watch the rerun.

Copy link
Copy Markdown
Collaborator Author

Quick update after the formatting-only follow-up commit: the new head cb53137 is back at GitHub's forked-PR workflow approval gate.

The two fresh runs waiting in action_required are:

  • Code Quality & Testing
  • PowerTrader AI+ CI/CD

No code action needed from me until those are approved/run. I'll keep watching and only touch this PR again if the rerun fails or you/reviewer asks for a change.

@rpelevin

Copy link
Copy Markdown
Collaborator Author

Rerun is green now.

The formatting-only commit cb53137 cleared the Black/code-quality failure, and the fresh checks passed across tests, build/integration, code quality, security, and notify completion.

Deployment Readiness is skipped by the workflow, not failing.

No further code action from me unless you or a reviewer wants a change.

@sjackson0109

Copy link
Copy Markdown
Owner

@rpelevin here is my review:

Main issue:
PR #131 only adds an isolated in-memory approval gate and tests. It does not wire the approval gate into the existing MCP, GUI, risk, audit, or execution paths. Issue #102 requires Propose + Confirm in the dashboard, Tier 3 execution controls, RiskLimits validation, audit logging, circuit breaker integration, and persisted approval mode. Those criteria remain open/unresolved.

The code itself looks reasonable for a first approval-gate component. It deep-copies the payload, computes a canonical digest, blocks changed payloads, blocks unapproved execution, and records audit events.

Specific concerns:

  1. It is not integrated. Nothing in the existing execution path appears to call this gate. Direct calls to PaperTradingAccount.place_order(...) still bypass it. How is this called exactly?
  2. It is not concurrency-safe. Two concurrent execution calls could both pass assert_executable() before either marks the proposal as executed. That could duplicate paper orders. This is probably me being a little pedantic; but it is a very real possibility.
  3. It uses naive datetime.now(). If expires_at is timezone-aware, comparison may fail. How about datetime.utcnow() ?
  4. The audit log is in-memory only. That may be acceptable for this slice, but it does not satisfy issue [FEATURE] MCP Tool Integration - Agentic Trading Mode #102’s immutable audit requirement. Compliance and or regulatory standards would mandate this immutable auditing.
  5. The checks are green on commit cb53137. Code Quality & Testing and PowerTrader AI+ CI/CD both completed successfully after a second commit. Not a bad effort all round.

@rpelevin

Copy link
Copy Markdown
Collaborator Author

Thanks Simon, that is fair.

I pushed follow-up commit acbb7ce that keeps this PR as a narrow first slice, but addresses the review points directly:

  • makes the approval gate thread-safe around executable assertion + paper order placement, so two concurrent execution attempts cannot both place the same approved proposal;
  • normalizes proposal, approval, audit, and expiry times to UTC-aware datetimes while still accepting naive datetimes as UTC for compatibility;
  • adds ApprovalGatedPaperTradingAccount, a small paper-mode adapter that proposes through the gate, runs the existing PaperTradingAccount.risk_manager.validate_trade(...) check, then executes the exact approved proposal through PaperTradingAccount.place_order(...);
  • optionally routes approved execution through an existing circuit breaker object;
  • adds an optional audit sink so lifecycle entries can be forwarded into a durable audit surface later without pretending this slice completes immutable audit storage;
  • adds regression tests for concurrent duplicate execution, timezone-aware expiry, unapproved adapter execution, approved adapter execution, circuit-breaker routing, and audit sink capture.

Local checks passed before push:

  • python3 -m unittest test_trade_proposal_approval.py
  • python3 -m unittest test_circuit_breaker.py test_paper_mode.py test_trade_proposal_approval.py
  • black --check on the two touched files
  • fatal flake8 checks on the two touched files
  • git diff --check

This still does not claim to close all of #102. The MCP server, dashboard Propose + Confirm UI, persisted approval mode, and compliance-grade immutable audit storage remain broader issue scope.

@sjackson0109 sjackson0109 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good effort for your first contribution to this project. Thank you for your efforts!

@sjackson0109 sjackson0109 merged commit 858a0eb into sjackson0109:main Jun 10, 2026
11 checks passed
@rpelevin

Copy link
Copy Markdown
Collaborator Author

Thanks Simon, appreciate the review and merge.

I’ll keep this as a merged first slice only: paper-mode proposal approval, digest-bound execution, existing risk validation, circuit-breaker routing, and audit handoff.

I’ll treat the remaining Issue 102 work as separate scope: MCP wiring, dashboard Propose + Confirm, persisted approval mode, and immutable audit storage.

No further action from me unless you want a follow-up slice in one of those areas.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants