Closed
Conversation
📝 WalkthroughWalkthroughThis PR implements milestone-based payouts for T3 bounties, enabling staged partial payments at defined checkpoints instead of all-or-nothing releases. The implementation spans a database migration creating a Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/BountyCreationWizard.tsx (1)
24-54: 🧹 Nitpick | 🔵 Trivial
isValidBountyFormDatadoesn't validate milestones array structure.The validation function checks most fields but doesn't validate the
milestonesarray. Malformed milestone objects from corrupted localStorage could cause runtime errors when accessingm.percentageorm.description.♻️ Proposed addition to validation
// Validate rewardAmount if (d.rewardAmount !== undefined && typeof d.rewardAmount !== 'number') return false; + // Validate milestones + if (d.milestones !== undefined) { + if (!Array.isArray(d.milestones)) return false; + for (const m of d.milestones) { + if (typeof m !== 'object' || m === null) return false; + if (typeof m.milestone_number !== 'number') return false; + if (typeof m.description !== 'string') return false; + if (typeof m.percentage !== 'number') return false; + } + } + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/BountyCreationWizard.tsx` around lines 24 - 54, The isValidBountyFormData validator currently skips validating the milestones field; update isValidBountyFormData to check that d.milestones (if defined) is an array and that every element is an object with the expected properties (e.g., percentage is a number within a sensible range and description is a string) to prevent runtime errors when code accesses m.percentage or m.description; locate and modify the isValidBountyFormData function and add the milestones checks (array check, element type check, and per-item property checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/alembic/versions/003_add_milestones_table.py`:
- Line 48: The new milestones table migration defines the column payout_tx_hash
as sa.String(100) which is inconsistent with the ORM and other tables
(PayoutTable, BuybackTable) that use String(128); update the migration's column
definition for payout_tx_hash to sa.String(128) so transaction-hash lengths
match across schemas and keep migrations consistent with the ORM models.
- Around line 49-60: The migration defines created_at and updated_at as
nullable=True but the ORM model MilestoneTable expects non-nullable timestamps;
update the Alembic migration to make the columns non-nullable (set
nullable=False for "created_at" and "updated_at" columns in the migration) so
the schema matches MilestoneTable and avoids insert/constraint mismatches,
keeping the server_default=sa.func.now() intact to populate values when omitted.
- Around line 63-72: Add the missing unique composite index
ix_bmilestone_bounty_num on the bounties_milestones table to enforce uniqueness
of (bounty_id, milestone_number): in the migration's upgrade add an
op.create_index call creating ix_bmilestone_bounty_num on columns ["bounty_id",
"milestone_number"] with unique=True (alongside the existing
ix_bounties_milestones_* indexes), and in the downgrade add the corresponding
op.drop_index("ix_bmilestone_bounty_num") to remove it; locate these changes in
the migration functions (upgrade/downgrade) of 003_add_milestones_table.py.
In `@backend/app/api/bounties.py`:
- Around line 813-816: Replace the incorrect exception type
UnauthorizedDisputeAccessError with UnauthorizedMilestoneAccessError in the
milestone endpoint except blocks so MilestoneService's raises are caught; update
the three occurrences (the except blocks currently grouping ValueError and
UnauthorizedDisputeAccessError around the MilestoneService calls) to catch
UnauthorizedMilestoneAccessError and re-raise an appropriate HTTPException (400
or 403 as intended) with the error message. Ensure the change is applied to all
three locations noted so unauthorized milestone access no longer produces an
unhandled 500.
- Around line 798-816: The code uses the typing name List in the
create_milestones endpoint (response_model=List[MilestoneResponse] and parameter
data: List[MilestoneCreate]) but List is not imported; add the missing import
from typing (e.g. from typing import List) near other imports so List is defined
for the create_milestones function and related type annotations
(MilestoneResponse, MilestoneCreate) resolve correctly.
In `@backend/app/models/milestone.py`:
- Around line 22-24: The MilestoneCreate schema currently requires
milestone_number but MilestoneService.create_milestones (which reassigns numbers
with enumerate) ignores it; either make milestone_number optional on
MilestoneCreate/MilestoneBase (e.g., allow None) or change create_milestones to
respect a provided milestone_number (use the provided value when present,
otherwise auto-assign with enumerate). Update the schema
(MilestoneCreate/milestone_number Field) or update the service logic in
MilestoneService.create_milestones to check each input's milestone_number before
overwriting so API behavior is consistent and unambiguous.
- Around line 11-16: The Milestone lifecycle enum is missing a REJECTED state;
update the MilestoneStatus enum by adding a REJECTED member (e.g., REJECTED =
"rejected") so the lifecycle can represent owner rejections and allow
transitions from SUBMITTED to REJECTED and then to PENDING for resubmission;
ensure any switch/if logic that checks MilestoneStatus (e.g., places that
read/write MilestoneStatus or functions handling status transitions) is updated
to handle the new REJECTED value.
In `@backend/app/models/tables.py`:
- Line 156: The Milestone model's description column is using Text while the
migration defines it as sa.String(500); update them to match. Edit the Milestone
model (class Milestone) to declare description = Column(String(500),
nullable=False) and import String from sqlalchemy, or alternatively change the
migration to use sa.Text() if you want unlimited text—ensure both the ORM
declaration (description Column) and the migration column type (sa.String(500)
vs sa.Text) are identical so the 500-character limit is enforced consistently.
In `@backend/app/services/bounty_service.py`:
- Around line 159-163: The import load_milestones_for_bounty is unused in the
list function — remove the unused import from the import block or, if milestones
should be loaded per bounty, call load_milestones_for_bounty inside the list
function (e.g., within the function that currently hardcodes milestones=[]),
replacing the hardcoded milestones with the actual call and mapping results to
the milestones field for each bounty; update the import accordingly if you
choose to remove or keep it.
- Around line 321-327: The milestone tier and total-percentage checks duplicated
in create_bounty should be centralized in MilestoneService.create_milestones to
avoid divergence: remove the tier check (data.tier != 3) and the
total_percentage sum validation from create_bounty and instead ensure
create_bounty delegates milestone creation/validation to
MilestoneService.create_milestones (which should raise MilestoneValidationError
on invalid tier or non-100% sum for data.milestones); update create_bounty to
call MilestoneService.create_milestones(data.milestones, data.tier) (or the
existing service API) and rely on that single validation point.
- Around line 344-352: The bounty persistence and milestone creation are using
separate DB sessions (see _persist_to_db then async with get_db_session() +
MilestoneService.create_milestones), which can leave a persisted bounty without
milestones on failure; change the flow to perform both operations in the same
transaction/session so they commit or rollback together: open a single
session/transaction, call the logic that persists the bounty (currently in
_persist_to_db) within that session, then instantiate MilestoneService with that
same session and call create_milestones(bounty.id, ...); on error let the
transaction rollback so the bounty is not left half-created, or alternatively
add compensating delete logic to remove the bounty if create_milestones fails.
Ensure _load_bounty_from_db (if still needed) uses the same committed
session/transaction semantics.
In `@backend/app/services/milestone_service.py`:
- Line 7: The import list in milestone_service.py includes an unused symbol
`func` from SQLAlchemy; remove `func` from the import statement (`from
sqlalchemy import select, and_, func`) so it becomes `from sqlalchemy import
select, and_` to eliminate the unused import warning and keep imports minimal.
- Around line 82-83: The returned MilestoneResponse objects may be stale because
DB defaults (created_at/updated_at) are set by the DB and not present on the
Python objects after await self.db.commit(); after committing, iterate the
new_milestones and call the session refresh method (e.g., await
self.db.refresh(m) for each m) to reload DB-populated fields before converting
to MilestoneResponse.model_validate; update the code around commit/return that
uses new_milestones and MilestoneResponse to refresh each instance.
- Line 183: Add a module-level logger and remove the inline import so calls like
logger.warning("No wallet address found for claimant %s, skipping automatic
payout", bounty.claimed_by) won't raise NameError: define logger =
logging.getLogger(__name__) near the top of milestone_service.py (and ensure
import logging is at module scope), then delete the local/inline logging import
and any local logger assignment inside the except block so all logging uses the
single module-level logger.
- Around line 61-66: The create_milestones flow deletes all existing
MilestoneTable rows (stmt/select + self.db.delete loop) which can wipe SUBMITTED
or APPROVED milestones and cause data loss/double payouts; modify
create_milestones to first query existing milestones and skip deletion (or
raise) if any have status in the finalized set (e.g., "SUBMITTED","APPROVED") or
are linked to processed payouts, and only allow removal of drafts/unsubmitted
milestones; implement the guard by checking result.scalars().all() for those
statuses and either abort with an error/exception or filter deletions to only
non-finalized records before calling self.db.delete.
In `@backend/app/services/telegram_service.py`:
- Around line 28-36: The Telegram send code is escaping text for MarkdownV2 via
_sanitize_telegram_text but the request omits parse_mode so escapes show
literally; update the json payload in the AsyncClient.post call (the sendMessage
request using TELEGRAM_BOT_TOKEN and TELEGRAM_CHAT_ID) to include "parse_mode":
"MarkdownV2" so Telegram interprets the escapes correctly; alternatively, if you
intended plain text, modify _sanitize_telegram_text to stop MarkdownV2 escaping
(keep only truncation) and do not add parse_mode—choose one approach and make
the corresponding change to the send logic or sanitizer.
In `@backend/tests/test_milestones_integration.py`:
- Around line 90-92: The authorization test currently calls
svc.approve_milestone for m3 while m3 is still PENDING so MilestoneSequenceError
triggers before authorization is checked; move the with
pytest.raises(UnauthorizedMilestoneAccessError): await
svc.approve_milestone(bounty_id, str(m3.id), contributor_id) block to after the
call that submits m3 (the submit_milestone invocation) so the milestone is in
the submitted state and the UnauthorizedMilestoneAccessError path is exercised;
ensure you still reference m3 and contributor_id in the moved assertion.
- Around line 24-103: The test_three_milestone_full_lifecycle integration test
leaves created state behind; update it to ensure DB isolation by using a
transactional or cleanup fixture—either run the test logic inside a DB
transaction via your existing get_db_session fixture with an automatic rollback
or call explicit teardown functions (e.g., delete_bounty/delete_contributor)
after assertions; modify the test to use the transactional fixture or add
try/finally around create_bounty/create_contributor and MilestoneService calls
to remove the created bounty (bounty_id) and contributor (contributor_id) so
subsequent tests are not polluted.
- Around line 98-102: Add concrete assertions to verify payouts: assert that
approved_m1.payout_tx_hash, approved_m2.payout_tx_hash, and
approved_m3.payout_tx_hash are truthy (not None/empty) and assert the paid
amounts equal the expected values (200_000 for approved_m1, 300_000 for
approved_m2, 500_000 for approved_m3) by checking the model/service fields
(e.g., approved_m1.payout_amount or approved_m1.paid_amount) or the service
response used in the test; if the test must fetch updated state, reload the
milestone records before asserting.
- Around line 74-76: The test is asserting the wrong failure path: instead of
testing sequence enforcement it currently hits the "not submitted" check for m2;
update the test to first submit m2 (call svc.submit_milestone(bounty_id,
str(m2.id), owner_id)) so m2 transitions from PENDING to SUBMITTED, then attempt
to approve m2 via svc.approve_milestone(bounty_id, str(m2.id), owner_id) and
assert it raises MilestoneSequenceError; keep references to
svc.approve_milestone, svc.submit_milestone, m1, m2, and MilestoneSequenceError
so the intent and target checks are clear.
In `@backend/tests/test_milestones.py`:
- Around line 1-7: The file contains an unused import: remove the sqlalchemy
symbol "select" from the imports in test_milestones.py; edit the import list at
the top of the file to delete "select" so only used imports (pytest, uuid,
datetime, timezone) remain.
- Around line 70-73: The test's expected regex doesn't match the actual error
raised by MilestoneService.approve_milestone; update the pytest.raises match in
test_milestones.py to reflect the service's message (e.g., match "Milestone
cannot be approved in .* state\\. It must be 'submitted' first\\." or a simpler
regex like "Milestone cannot be approved in .*state") so the test matches the
error thrown by approve_milestone in MilestoneService when a milestone is
pending.
- Around line 103-109: The test currently expects a ValueError and the message
"Milestones can only be added to T3 bounties" but the service raises
MilestoneValidationError with message "Milestones can only be added to T3
(Large) bounties"; update the test to expect MilestoneValidationError (not
ValueError) and change the pytest.raises match to the exact message "Milestones
can only be added to T3 (Large) bounties" when calling svc.create_milestones
with MilestoneCreate, or alternatively change MilestoneValidationError to
subclass ValueError if you prefer altering the exception type instead of the
test; reference svc.create_milestones and MilestoneValidationError to locate the
code to adjust.
In `@frontend/src/components/bounties/MilestoneProgress.tsx`:
- Around line 91-109: The Submit button currently renders whenever
milestone.status === 'pending' regardless of prior milestones; update
MilestoneProgress.tsx to prevent out-of-order submissions by requiring the
previous milestone be approved before enabling submit: either add a prop (e.g.,
previousMilestoneApproved: boolean) or accept (milestones: Milestone[], index:
number) and compute const canSubmit = milestone.status === 'pending' &&
previousMilestoneApproved; then use canSubmit in the conditional and as the
disabled prop on the Submit button (and optionally show a disabled
tooltip/explain). Ensure you reference the existing onSubmit, loading,
milestone.status and isCreator logic so only the intended Submit button is
affected.
- Line 54: The code currently calls milestones.sort(...) which mutates the
incoming milestones prop; update the render in MilestoneProgress.tsx to sort
without mutation by creating a sorted copy (e.g., use a shallow copy via spread
or Array.prototype.toSorted) before calling .map, ensuring the original
milestones prop is not modified; locate the expression with milestones.sort((a,
b) => a.milestone_number - b.milestone_number).map(...) and replace it with a
non-mutating sorted copy followed by .map to preserve props immutability and
prevent React rendering issues.
- Around line 46-51: The inline style for the progress width can produce NaN
when totalPercentage is 0; update the calculation used in the style on the inner
div (where width: `${(approvedPercentage / totalPercentage) * 100}%`` is set) to
defensively handle totalPercentage === 0 by using a conditional/fallback (e.g.,
set width to "0%" or compute percentage as 0 when totalPercentage is zero),
ensuring the component (MilestoneProgress) never sets an invalid CSS width.
- Around line 3-12: Remove the duplicated Milestone interface declared in
MilestoneProgress.tsx and instead import the shared Milestone type from the
central bounty types file; update the top of MilestoneProgress.tsx to import {
Milestone } from the shared types (ensuring the exported name matches) and
delete the local interface declaration, then confirm all usages in
MilestoneProgress (props, state, function signatures) reference the imported
Milestone type.
In `@frontend/src/components/BountyCreationWizard.tsx`:
- Around line 943-944: Frontend validation in BountyCreationWizard.tsx computes
totalPerc from formData.milestones and uses a 0.01 tolerance (Math.abs(totalPerc
- 100) > 0.01) which mismatches the backend's 0.001 tolerance; change the
frontend check to use the same tolerance (0.001) or perform an exact normalized
check so values that pass the UI cannot fail the backend (replace 0.01 with
0.001 in the Math.abs(totalPerc - 100) comparison or implement equivalent
normalization before comparison).
- Around line 936-937: In the switch statement inside BountyCreationWizard (the
switch handling the wizard steps / case 3), remove the duplicate consecutive
`break` so only a single `break` terminates case 3; this eliminates the
unreachable/dead `break` that appears to be a merge artifact and prevents the
syntax/dead-code issue.
---
Outside diff comments:
In `@frontend/src/components/BountyCreationWizard.tsx`:
- Around line 24-54: The isValidBountyFormData validator currently skips
validating the milestones field; update isValidBountyFormData to check that
d.milestones (if defined) is an array and that every element is an object with
the expected properties (e.g., percentage is a number within a sensible range
and description is a string) to prevent runtime errors when code accesses
m.percentage or m.description; locate and modify the isValidBountyFormData
function and add the milestones checks (array check, element type check, and
per-item property checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5024338-f7f5-4f7e-ab72-0689cd7bbfb9
📒 Files selected for processing (22)
backend/alembic/versions/003_add_milestones_table.pybackend/app/api/bounties.pybackend/app/database.pybackend/app/exceptions.pybackend/app/models/bounty.pybackend/app/models/bounty_table.pybackend/app/models/milestone.pybackend/app/models/tables.pybackend/app/services/bounty_lifecycle_service.pybackend/app/services/bounty_service.pybackend/app/services/milestone_service.pybackend/app/services/pg_store.pybackend/app/services/telegram_service.pybackend/tests/test_milestones.pybackend/tests/test_milestones_integration.pyfrontend/src/components/BountyCreationWizard.tsxfrontend/src/components/BountyDetailPage.tsxfrontend/src/components/bounties/BountyCard.tsxfrontend/src/components/bounties/CreatorBountyCard.tsxfrontend/src/components/bounties/MilestoneProgress.tsxfrontend/src/hooks/useBountySubmission.tsfrontend/src/types/bounty.ts
| ), | ||
| sa.Column("submitted_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("approved_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("payout_tx_hash", sa.String(100), nullable=True), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: payout_tx_hash length
Migration uses String(100) but ORM and other tables (PayoutTable, BuybackTable) use String(128) for transaction hashes. While unlikely to cause issues (Solana tx hashes are 88 chars base58), consistency is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/alembic/versions/003_add_milestones_table.py` at line 48, The new
milestones table migration defines the column payout_tx_hash as sa.String(100)
which is inconsistent with the ORM and other tables (PayoutTable, BuybackTable)
that use String(128); update the migration's column definition for
payout_tx_hash to sa.String(128) so transaction-hash lengths match across
schemas and keep migrations consistent with the ORM models.
Comment on lines
+49
to
+60
| sa.Column( | ||
| "created_at", | ||
| sa.DateTime(timezone=True), | ||
| nullable=True, | ||
| server_default=sa.func.now(), | ||
| ), | ||
| sa.Column( | ||
| "updated_at", | ||
| sa.DateTime(timezone=True), | ||
| nullable=True, | ||
| server_default=sa.func.now(), | ||
| ), |
There was a problem hiding this comment.
Nullability mismatch: created_at and updated_at
Migration defines created_at and updated_at with nullable=True, but the ORM model (MilestoneTable) defines them with nullable=False. This inconsistency will cause constraint violations when the ORM attempts inserts without explicit timestamps, or schema validation failures.
Fix nullability to match ORM
sa.Column(
"created_at",
sa.DateTime(timezone=True),
- nullable=True,
+ nullable=False,
server_default=sa.func.now(),
),
sa.Column(
"updated_at",
sa.DateTime(timezone=True),
- nullable=True,
+ nullable=False,
server_default=sa.func.now(),
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/alembic/versions/003_add_milestones_table.py` around lines 49 - 60,
The migration defines created_at and updated_at as nullable=True but the ORM
model MilestoneTable expects non-nullable timestamps; update the Alembic
migration to make the columns non-nullable (set nullable=False for "created_at"
and "updated_at" columns in the migration) so the schema matches MilestoneTable
and avoids insert/constraint mismatches, keeping the
server_default=sa.func.now() intact to populate values when omitted.
Comment on lines
+63
to
+72
| op.create_index( | ||
| "ix_bounties_milestones_bounty_id", | ||
| "bounties_milestones", | ||
| ["bounty_id"], | ||
| ) | ||
| op.create_index( | ||
| "ix_bounties_milestones_status", | ||
| "bounties_milestones", | ||
| ["status"], | ||
| ) |
There was a problem hiding this comment.
Missing unique composite index on (bounty_id, milestone_number)
The ORM model defines a unique index ix_bmilestone_bounty_num on (bounty_id, milestone_number) to enforce that milestone numbers are unique per bounty. This index is missing from the migration, which will cause duplicate milestone numbers to be allowed and break milestone ordering logic.
Add the missing unique index
op.create_index(
"ix_bounties_milestones_status",
"bounties_milestones",
["status"],
)
+ op.create_index(
+ "ix_bmilestone_bounty_num",
+ "bounties_milestones",
+ ["bounty_id", "milestone_number"],
+ unique=True,
+ )And in downgrade:
def downgrade() -> None:
"""Drop the bounties_milestones table."""
+ op.drop_index("ix_bmilestone_bounty_num", table_name="bounties_milestones")
op.drop_index("ix_bounties_milestones_status", table_name="bounties_milestones")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| op.create_index( | |
| "ix_bounties_milestones_bounty_id", | |
| "bounties_milestones", | |
| ["bounty_id"], | |
| ) | |
| op.create_index( | |
| "ix_bounties_milestones_status", | |
| "bounties_milestones", | |
| ["status"], | |
| ) | |
| op.create_index( | |
| "ix_bounties_milestones_bounty_id", | |
| "bounties_milestones", | |
| ["bounty_id"], | |
| ) | |
| op.create_index( | |
| "ix_bounties_milestones_status", | |
| "bounties_milestones", | |
| ["status"], | |
| ) | |
| op.create_index( | |
| "ix_bmilestone_bounty_num", | |
| "bounties_milestones", | |
| ["bounty_id", "milestone_number"], | |
| unique=True, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/alembic/versions/003_add_milestones_table.py` around lines 63 - 72,
Add the missing unique composite index ix_bmilestone_bounty_num on the
bounties_milestones table to enforce uniqueness of (bounty_id,
milestone_number): in the migration's upgrade add an op.create_index call
creating ix_bmilestone_bounty_num on columns ["bounty_id", "milestone_number"]
with unique=True (alongside the existing ix_bounties_milestones_* indexes), and
in the downgrade add the corresponding op.drop_index("ix_bmilestone_bounty_num")
to remove it; locate these changes in the migration functions
(upgrade/downgrade) of 003_add_milestones_table.py.
Comment on lines
+798
to
+816
| response_model=List[MilestoneResponse], | ||
| status_code=status.HTTP_201_CREATED, | ||
| summary="Define milestones for a bounty", | ||
| ) | ||
| async def create_milestones( | ||
| bounty_id: str, | ||
| data: List[MilestoneCreate], | ||
| db: AsyncSession = Depends(get_db), | ||
| user: UserResponse = Depends(get_current_user), | ||
| ) -> List[MilestoneResponse]: | ||
| """Register milestone checkpoints for a T3 bounty. Total must be 100%.""" | ||
| svc = MilestoneService(db) | ||
| try: | ||
| user_id = user.wallet_address or str(user.id) | ||
| return await svc.create_milestones(bounty_id, data, user_id) | ||
| except (ValueError, UnauthorizedDisputeAccessError) as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| except BountyNotFoundError as e: | ||
| raise HTTPException(status_code=404, detail=str(e)) |
There was a problem hiding this comment.
Missing import for List type annotation.
List[MilestoneResponse] is used at lines 798 and 807 but List is not imported from typing (line 8 only imports Optional). This will cause a NameError at runtime.
🐛 Proposed fix at line 8
-from typing import Optional
+from typing import List, Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/bounties.py` around lines 798 - 816, The code uses the typing
name List in the create_milestones endpoint
(response_model=List[MilestoneResponse] and parameter data:
List[MilestoneCreate]) but List is not imported; add the missing import from
typing (e.g. from typing import List) near other imports so List is defined for
the create_milestones function and related type annotations (MilestoneResponse,
MilestoneCreate) resolve correctly.
Comment on lines
+813
to
+816
| except (ValueError, UnauthorizedDisputeAccessError) as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| except BountyNotFoundError as e: | ||
| raise HTTPException(status_code=404, detail=str(e)) |
There was a problem hiding this comment.
Wrong exception type caught - UnauthorizedDisputeAccessError instead of UnauthorizedMilestoneAccessError.
The milestone endpoints catch UnauthorizedDisputeAccessError but MilestoneService raises UnauthorizedMilestoneAccessError. This means unauthorized access attempts will result in unhandled 500 errors instead of proper 400/403 responses.
🐛 Proposed fix - update exception handling
+from app.exceptions import (
+ BountyNotFoundError,
+ UnauthorizedMilestoneAccessError,
+ MilestoneValidationError,
+ MilestoneSequenceError,
+ MilestoneNotFoundError,
+)
# In each endpoint:
- except (ValueError, UnauthorizedDisputeAccessError) as e:
+ except (ValueError, UnauthorizedMilestoneAccessError, MilestoneValidationError, MilestoneSequenceError) as e:
raise HTTPException(status_code=400, detail=str(e))
+ except MilestoneNotFoundError as e:
+ raise HTTPException(status_code=404, detail=str(e))
except BountyNotFoundError as e:
raise HTTPException(status_code=404, detail=str(e))Also applies to: 836-839, 858-861
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/bounties.py` around lines 813 - 816, Replace the incorrect
exception type UnauthorizedDisputeAccessError with
UnauthorizedMilestoneAccessError in the milestone endpoint except blocks so
MilestoneService's raises are caught; update the three occurrences (the except
blocks currently grouping ValueError and UnauthorizedDisputeAccessError around
the MilestoneService calls) to catch UnauthorizedMilestoneAccessError and
re-raise an appropriate HTTPException (400 or 403 as intended) with the error
message. Ensure the change is applied to all three locations noted so
unauthorized milestone access no longer produces an unhandled 500.
Comment on lines
+46
to
+51
| <div className="relative h-4 w-full bg-gray-800 rounded-full overflow-hidden"> | ||
| <div | ||
| className="absolute top-0 left-0 h-full bg-gradient-to-r from-purple-600 to-blue-500 transition-all duration-500 ease-out" | ||
| style={{ width: `${(approvedPercentage / totalPercentage) * 100}%` }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Potential division by zero in progress bar width
If all milestones have percentage: 0, then totalPercentage would be 0, causing (approvedPercentage / totalPercentage) * 100 to produce NaN. While business logic should prevent this, defensive coding is recommended.
Add zero check
<div
className="absolute top-0 left-0 h-full bg-gradient-to-r from-purple-600 to-blue-500 transition-all duration-500 ease-out"
- style={{ width: `${(approvedPercentage / totalPercentage) * 100}%` }}
+ style={{ width: `${totalPercentage > 0 ? (approvedPercentage / totalPercentage) * 100 : 0}%` }}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="relative h-4 w-full bg-gray-800 rounded-full overflow-hidden"> | |
| <div | |
| className="absolute top-0 left-0 h-full bg-gradient-to-r from-purple-600 to-blue-500 transition-all duration-500 ease-out" | |
| style={{ width: `${(approvedPercentage / totalPercentage) * 100}%` }} | |
| /> | |
| </div> | |
| <div className="relative h-4 w-full bg-gray-800 rounded-full overflow-hidden"> | |
| <div | |
| className="absolute top-0 left-0 h-full bg-gradient-to-r from-purple-600 to-blue-500 transition-all duration-500 ease-out" | |
| style={{ width: `${totalPercentage > 0 ? (approvedPercentage / totalPercentage) * 100 : 0}%` }} | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/bounties/MilestoneProgress.tsx` around lines 46 - 51,
The inline style for the progress width can produce NaN when totalPercentage is
0; update the calculation used in the style on the inner div (where width:
`${(approvedPercentage / totalPercentage) * 100}%`` is set) to defensively
handle totalPercentage === 0 by using a conditional/fallback (e.g., set width to
"0%" or compute percentage as 0 when totalPercentage is zero), ensuring the
component (MilestoneProgress) never sets an invalid CSS width.
| </div> | ||
|
|
||
| <div className="space-y-4"> | ||
| {milestones.sort((a, b) => a.milestone_number - b.milestone_number).map((milestone) => ( |
There was a problem hiding this comment.
Array mutation: .sort() modifies props directly
milestones.sort() mutates the original array, which can cause React rendering issues and violates the principle of treating props as immutable. Use a non-mutating sort instead.
Use spread or toSorted() for immutable sorting
- {milestones.sort((a, b) => a.milestone_number - b.milestone_number).map((milestone) => (
+ {[...milestones].sort((a, b) => a.milestone_number - b.milestone_number).map((milestone) => (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {milestones.sort((a, b) => a.milestone_number - b.milestone_number).map((milestone) => ( | |
| {[...milestones].sort((a, b) => a.milestone_number - b.milestone_number).map((milestone) => ( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/bounties/MilestoneProgress.tsx` at line 54, The code
currently calls milestones.sort(...) which mutates the incoming milestones prop;
update the render in MilestoneProgress.tsx to sort without mutation by creating
a sorted copy (e.g., use a shallow copy via spread or Array.prototype.toSorted)
before calling .map, ensuring the original milestones prop is not modified;
locate the expression with milestones.sort((a, b) => a.milestone_number -
b.milestone_number).map(...) and replace it with a non-mutating sorted copy
followed by .map to preserve props immutability and prevent React rendering
issues.
Comment on lines
+91
to
+109
| <div className="flex items-center gap-2"> | ||
| {milestone.status === 'pending' && !isCreator && onSubmit && ( | ||
| <button | ||
| onClick={() => onSubmit(milestone.id)} | ||
| disabled={loading} | ||
| className="px-3 py-1 text-xs bg-purple-600 hover:bg-purple-700 text-white rounded transition-colors disabled:opacity-50" | ||
| > | ||
| Submit | ||
| </button> | ||
| )} | ||
| {milestone.status === 'submitted' && isCreator && onApprove && ( | ||
| <button | ||
| onClick={() => onApprove(milestone.id)} | ||
| disabled={loading} | ||
| className="px-3 py-1 text-xs bg-emerald-600 hover:bg-emerald-700 text-white rounded transition-colors disabled:opacity-50" | ||
| > | ||
| Approve & Pay | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sequential milestone submission not enforced in UI
The UI allows submitting any pending milestone without checking if previous milestones are approved. While the backend enforces sequence, users may get confusing errors. Consider disabling submit buttons for milestones where the previous one isn't approved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/bounties/MilestoneProgress.tsx` around lines 91 -
109, The Submit button currently renders whenever milestone.status === 'pending'
regardless of prior milestones; update MilestoneProgress.tsx to prevent
out-of-order submissions by requiring the previous milestone be approved before
enabling submit: either add a prop (e.g., previousMilestoneApproved: boolean) or
accept (milestones: Milestone[], index: number) and compute const canSubmit =
milestone.status === 'pending' && previousMilestoneApproved; then use canSubmit
in the conditional and as the disabled prop on the Submit button (and optionally
show a disabled tooltip/explain). Ensure you reference the existing onSubmit,
loading, milestone.status and isCreator logic so only the intended Submit button
is affected.
Comment on lines
936
to
+937
| break; | ||
| break; |
There was a problem hiding this comment.
Duplicate break statement - syntax error or dead code.
There are two consecutive break statements at lines 936-937. This appears to be a merge artifact. The first break terminates case 3, making the second unreachable.
🐛 Proposed fix
case 3:
if (!formData.requirements.some((r) => r.trim())) {
newErrors.requirements = 'At least one requirement is required';
}
break;
- break;
case 4:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| break; | |
| break; | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/BountyCreationWizard.tsx` around lines 936 - 937, In
the switch statement inside BountyCreationWizard (the switch handling the wizard
steps / case 3), remove the duplicate consecutive `break` so only a single
`break` terminates case 3; this eliminates the unreachable/dead `break` that
appears to be a merge artifact and prevents the syntax/dead-code issue.
Comment on lines
+943
to
+944
| const totalPerc = formData.milestones.reduce((acc, m) => acc + m.percentage, 0); | ||
| if (Math.abs(totalPerc - 100) > 0.01) { |
There was a problem hiding this comment.
Frontend/backend tolerance mismatch for percentage validation.
Frontend uses 0.01 tolerance (line 944: Math.abs(totalPerc - 100) > 0.01) while backend uses 0.001 tolerance (bounty_service.py line 326). A user could pass frontend validation with percentages summing to 99.995 but fail backend validation.
Align tolerances or use exact comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/BountyCreationWizard.tsx` around lines 943 - 944,
Frontend validation in BountyCreationWizard.tsx computes totalPerc from
formData.milestones and uses a 0.01 tolerance (Math.abs(totalPerc - 100) > 0.01)
which mismatches the backend's 0.001 tolerance; change the frontend check to use
the same tolerance (0.001) or perform an exact normalized check so values that
pass the UI cannot fail the backend (replace 0.01 with 0.001 in the
Math.abs(totalPerc - 100) comparison or implement equivalent normalization
before comparison).
Contributor
Author
|
@chronoeth-creator Multi-LLM review did not run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implement milestone-based payouts for T3 bounties to enable partial payments at defined checkpoints throughout large-scale projects. This PR introduces the core models, backend services, API endpoints, and frontend components required for milestone management and automated reward distribution.
Closes #494
Solana Wallet for Payout
Wallet: 4QhseKvBuaCQhdkP248iXoUxohPzVC5m8pE9hAv4nMYw
Type of Change
Checklist
console.logor debugging code left behindTesting
Screenshots (if applicable)
Additional Notes
payout_serviceusing the milestone's defined percentage of the total bounty reward.003_milestonesfor the newbounties_milestonestable.