chore: update flow council factory and subgraph to v0.4.1 on Celo#268
chore: update flow council factory and subgraph to v0.4.1 on Celo#268
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
gravenp
left a comment
There was a problem hiding this comment.
@tnrdd, to functionally test the new contracts & subgraph, I think we need to make a couple of additional changes in this PR:
- Revert #249
- Update the way that a ballot with a removed grantee is displayed & resubmitted
When testing a local branch with PR249 reverted, my ballot looked like the image below (on /flow-councils/42220/0x30fc88832401dfd6203cb13fa48bff872c3bf0b6):
On that ballot UI, there's no way to successfully "submit 0 votes" for that removed grantee. Any attempt to vote the full allocation for the remaining grantees fails. Based on our past conversations, I don't think we can return all voters' votes at the time of graduation/removal. But when they return to the UI for their next ballot, we want it to appear and function like that:
- "Stale" votes should be excluded from their voting power used numerator and their ballot display, but remain in the denominator as available
- When they submit their next ballot, we should remove any votes for graduated/removed grantees so they can be added to others
Filter removed recipients in council query using subgraph v0.4.1 soft-delete field. For GoodDollar council, keep reducing voting power by stale amount. For other councils, full voting power remains available.
There was a problem hiding this comment.
🔄 Changes Requested
Score: █████░░░░░ 5/10
The PR replaces contract-based stale vote detection with subgraph-based detection (leveraging the new removed field in v0.4.1) and updates the factory/subgraph addresses. The approach is sound — moving stale vote detection to the subgraph is cleaner and avoids multiple contract calls. However, there's one significant issue:
Hardcoded council address: GOODDOLLAR_COUNCIL_ADDRESS is hardcoded inline as a string literal in FlowCouncil.tsx, but the exact same address already exists in src/app/flow-councils/lib/constants.ts as GOODBUILDERS_COUNCIL_ADDRESSES[1]. This creates a maintenance risk — if the address changes in one place but not the other, the stale vote logic silently breaks.
Stale vote filtering is council-specific but shouldn't be: The staleVotesList and filteredCurrentBallot computations apply to all councils (good), but the voting power adjustment only applies to the GoodDollar council. The commit message says "detect stale votes from subgraph instead of contract ABI" — but the old code applied to all councils, not just GoodDollar. This is a behavior change that should be intentional and documented.
The subgraph/factory address updates and the recipients(where: { removed: false }) filter look correct.
📌 2 inline comments
🔍 Reviewed by Gaston · Model: claude-opus-4-6
FlowCouncilContextProvider wraps the entire app but councilId is only present on flow-council routes. The stale votes refactor called councilId.toLowerCase() unconditionally, crashing every other page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🔄 Changes Requested
Score: ██████░░░░ 6/10
Second pass — both prior concerns remain unaddressed with no author response:
-
Hardcoded address:
GOODDOLLAR_COUNCIL_ADDRESSon line 218 duplicatesGOODBUILDERS_COUNCIL_ADDRESSES[1]fromconstants.ts. This is already imported in three other files. Duplicating it inline is a maintenance hazard. -
Voting power adjustment scoped to GoodDollar only: The old code (deleted
staleVotesQuery.ts) adjusted voting power for all councils. The new code gates it behindisGoodDollarCouncil. If this is intentional, a comment explaining why is warranted. If not, it's a regression.
The core approach — replacing contract reads with subgraph filtering via removed: false and client-side stale vote detection — is solid. The staleVotesList / filteredCurrentBallot / councilMember memos are well-structured. Factory and subgraph URL updates look correct.
📌 2 inline comments
🔍 Reviewed by Gaston · Model: claude-opus-4-6
src/context/FlowCouncil.tsx
Outdated
| [councilMemberRaw, staleVotes, isLoadingStaleVotes], | ||
| ); | ||
| const GOODDOLLAR_COUNCIL_ADDRESS = | ||
| "0xfabef1abae4998146e8a8422813eb787caa26ec2"; |
There was a problem hiding this comment.
Re-flagging from prior review (unaddressed, code unchanged): this address is already defined as GOODBUILDERS_COUNCIL_ADDRESSES[1] in src/app/flow-councils/lib/constants.ts and imported in three other files (EligibilityButton.tsx, s2StreamMigration.ts, eligibility/route.ts). Import and reuse it:
import { GOODBUILDERS_COUNCIL_ADDRESSES } from "@/app/flow-councils/lib/constants";
// ...
const isGoodDollarCouncil =
councilId?.toLowerCase() === GOODBUILDERS_COUNCIL_ADDRESSES[1];Duplicate magic strings rot silently — if the constant is updated but this literal isn't, stale vote deduction breaks without any compile-time signal.
Wrap useBallotQuery's filter/map in useMemo to stabilize object references. Without this, every provider render created new arrays, cascading through staleVotesList → councilMember → newBallot dispatch, causing a render loop that broke the VoteBubble pie chart and click handler. Also replace inline GoodDollar address with GOODBUILDERS_COUNCIL_ADDRESSES constant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🔄 Changes Requested
Score: ██████░░░░ 6/10
Third pass. The hardcoded address concern from prior reviews has been addressed — now using the shared GOODBUILDERS_COUNCIL_ADDRESSES[1] constant. The useMemo addition to ballotQuery.ts is correct and well-motivated. The subgraph/factory updates and removed: false filter look good.
One concern remains unaddressed after two prior reviews with no author response: the voting power stale-vote deduction is now scoped to GoodDollar only (line 250), whereas the old code applied it universally. This is either a silent regression for other councils or an intentional change that needs documentation.
📌 1 inline comment
🔍 Reviewed by Gaston · Model: claude-opus-4-6
Stale vote deduction on GoodBuildersThe switch from contract read ( Root causeThe contract's Example — voter
Both v0.3.7 and v0.4.1 subgraphs have this same gap. The old Options
@tnrdd what's the preferred approach? |
No description provided.