Skip to content

Feat/code improvements#178

Merged
cardosofede merged 59 commits into
mainfrom
feat/code_improvements
Jun 19, 2026
Merged

Feat/code improvements#178
cardosofede merged 59 commits into
mainfrom
feat/code_improvements

Conversation

@cardosofede

Copy link
Copy Markdown
Contributor

What changed

Security

  • Default creds warning + debug bypass gated: app warns loudly when running with admin:admin; the debug_mode auth bypass now only works in non-production environments.
  • CORS: configurable allowed origins instead of * + credentials.
  • Path traversal hardening: account/connector names and archived-bot db_path are validated to stay inside their expected directories.

Correctness

  • Per-instance (not shared) _last_known_prices.
  • Accounts state is snapshotted before iterating (avoids mutation-during-iteration).
  • MQTT teardown is awaited on orchestrator stop; recorder event tasks keep strong refs (no GC mid-flight).

Performance

  • Account snapshots committed once per dump (not per row).
  • Multi-account portfolio history in a single query; one DB session per connector in order sync; batched controller perf snapshots; demoted hot-path logging to debug.

Refactors (behavior-preserving)

  • AccountsService god-class split apart → new gateway_wallet_service, perpetual_trading_service, portfolio_analytics_service (~1100 lines removed from the original).
  • trading.py router and create_executor slimmed; shared pagination/cursor helpers extracted.

What to test for QA

  1. Auth: login with real creds works; startup logs the default-credentials warning; debug bypass does NOT work when env=production.
  2. CORS: configured origins accepted, others rejected; credentialed requests still work.
  3. Path traversal: account/connector names and archived-bot endpoints reject ../ / absolute paths; valid names still work.
  4. Accounts/portfolio: account state dump, balances, and multi-account portfolio history return correct data (these touched the biggest refactor — AccountsService split).
    Performance
  • Account snapshots committed once per dump (not per row).
  • Multi-account portfolio history in a single query; one DB session per connector in order sync; batched controller perf snapshots; demoted hot-path logging to debug.

Refactors (behavior-preserving)

  • AccountsService god-class split apart → new gateway_wallet_service, perpetual_trading_service, portfolio_analytics_service (~1100 lines removed from the original).
  • trading.py router and create_executor slimmed; shared pagination/cursor helpers extracted.

What to test for QA

  1. Auth: login with real creds works; startup logs the default-credentials warning; debug bypass does NOT work when env=production.
  2. CORS: configured origins accepted, others rejected; credentialed requests still work.
  3. Path traversal: account/connector names and archived-bot endpoints reject ../ / absolute paths; valid names still work.
  4. Accounts/portfolio: account state dump, balances, and multi-account portfolio history return correct data (these touched the biggest refactor — AccountsService split).
  5. Gateway / perpetual / DEX wallet flows — they now live in new service modules; smoke-test the endpoints that use them.
  6. Executors & trading: create/search/stop executors and the trading router endpoints behave identically.
  7. Bots lifecycle: start/stop a bot, confirm MQTT shuts down cleanly with no dangling tasks.
  8. Archived bots: list/read archived bots still works.

cardosofede and others added 30 commits June 12, 2026 15:44
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace N sequential per-account DB queries (plus Python re-sort and
shared-cursor mis-pagination) in /portfolio/history with one SQL query:
AccountRepository.get_account_state_history accepts an account_names IN
filter, AccountsService.load_account_state_history passes it through, and
the router paginates one merged timestamp-desc stream. Response shape is
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s_service

