Revalidate treasury proposals before execution#550
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📝 WalkthroughWalkthroughThis PR introduces a treasury proposal governance layer: admin-initiated bounty create/pay/close operations now create public proposals (stored in DB) with a 24-hour delay, epoch reserve caps, and GitHub-based challenges; APIs, admin UI, migrations, and tests are updated to submit, list, execute, and challenge proposals. ChangesTreasury Proposal Governance for Bounty Operations
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 |
ifanatics-media
left a comment
ifanatics-media
left a comment
There was a problem hiding this comment.
Requesting changes because the current PR head fails the required formatting gate.
Evidence checked:
- Hosted CI for head
a21c81355efe048fb0ff1e1f4482be74c776a023ran the full test suite successfully (436 passed) and then failed duringruff format --check .withWould reformat: tests/test_treasury_proposals.py. - I checked out PR #550 locally and reproduced the same formatter failure with
.\.venv\Scripts\python.exe -m ruff format --check tests\test_treasury_proposals.py. - The focused behavior/migration tests the PR body mentions still pass locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_treasury_proposals.py tests\test_migrations.py -q-> 23 passed. git diff --check origin/main...HEADis clean, so this looks scoped to Ruff formatting rather than whitespace errors.
Suggested fix: run ruff format tests/test_treasury_proposals.py, commit the formatter output, and let CI rerun. No deeper behavior blocker found in the slice I checked.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_treasury_proposals.py (1)
1-796:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix failing Ruff format check in this new test module.
CI is failing because this file is not Ruff-formatted. Please run
ruff format tests/test_treasury_proposals.py(orruff format .) and re-push.As per coding guidelines
**/*.py: "Runruff format --check .to verify code formatting".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 244de267-aaf4-4ae5-97a2-ddaa0fc46f98
📒 Files selected for processing (19)
README.mdapp/admin.pyapp/admin_routes.pyapp/bounty_api.pyapp/main.pyapp/models.pyapp/templates/admin.htmlapp/treasury.pyapp/treasury_routes.pydocs/admin-runbook.mddocs/agent-guide.mddocs/api-examples.mddocs/bounty-rules.mddocs/ledger.mdmigrations/versions/0005_treasury_proposals.pytests/test_admin_helpers.pytests/test_migrations.pytests/test_security.pytests/test_treasury_proposals.py
Baijack-star
left a comment
Baijack-star
left a comment
There was a problem hiding this comment.
Requesting changes: I found one execution-time correctness blocker in addition to the existing formatting failure.
app/treasury.pydouble-counts the currently executingcreate_bountyproposal during reserve-cap revalidation._validate_create_bounty_proposal()computespending_reservedwith_pending_create_bounty_reserved_microunits(... through_proposal_id=exclude_id), which already includes the current pending proposal, and then addsreservedagain. A proposal that exactly fits the 10,000 MRWK epoch cap can be created but later fails execution withtreasury epoch reserve cap exceeded.
Reproduction against head a21c81355efe048fb0ff1e1f4482be74c776a023:
- create schema/genesis in a temp SQLite DB;
POST /api/v1/bountieswithreward_mrwk: "10000",max_awards: 1-> 200, proposal created;- move
executes_afterinto the past; POST /api/v1/treasury/proposals/{id}/execute-> 400{'detail': 'treasury epoch reserve cap exceeded'}.
Expected: the exact-cap proposal should execute because creation already accepted it and no other epoch reserve exists. The execution revalidation should exclude the current proposal from pending totals, or otherwise avoid adding its reserve twice.
- The required formatting gate still fails locally:
./.venv/bin/python -m ruff format --check tests/test_treasury_proposals.py->Would reformat: tests/test_treasury_proposals.py.
Positive checks while reviewing:
- CodeRabbit is green;
- the stale payout/closed-bounty path described in the PR body is directionally covered, but the exact reserve-cap boundary needs a regression test before merge.
|
Addressed the review items in follow-up commit ceabbf7 on the PR branch:
Validation saved locally:
|
Baijack-star
left a comment
Baijack-star
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I rechecked current head ceabbf75d67154e100b5f3bf6fd29ac59726c3b9.
The original create-bounty exact-cap double-count blocker I reported is fixed on this head. I reran the exact-cap smoke locally: a create_bounty proposal with reward_mrwk: "10000" and max_awards: 1 now executes successfully after the delay and stores the proposal as executed.
I am still requesting changes because the required project check is red on two test expectation mismatches, and the formatter gate is still not clean locally:
- Hosted CI
Quality, readiness, docs, and image checksfails with2 failed, 434 passed. tests/test_treasury_proposals.py::test_proposal_execution_requires_admin_delay_and_is_idempotentnow receives HTTP 409 for the too-early execution path, but still asserts 400.tests/test_treasury_proposals.py::test_challenges_require_accepted_work_and_can_block_invalid_proposalsnow receives HTTP 409 for blocked-proposal execution, but still asserts 400.- Local focused reproduction on this head matches CI:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_treasury_proposals.py::test_proposal_execution_requires_admin_delay_and_is_idempotent tests/test_treasury_proposals.py::test_challenges_require_accepted_work_and_can_block_invalid_proposals -q->2 failedwith409 == 400assertion mismatches. - Local formatting gate still reports:
./.venv/bin/python -m ruff format --check tests/test_treasury_proposals.py->Would reformat: tests/test_treasury_proposals.py.
Expected fix: update the affected assertions to the intended 409 conflict semantics for execution-state conflicts, keep the existing detail assertions, run Ruff format on tests/test_treasury_proposals.py, and let CI rerun. I did not find a remaining blocker for the exact-cap execution behavior after your latest fix.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/treasury_routes.py (1)
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap the remaining execution revalidation conflicts to 409.
execute_treasury_proposal()now re-runs the action validators, but several stale-state failures from that path still fall through to 400 here — for example"bounty is not open"and"treasury epoch reserve cap exceeded"fromapp/treasury.py. That makes an outdated proposal look like bad input instead of a resource-state conflict.Suggested fix
if detail in { "proposal already executed", "proposal has blocking challenge", "proposal is not pending", "proposal delay has not elapsed", + "bounty is not open", + "bounty has pending close proposal", + "bounty has pending payout proposals", + "close_bounty proposal already pending", + "create_bounty proposal already pending", + "pay_bounty proposal already pending for submission", + "pending payout proposals exceed bounty remaining awards", + "pending payout proposals exceed bounty reserve", "submission already paid", + "treasury epoch reserve cap exceeded", }: return HTTPException(status_code=409, detail=detail)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 822fb48a-e1a9-4b20-9f58-beef0d48c940
📒 Files selected for processing (4)
app/bounty_api.pyapp/treasury.pyapp/treasury_routes.pytests/test_security.py
|
Reviewed current PR #550 head Independent current-head recheck: I still agree this PR should not merge until the red gate is fixed. I did not find a new application-logic blocker beyond the already requested changes, but I reproduced the current failure state locally. Evidence checked:
The two focused failures are still the execution-state status-code mismatches:
Expected fix: format both |
|
Reviewed current PR #550 head No blocker found in the revalidated treasury-proposal execution slice. Evidence checked:
Validation run locally:
This looks mergeable from my reviewed current-head slice. I did not review every documentation sentence or every admin UI path in this pass; the review is focused on the treasury proposal execution/revalidation state that had been blocking the PR. |
|
Retargeting this to |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 69078f19fdf5b3a4d4c8cb38d4ebfd2cb14aa053 after the retarget to governance-mvp-proposals.
No blocker found in the treasury proposal revalidation slice I checked. The current head resolves the earlier execution-state failures by mapping stale/conflict execution errors to 409, including too-early execution, blocking challenges, stale closed bounties, duplicate pending proposals, pending payout reserve conflicts, and epoch reserve conflicts. The focused assertions now match that intended conflict semantics, and the previously red formatter/test gates are green.
Validation performed locally:
.\.venv\Scripts\python.exe -m pytest tests\test_treasury_proposals.py tests\test_migrations.py -q-> 23 passed.\.venv\Scripts\python.exe -m pytest tests\test_treasury_proposals.py::test_proposal_execution_requires_admin_delay_and_is_idempotent tests\test_treasury_proposals.py::test_challenges_require_accepted_work_and_can_block_invalid_proposals -q-> 2 passed.\.venv\Scripts\ruff.exe check app\treasury.py app\treasury_routes.py tests\test_treasury_proposals.py tests\test_migrations.py-> passed.\.venv\Scripts\ruff.exe format --check app\treasury.py app\treasury_routes.py tests\test_treasury_proposals.py tests\test_migrations.py-> 4 files already formatted.\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangh pr checks 550 --repo ramimbo/mergework-> CodeRabbit pass; Quality/readiness/docs/images pass
Scope note: I focused on execution revalidation, route error mapping, migration coverage, formatting, docs smoke, and current hosted checks. I did not do a full end-to-end admin UI review.
7 participants
Follow-up fix for PR #458 / bounty #512.
What changed:
create_bounty,pay_bounty, andclose_bountyproposals immediately before execution.pay_bountyproposal whose bounty is closed after proposal creation but before execution.Why:
Validation:
PYTHONPATH=. python -m pytest -q tests/test_treasury_proposals.py tests/test_migrations.py23 passed in 6.74sSummary by CodeRabbit
New Features
UI
Database
Documentation
Tests