Add production treasury executor service#650
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a periodic treasury executor service that automatically discovers and executes due treasury proposals. Execution orchestration is extracted into ChangesTreasury Executor Service
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5473622-ac1c-4969-bcf1-133d93afd4dc
📒 Files selected for processing (12)
.env.exampleapp/treasury_executor.pyapp/treasury_routes.pydocker-compose.ymldocs/admin-runbook.mdscripts/docs_smoke.pyscripts/treasury_executor.pytests/test_docker_compose.pytests/test_docs_public_urls.pytests/test_treasury_executor.pytests/test_treasury_executor_script.pytests/test_treasury_proposals.py
bitdamii
left a comment
There was a problem hiding this comment.
Reviewed current head ab777afb0fadba4f203d14152decff4784161a32 as a non-author.
Verdict: APPROVE with a mergeability note. I did not find a code blocker in this head. Pre-review GitHub mergeStateStatus reported UNSTABLE, but a local merge-tree against origin/main completed cleanly; I would still recheck branch protection/mergeability after review state refresh before merging.
Evidence:
- Inspected
scripts/treasury_executor.py,app/treasury_executor.py,app/treasury_routes.py,docker-compose.yml,.env.example,docs/admin-runbook.md,tests/test_treasury_executor.py,tests/test_treasury_executor_script.py,tests/test_treasury_proposals.py,tests/test_docker_compose.py, andtests/test_docs_public_urls.py. - Verified the one-shot executor failure path now returns non-zero and has a regression test.
- Verified due proposals are selected oldest-first, future proposals are skipped, LedgerError failures are reported while later due proposals continue, and create-bounty GitHub finalization records status back onto the proposal result.
- Verified the route refactor still maps LedgerError through the existing HTTP error handling.
- Verified the Docker service is disabled by default through
.env.example, shares the production data volume, and the runbook documents executor enablement, logs, finalization checks, and manual fallback expectations.
Validation run locally:
uv run --extra dev python -m pytest tests/test_treasury_executor.py tests/test_treasury_executor_script.py tests/test_treasury_proposals.py tests/test_docker_compose.py tests/test_docs_public_urls.py -q-> 76 passed.uv run --extra dev ruff check app/treasury_executor.py app/treasury_routes.py scripts/treasury_executor.py tests/test_treasury_executor.py tests/test_treasury_executor_script.py tests/test_treasury_proposals.py tests/test_docker_compose.py tests/test_docs_public_urls.py-> passed.uv run --extra dev ruff format --check ...-> 8 files already formatted.uv run --extra dev python -m mypy app/treasury_executor.py app/treasury_routes.py scripts/treasury_executor.py-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean.- GitHub Quality/readiness/docs/image check -> success.
No private data, credentials, wallet material, production mutation, price/exchange/bridge/off-ramp claims, or fabricated payout claims used.
Thanhdn1984
left a comment
There was a problem hiding this comment.
Reviewed current head ab777afb0fadba4f203d14152decff4784161a32 as a non-author.
Verdict: APPROVE. Clean extraction of the treasury-executor concern from the route handler into a reusable app/treasury_executor.py module plus a standalone scripts/treasury_executor.py entry point.
Strengths:
- Proper separation: route handler now delegates to
execute_treasury_proposal_with_finalization, keeping the HTTP layer thin. - Executor loop is opt-in (
MERGEWORK_TREASURY_EXECUTOR_ENABLED), with sane defaults (300s interval, 25 batch limit) and reasonable bounds validation. _db_utchelper correctly strips timezone for SQLite comparison.- Error isolation: one proposal failure does not block remaining due proposals in the same batch.
- Docker Compose service uses the same
.envand shared data volume — straightforward production deployment. - Comprehensive test coverage: unit tests for config parsing, executor logic with fake finalizer, finalizer failure handling, Docker Compose structure, and docs smoke tests.
- Existing
test_treasury_proposals.pymonkeypatch targets updated to match the new import path.
Minor observation (non-blocking):
- The disabled-but-running loop (
while True: time.sleep(...)) inmain()means the container stays alive even when disabled, which is fine for therestart: unless-stoppedstrategy but could be documented as intentional idle behavior.
No security, correctness, or merge-readiness issues found.
Summary
scripts/treasury_executor.pyand atreasury-executorDocker Compose service that uses the production.envand shared data volumeMERGEWORK_TREASURY_EXECUTOR_ENABLED=1, plus interval and batch-limit settingsMaintainer Note
Maintainer infrastructure PR; not requesting a bounty.
This does not change the 24-hour proposal delay, challenge behavior, reserve cap checks, or the rule that GitHub issues must not get
mrwk:bounty/Reserved on MergeWorkbefore successful proposal execution. It only adds a production-side loop that can execute proposals after they become eligible, using the same execution path as the manual admin route.After merge and deploy, production still needs the real production
.envupdated withMERGEWORK_TREASURY_EXECUTOR_ENABLED=1, then thetreasury-executorservice started. First due create-bounty proposal should be treated as a canary: inspectresult.github_issue_finalization, then confirm bothmrwk:bountyandReserved on MergeWorkare present before relying on the service unattended.Validation
./.venv/bin/python -m pytest->535 passed./.venv/bin/python -m ruff format --check ../.venv/bin/python -m ruff check ../.venv/bin/python -m mypy app./.venv/bin/python scripts/docs_smoke.pycp .env.example .env; docker compose config --quiet; rm -f .envSummary by CodeRabbit
New Features
Chores
Documentation
Tests