Delete the dead duplicated AccountTradingInterface class, the unused
get_trading_interface factory, the _trading_interfaces dict and its
cleanup in stop(), plus now-unused imports. The live implementation in
services/trading_service.py remains the single source of truth (used by
executor_service).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add paginate_by_cursor(items, cursor, limit, sort_key, reverse) to
models/pagination.py and replace the five copy-pasted in-memory
pagination blocks in routers/trading.py (positions, active orders,
orders, trades, funding payments). Responses are byte-identical
(verified by an equivalence walk over multi-page datasets).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Break the ~115-line create_executor into _validate_executor_config
(pure validation + typed-config build), _prepare_market (connector and
market readiness) and _instantiate_and_register. Public signature,
status codes and payloads unchanged; invalid configs now fail fast
before connector initialization.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OrdersRecorder and FundingRecorder spawned fire-and-forget
asyncio.create_task calls that could be garbage-collected, silently
dropping order/trade/funding DB writes. Track tasks in a _pending_tasks
set via _create_tracked_task (add + add_done_callback(discard)) and
drain in-flight tasks in stop() after removing listeners.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make stop() async: cancel and await the update/performance tasks and
await mqtt_manager.stop() directly instead of spawning an unretained
fire-and-forget task that could run with no event loop. The lifespan
shutdown in main.py now awaits bots_orchestrator.stop().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_sync_orders_to_database opened a session and ran a redundant SELECT
per in-flight order every minute. Hoist the session out of the loop
(one get_session_context per connector, single commit) and mutate the
already-fetched ORM row directly. OrderRepository.update_order_status
now reuses get_order_by_client_id for remaining callers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
db_path was passed unvalidated to sqlite, allowing arbitrary file reads.
Add FileSystemUtil.get_archived_db_path (realpath + commonpath
containment under bots/archived); all 8 read endpoints validate via
_validate_db_path (400 on escape, 404 on missing) before opening, and
delete_archived_bot reuses the same check. HummingbotDatabase now
rejects non-existent files.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ARCH-010, ARCH-013, ARCH-014, CORR-006, CORR-009, PERF-002, PERF-004,
SEC-016, READ-024, READ-025 -> done/ with their commits annotated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delete place_order, cancel_order, get_active_orders, get_positions and
set_leverage from TradingService — zero callers; the live versions are
in AccountsService and wired to routers/trading.py. TradingService now
only owns executor-facing trading interfaces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add SecuritySettings.insecure_defaults_in_use() and
warn_if_insecure_security_defaults(); the lifespan logs a CRITICAL
security warning at startup naming the env vars (USERNAME, PASSWORD,
CONFIG_PASSWORD) still at their insecure defaults (admin/admin/'a').
Defaults stay usable for local development; production requirements
are documented in SecuritySettings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add ControllerPerformanceRepository.save_controller_performances using
a single add_all + flush; dump_controller_performance accumulates all
snapshots across bots/controllers and writes them in one bulk call.
saved_count still reflects rows written.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-event INFO logs in _did_create_order/_handle_order_created become
debug, and the per-listener introspection in start() only runs when
DEBUG is enabled. Error/warning logging and the one-time start/stop
summaries are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ARCH-011, SEC-018, PERF-003, PERF-005 -> done/ with commits annotated.
SEC-018 closed via the warning path (defaults remain usable for local
dev); criteria 2/3 deliberately partial, noted in the doc.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
account_name and connector_name reached filesystem operations
unvalidated, allowing creation/deletion of arbitrary directories. Add
validate_safe_name (alnum/_/- only, HTTP 400 otherwise) enforced in the
accounts router and in add/delete_account and credential methods;
fs_util.delete_folder and list_files now defensively reject separators
and '..' components.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
allow_origins='*' combined with allow_credentials=True made Starlette
reflect any Origin. CORS is now driven by CORSSettings (CORS_ env
prefix): default is an explicit empty list plus a localhost-only
origin regex (local dev unchanged); production origins via
CORS_ALLOW_ORIGINS / CORS_ALLOW_ORIGIN_REGEX. Adds
test/test_cors_settings.py.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ents

DEBUG_MODE disabled HTTP Basic and WebSocket auth unconditionally. The
bypass now only applies when LOGFIRE_ENVIRONMENT is non-production
(Settings.auth_disabled_by_debug_mode); in prod it is refused with a
CRITICAL log, and whenever it is active startup logs a loud CRITICAL
warning (warn_if_debug_mode_enabled, alongside the SEC-018 check).
Adds test/test_debug_mode_settings.py (9 tests).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
READ-021 closed as subsumed by ARCH-010 (dead class removal already
deleted _wait_for_order_book_ready).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_safe_get_last_traded_prices re-implemented the _last_known_prices
fallback inline; it now computes the missing pairs and delegates to the
single _get_fallback_prices implementation. Behavior unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The price cache was a class-level mutable dict shared by every
AccountsService instance; it is now initialized in __init__ so caches
no longer leak across instances.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract the duplicated token-balance dict literal into
AccountsService._balance_entry (same float/value conventions) and the
four copy-pasted gateway ping-or-503 guards into _require_gateway().
Balance JSON shape and 503 behavior unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dump_account_state awaited DB writes while iterating the live
accounts_state dict, racing concurrent balance updates and account
deletions (RuntimeError: dictionary changed size during iteration).
Take a synchronous shallow snapshot before the loop; the same pattern
is applied to get_portfolio_distribution and get_account_distribution.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace 14 annotations using the builtin any() function as a type
(Dict[str, any], value: any, instance attrs) with typing.Any across
accounts_service, gateway_service, gateway_client and
unified_connector_service.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AccountRepository.save_account_state committed per connector,
multiplying transaction round-trips in the periodic dump. It now only
flushes (to obtain the AccountState id); the single get_session_context
in dump_account_state owns the transaction and commits once per
snapshot, making the whole snapshot atomic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
READ-022, CORR-008, ARCH-015, CORR-007, READ-023, PERF-001 -> done/.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract three collaborating services composed by AccountsService:
PortfolioAnalyticsService (pure distribution math, no IO imports, 16
unit tests), GatewayWalletService (wallet CRUD, gateway balances and
immediate pricing, owns the require-gateway guard and balance_entry
helper) and PerpetualTradingService (leverage, position mode and
positions via an injected connector provider). AccountsService keeps
balance polling, state coordination and DB persistence and exposes
thin delegators with identical signatures, so routers, deps.py,
main.py and executor_service are unchanged. accounts_service.py goes
from 1887 to 1344 lines. Distribution outputs verified byte-identical
against the previous implementation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cardosofede and others added 19 commits June 16, 2026 22:35
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ository

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ector

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ator

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ervice private call

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/restore)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d BotsOrchestrator

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sService

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rapcmia

rapcmia commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Commit Commit 60437ca

  • Test PR using make setup; make deploy

Auth: login with real creds works; startup logs the default-credentials warning; debug bypass does NOT work when env=production.

  • Test route with no auth, payload response not authenticated ok
  • Test route with incorrect auth, payload response Incorrect username or password ok
  • Test route with correct auth, returns needed payload response ok
  • Test DEBUG_MODE=true, routes with no or incorrect auth
    • No auth still returns Not authenticated, meant that it still rejected on this test case ❌
      curl -s -X POST http://localhost:8000/portfolio/state \
          -H "Content-Type: application/json" -d '
        {
          "account_names": ["master_account"],
          "refresh": false
        }' | jq
      
      {
        "detail": "Not authenticated"
      }
      

CORS: configured origins accepted, others rejected; credentialed requests still work.

  curl -s -D - -o /dev/null -u admin:admin -X POST http://localhost:8000/portfolio/state \
    -H "Origin: http://localhost:3000" \
    -H "Content-Type: application/json" \
    -d '
  {
    "account_names": ["master_account"],
    "refresh": false
  }'
  • Quick check what is CORS and how it works ok
  • Tested with http://localhost:3000, returns access controller allow origin and credentials are localhost and true
  • Test failed on CORS rejected-origin handling ❌
    • Tested whether /portfolio/state would block origins that should not be permitted, such as http://ralphwashere:36/ and 1:36
    • Sent authenticated requests with those Origin values and checked the response headers
    • It should pass by rejecting those origins and not returning Access-Control-Allow-Origin for them
      curl -s -D - -o /dev/null -u admin:admin -X POST http://localhost:8000/portfolio/state \
          -H "Origin: 1:36" \
          -H "Content-Type: application/json" \
          -d '
        {
          "account_names": ["master_account"],
          "refresh": false
        }'
        
        
      HTTP/1.1 200 OK
      date: Thu, 18 Jun 2026 14:44:04 GMT
      server: uvicorn
      content-length: 464
      content-type: application/json
      access-control-allow-origin: 1:36
      access-control-allow-credentials: true
      vary: Origin
      

Path traversal: account/connector names and archived-bot endpoints reject ../ / absolute paths; valid names still work.


Todo: Test in progress

@rapcmia

rapcmia commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Commit 1402e47

  • DEBUG_MODE has been removed
  • Retest all and make sure image is set to local PR178
  • Clone fresh version and test usign docker
  • Start HAPI with condor main branch ok

Auth: login with real creds works; startup logs the default-credentials warning ✅

  • Routes without auth, request rejected with payload response Not authenticated
  • Routes with auth but incorrect credentials, payload response returned Incorrect username or password
  • Routes with correct credentials, payload response returned requested data

CORS: configured origins accepted, others rejected; credentialed requests still work. ✅

  • Test http://localhost/, returned 200 ok along with access-controller-allow-origin
  • Test http://ralphwashere/, returned 200 but omitted access-controller-allow-origin
    • curl still gets 200 ok not does not enforce CORS

Path traversal: account/connector names and archived-bot endpoints reject ../ / absolute paths; valid names still work ✅

  • Test archive-bot traversal rejection
    curl -s -u XXX:XXX 'http://localhost:8000/archived-bots/..%2F/status' | jq
    curl -s -u XXX:XXX 'http://localhost:8000/archived-bots/%2Ftmp%2Ftest/status' | jq
    curl -s -u XXX:XXX 'http://localhost:8000/archived-bots/..%2F%2F%2F%2Ftmp%2F%2F%2F%2Fx/status' | jq
    
    {
      "detail": "Path '../' is outside the archived bots directory"
    }
    {
      "detail": "Path '/tmp/test' is outside the archived bots directory"
    }
    {
      "detail": "Path '..////tmp////x' is outside the archived bots directory"
    }
    
    • GET /archived-bots/{db_path}/status now rejects traversal-style and path-style values with a clean error instead of attempting to open a SQLite path.
    • ..%2F, %2Ftmp%2Ftest, and ..%2F%2F%2F%2Ftmp%2F%2F%2F%2Fx all returned a clear "outside the archived bots directory" message.
  • Test valid account and connector names
    • GET /accounts/master_account/credentials still works and returned configured connector names for the real account.
    • GET /connectors/binance/config-map still works and returned the expected config fields for a valid connector name.
  • Test traversal-style account and connector names rejection
      curl -s -u XXX:XXX 'http://localhost:8000/accounts/..%2F/credentials' | jq
      curl -s -u XXX:XXX 'http://localhost:8000/accounts/%2Ftmp%2Ftest/credentials' | jq
      curl -s -u XXX:XXX 'http://localhost:8000/connectors/..%2F/config-map' | jq
      curl -s -u XXX:XXX 'http://localhost:8000/connectors/%2Ftmp%2Ftest/config-map' | jq
    
      {
        "detail": "Not Found"
      }
    
    • GET /accounts/..%2F/credentials and GET /accounts/%2Ftmp%2Ftest/credentials returned {"detail":"Not Found"}.
    • GET /connectors/..%2F/config-map and GET /connectors/%2Ftmp%2Ftest/config-map also returned {"detail":"Not Found"}.

Archived bots happy path: call /archived-bots to list databases, pick a real db_path from that response, then verify /status, /summary, /orders, /trades, and /controllers all still work with that valid path. ✅

  • Deploy a bot, wait for filled orders then stop to archive
  • GET /archived-bots/ returned a real archived bot database path.
  • The listed db_path worked on /status, /summary, /orders, /trades, and /controllers.
  • /status returned normal archived-bot metadata and table checks for the selected database.
  • /summary returned expected aggregate data
  • /orders returned records and /trades returned records with no pagination errors.
  • /controllers returned controller record for the archived bot.

Portfolio routes: test /portfolio/state, /portfolio/history, and /portfolio/distribution for one account and for multiple accounts. Also verify cursor pagination and connector filters, since both the service split and history query

changes affect these routes. ✅


Executors and trading flow: create an executor, confirm it appears in search, then stop it. If possible, also let one executor finish naturally. After that, verify it is persisted only once, active-order reconciliation still works ✅

  • Executor search and stop flow, ok
  • Active order clean up after executor stop, ok
  • The exchange_order_id preserved in historical orders after stop, ok

Market data candles: test /market-data/candles and /market-data/historical-candles with valid inputs and with bad connector or trading-pair values, so we cover both validation errors and timeout-related behavior. ✅

  • /market-data/candles returned real-time candle rows for a valid connector and trading pair
  • /market-data/historical-candles returned historical candle rows for the same valid Binance market and time window
  • Both candle routes returned clear API errors for an unsupported connector
  • Both candle routes returned a fast invalid-symbol error for a bad trading pair instead of hanging
  • No timeout behavior was observed in these invalid-input checks

Bot lifecycle: test bot start and stop routes, especially the ones that depend on BotsOrchestrator. Also verify shutdown is clean and no MQTT-related tasks are left hanging, because that part changed significantly. ✅

  • Deployed a real pmm3-test01 bot, ok
  • Verified it showed as running, then called stop-and-archive-bot
  • Rechecked /bot-orchestration/status, /bot-orchestration/mqtt, and /archived-bots/ after allowing time for the background cleanup to finish, all ok

@rapcmia

rapcmia commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Commit ce8e1ea

curl -s -u XXX:XXX -X POST http://localhost:8000/portfolio/history -H "Content-Type: application/json" -d '
  {
    "account_names": ["master_account"],
    "connector_names": ["gate_io"],
    "limit": 1,
    "interval": "5m"
  }' | jq

...trimmed
{
    "data": [
      {
        "timestamp": "2026-06-19T13:44:00+00:00",
        "state": {
          "master_account": {
            "gate_io": [
              { "token": "BTC" },
              { "token": "SOL" },
              { "token": "NOT" },
              { "token": "HYPE" },
              { "token": "GRAM" },
              { "token": "XRP" },
              { "token": "ETH" },
              { "token": "USDT" },
              { "token": "POLIS" },
              { "token": "TLOS" },
              { "token": "GT" }
            ]
          }
        }
      }
    ],
...trimmed

@cardosofede cardosofede merged commit 5b8918a into main Jun 19, 2026
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