feat: add NFT Holder voter groups for Flow Councils#348
Conversation
…llback and UI fixes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
tnrdd
changed the title
![gaston-review[bot]](https://avatars.githubusercontent.com/in/3010035?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils—a full feature spanning database schema, three new API routes (probe, status, claim), admin CRUD integration, a voter-facing eligibility modal, and comprehensive docs. The claim flow verifies a wallet signature, checks NFT holdings via multicall, grants the highest eligible allocation on-chain through the Flow State bot, and handles rollback on failure. It enforces mutual exclusivity with GoodDollar groups, prevents ERC-20 contracts from being mistaken for NFT collections, and adds a council-scoped rate limit to protect the bot's nonce.
Review Summary
🟡 Warning — getVoter failure allows claim to proceed without knowing current voting power
When the on-chain getVoter read fails, evaluation.votingPower is null, and the claim route skips the already-voter guard. This is handled gracefully by the ALREADY_ADDED revert catch, but it means a transient RPC failure could cause a claim attempt (with gas cost) for a wallet that already has votes. The design is defensible—failing closed would prevent all claims during an RPC blip—but worth documenting as a known tradeoff.
🔵 Suggestion — Bot address drift guard removed with no replacement
The test in constants.test.ts that validated FLOW_STATE_BOT_ADDRESS matches the derived address from FLOW_STATE_ELIGIBILITY_PK was deleted. With the bot now spending gas on four additional chains, a mismatch would silently break all NFT claims. A startup assertion in buildBotSigner would catch this in every environment that configures the key, without depending on test infrastructure.
This is one of the better large-feature PRs I've seen. The security surface is carefully thought through: ERC-20 rejection, factory verification before spending gas, EOA vs. contract signature verification, gas cap on the addVoter call, and distinct error codes for every refusal path. The test coverage is excellent—over 2,500 lines of tests covering happy paths, edge cases, rollback scenarios, and error hygiene. The rollback logic correctly distinguishes broadcast from non-broadcast failures. I have two substantive observations (getVoter failure behavior and the removed drift guard) that are worth discussing but neither blocks the merge. Production-ready.
📌 7 inline comments
🟡 Warning: 1 · 🔵 Suggestion: 3 · ⚪ Nitpick: 3
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils: admins can point a group at an ERC-721 or ERC-1155 collection, and holders self-claim votes via a signed message. It spans the full stack: migration (five new columns + a rate-limit slot + two partial unique indexes), detection/override logic for contract classification, three new API routes (nft-probe, nft-status, nft-claim), Zod validation, frontend admin fields, an eligibility popup, shared claim-message builder, factory-council origin verification at registration, and comprehensive test coverage (~2500 lines of integration tests). GoodDollar/NFT method exclusivity is enforced per council, the bot signer is now shared, and the public voter-groups endpoint gains an includeMembers=0 option.
Review Summary
🟡 Warning — Unbounded factoryCouncils cache grows forever in long-lived processes
The factoryCouncils Set in auth.ts only adds entries and never evicts. In a long-running Next.js server process, every unique council that passes the check stays in memory permanently. With thousands of councils over months this becomes a leak. Consider an LRU cache with a size cap (e.g. 500 entries) or a TTL. The risk is low today but worth a follow-up.
🟡 Warning — Claim route treats null botHasRole as 'has role'
In the nft-claim route, evaluation.botHasRole is checked with === false, so a null value (meaning the hasRole read failed) is silently treated as permission granted. The claim will likely fail later at the on-chain addVoter call and return chain_error, but it burns the rate-limit window and attempts a transaction first. An explicit guard for botHasRole === null returning check_unavailable would be more defensive.
🔵 Suggestion — Deleted bot-address drift guard test without replacement
The constants.test.ts file was deleted, which contained a guard that verified FLOW_STATE_BOT_ADDRESS matches the address derived from FLOW_STATE_ELIGIBILITY_PK. The PR commit message says 'drop the dormant bot-address drift guard', but the guard was conditional (runIf), not dormant — it ran wherever the env var was set. Consider keeping it or noting why it's no longer needed.
This is a well-structured, security-conscious feature. The separation of read-only status from write claim, the careful rollback semantics, the factory-council verification before spending gas, the ERC-20 rejection during override, and the refusal to treat a failed read as "not eligible" all show thoughtful design. Test coverage is thorough. The two things I'd address before merge are the unbounded factory-council cache and the claim route's missing null guard on botHasRole, but neither is a showstopper — the cache grows slowly in practice and the null case degrades safely (the claim will fail at the on-chain call). Strong work overall.
📌 6 inline comments
🟡 Warning: 2 · 🔵 Suggestion: 3 · ⚪ Nitpick: 1
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: █████████░ 9/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils as a full-stack feature: database migration (five new columns, a rate-limit slot, two partial unique indexes with IF NOT EXISTS guards), three new API routes (nft-probe for admin detection, nft-status for read-only eligibility checks, nft-claim for on-chain vote assignment), admin CRUD integration with server-side re-probing, a voter-facing eligibility modal, shared claim-message builder, factory-council origin verification at registration, and around 2,500 lines of integration tests. The claim flow verifies a wallet signature, checks NFT holdings via multicall, grants the highest eligible allocation on-chain through the Flow State bot, and handles rollback correctly distinguishing broadcast from non-broadcast failures. It enforces mutual exclusivity with GoodDollar groups and prevents ERC-20 contracts from being mistaken for NFT collections.
Review Summary
🔵 Suggestion — nft-status outer catch returns 200 while nft-claim returns 500
The claim route was updated to return HTTP 500 for genuinely unexpected errors, with a comment explaining the intent. The nft-status route's outer catch still returns HTTP 200 with success: false. Both routes were written in this PR, so this is the right time to align them. A status-code monitor that watches for 5xx will miss server errors on the status route.
🔵 Suggestion — Prior review items have been addressed
The switchesNftMethod member-count check now runs inside the lockCouncilGroups transaction block (line 749 of voter-groups route.ts), addressing the race condition flagged in the prior review. The claim route's outer catch now returns 500 as suggested. The getVoter null-power behavior and the removed bot-address drift guard remain documented tradeoffs from the prior review and don't need re-flagging.
This is an exceptional large-feature PR. The security surface is carefully handled: ERC-20 rejection via decimals()/allowance() probes, factory verification before spending gas, EOA vs. contract signature verification with the on-chain path reserved for contract accounts only, gas cap on addVoter, council-scoped rate limiting, and distinct error codes for every refusal path. The rollback logic correctly distinguishes pre-broadcast from post-broadcast failures and handles the ALREADY_ADDED revert gracefully. The concurrency model is solid—lockCouncilGroups acquires a FOR UPDATE lock on the rounds row (not just the group rows) to prevent two concurrent first-creates from racing past the exclusivity check. The prior review's suggestion about moving the switchesNftMethod check inside the transaction has been addressed. The only remaining nit is the nft-status outer catch returning HTTP 200 instead of 500, which I flagged as a suggestion since the claim route was already fixed. Production-ready.
📌 2 inline comments
🔵 Suggestion: 2
🔍 Reviewed by Gaston
…lign status codes
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils: schema changes (five new columns, a rate-limit slot, two partial unique indexes), three API routes (probe, status, claim), admin CRUD integration with mutual exclusivity against GoodDollar groups, a voter eligibility popup, factory-origin verification at registration, and ~2500 lines of integration tests. Since the prior two approvals, the latest commits address every substantive issue I raised: the outer catch now returns 500, the factory council cache is bounded to 500 entries, the method-switch member-count check runs inside the transaction lock, and unknown bot role now refuses the claim rather than proceeding.
Review Summary
🟡 Warning — ALREADY_ADDED response missing votingPower renders NaN in the modal
The ALREADY_ADDED catch returns { success: true, alreadyVoter: true } without a votingPower field. The client calls Number(data.votingPower) on every success, producing NaN, which then appears in the UI as "You have NaN votes". The race is narrow (getVoter read returned 0 or failed, then addVoter reverted ALREADY_ADDED between the read and the write), but it's a real rendering bug.
Every issue from my prior two reviews has been addressed in the latest commits. I found one new minor bug: the ALREADY_ADDED catch path omits votingPower, which would render NaN in the modal during a narrow race condition. The rest of the code is solid—security surface is carefully designed (ERC-20 rejection, factory verification, gas cap, distinct error codes), rollback logic correctly distinguishes broadcast vs. non-broadcast failures, and the test coverage is excellent. Production-ready with that one edge-case fix.
📌 1 inline comment
🟡 Warning: 1
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: █████████░ 9/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils — the full stack from database migration through three API routes (probe, status, claim), admin CRUD with mutual exclusivity enforcement, a voter-facing eligibility modal, factory-origin verification at registration, shared bot signer, and ~2,500 lines of integration tests.
Every concern from my prior reviews has been addressed in the latest commits: the outer catch now returns 500, the factory council cache is bounded at 500 entries with FIFO eviction, unknown bot role returns check_unavailable instead of proceeding, and the method-switch member count check runs inside the lockCouncilGroups transaction. The security surface remains carefully handled (ERC-20 rejection, factory verification before gas spend, EOA vs contract signature paths, gas cap on addVoter, distinct error codes). The rollback logic correctly distinguishes broadcast from non-broadcast failures. Production-ready — one of the best large-feature PRs I've reviewed.
🔍 Reviewed by Gaston
There was a problem hiding this comment.
Gaston Review
Verdict: Approved
Score: ████████░░ 8/10
Pull Request Summary
This PR adds NFT Holder voter groups to Flow Councils—a full-stack feature spanning a database migration, three new API routes (nft-probe, nft-status, nft-claim), admin CRUD integration with mutual exclusivity enforcement, a voter eligibility modal with tier-upgrade support, factory-council origin verification, and ~2,500 lines of integration tests. The claim flow verifies a wallet signature, checks NFT holdings via multicall, selects the highest eligible tier, grants votes on-chain through the bot, and handles rollback with correct broadcast-vs-non-broadcast distinction.
Review Summary
🔵 Suggestion — Public endpoint still joins all members when includeMembers=0
The includeMembers=0 query parameter was added so the eligibility popup can fetch only group metadata. However, the DB query still left-joins voterGroupMembers and selects every address—only the JSON response drops the array. On a council with thousands of voters, the database work and row transfer are the expensive part, not the JSON serialization. Branching the query to skip the join when members aren't needed would deliver the intended performance benefit.
This is still one of the best large-feature PRs I've reviewed. All the concerns from my prior two reviews have been addressed in subsequent commits: the outer catch now returns 500, the factory council cache is bounded, the bot role check prevents wasted gas, the method-switch member count check moved inside the transaction, and the recheck/tier-upgrade path is cleanly integrated. The one substantive observation remaining is that includeMembers=0 on the public route saves payload but not DB work—a future optimization, not a blocker. The security surface is carefully handled: ERC-20 rejection, factory verification before spending gas, EOA vs contract signature routing, explicit gas cap, and council-scoped rate limiting with correct release semantics. Production-ready.
📌 4 inline comments
🔵 Suggestion: 2 · ⚪ Nitpick: 2
🔍 Reviewed by Gaston
| @@ -46,29 +62,45 @@ export async function GET(request: Request) { | |||
| "voterGroups.id as groupId", | |||
There was a problem hiding this comment.
🔵 Suggestion: When includeMembers=0, the left join on voterGroupMembers still runs and fetches every member address from the DB—only the JSON response drops them. On a council with thousands of voters this is the expensive part. Consider branching the query: when includeMembers is false, skip the join entirely and select only from voterGroups. The payload benefit is real, but the DB work is unchanged right now.
| if (!broadcast) { | ||
| await releaseRateWindow(round.id, previousLastClaimAt, claimedAt); | ||
| } | ||
|
|
There was a problem hiding this comment.
⚪ Nitpick: The ALREADY_ADDED handler when !broadcast returns success with groupId and groupName but no votingPower. The client handles this gracefully via Number.isFinite, but the two ALREADY_ADDED return shapes (this one at line 271 without votingPower, and the post-revert one at line 305 with it) are inconsistent. The client already has fallback logic, so this isn't a bug—just a slightly uneven API surface.
|
|
||
| if (!address || !chainId || !councilId) { | ||
| return Response.json({ success: false, error: "Invalid request" }); | ||
| } |
There was a problem hiding this comment.
🔵 Suggestion: The body is parsed with await request.json() without a try-catch of its own. A malformed JSON body will throw and land in the outer catch, returning a generic 500. This is fine functionally (the outer catch handles it), but it means a client sending garbage JSON sees a 500 rather than a 400. A targeted catch around request.json() could return a more specific status code, matching how the other routes handle parse errors.
| setState("claim-error"); | ||
| } finally { | ||
| claimInFlightRef.current = false; | ||
| } |
There was a problem hiding this comment.
⚪ Nitpick: After a successful claim, grantedVotes is set from the response but the councilMember context isn't refreshed. The modal's loadStatus() re-reads on next open, so it self-corrects. Noted in the prior review and remains a very minor UX lag—not a blocker, just worth being aware of.
1 participant
Adds NFT Holder as a fourth voter-group eligibility method, plus an eligibility requirements popup so voters can see what a council actually requires instead of a black-box "Check Voter Eligibility" button.
Communities with a membership NFT can now point a council at that collection and let holders claim their own votes, with tiering across collections (core contributor NFT worth 20, community NFT worth 5). We supported NFT gating in the old SQF product, so this is a capability we lost rather than a new idea.
Spec:
.claude/specs/nft-voter-group.mdWhat changed
Admin. "NFT Holder" in the voter-group create modal and group editor. Paste a collection address, the standard is auto-detected (ERC-721 / ERC-1155), 1155 requires a token ID, and you set the vote allocation plus an optional label and "where to get this NFT" link. A council uses one automated method: GoodDollar and NFT gating are mutually exclusive, enforced in both directions in the API and the UI.
Voter. On NFT-gated councils the button opens a popup listing every way to earn votes, what each is worth, and whether the connected wallet meets it. Claiming takes a signature (no gas, no session) and grants the largest single allocation, never the sum.
Not touched. GoodDollar, manual and metrics councils. See the criterion 17 evidence below.
New endpoints
POST /api/flow-council/voter-groups/nft-probe(council-manager auth) - standard detection and manual-override verificationPOST /api/flow-council/eligibility/nft-status- read-only requirement evaluationPOST /api/flow-council/eligibility/nft-claim- signature-gated claimDocumented in
docs/developers/006-nft-eligibility-api.md, with operator and voter docs updated underdocs/platform/flow-councils/.Success criteria
All 17 walked against the implementation.
Criterion 17 evidence.
git diff main...HEADis empty for bothsrc/app/api/flow-council/eligibility/route.tsandsrc/app/flow-councils/components/EligibilityButton.tsx. The NFT popup is a wrapper at the mount point that falls through to the existing button, so those councils run exactly the code they ran before. The shared surfaces were re-checked too:bot.tsis behavior-neutral for GoodDollar and metrics, and the public voter-groups additions are additive only.Criterion 15 evidence. True by absence, confirmed by search rather than argument: no
removeVotercall anywhere outside an admin-path comment, no cron or scheduled routes, and the requirement evaluator is reachable only from the two voter-initiated eligibility routes. Nothing re-evaluates or revokes after a grant.Closes a pre-existing registration hole
POST /api/flow-council/launchverifiedhasRole(DEFAULT_ADMIN_ROLE, caller)by calling the candidate contract itself, which a contract can simply answertrueto. The real launch flow deploys through the factory (launch.tsx:256) and then posts the resulting address (launch.tsx:314), but nothing bound the second step to the first, so anyone posting directly could register a contract they wrote.That was harmless-ish until now. NFT gating made it exploitable for real money, because the attacker also controls the
balanceOfthat decides eligibility. It is not capped by the GoodDollar identity requirement either:eligibility/route.tsskipsaddVoteronly when a membership row already exists for(roundId, address), androundIddiffers per council, so unlimited fake councils give one verified identity unlimited uncappedaddVotercalls paid by the bot. Draining the Celo bot would take down real GoodDollar claims and metrics ballots.Fixed in two places:
/launchnow requires the address to be a council the factory actually deployed, verified via the subgraph. Legitimate launches pass unchanged.addVotergas.Migration
20260721000000_add_nft_voter_groupadds the NFT columns onvoter_groups,rounds.last_claim_at, and two unique indexes: one NFT group per (council, collection, token ID) and one metrics group per council, both DB-level backstops for check-then-insert races.It also creates
voter_groups_round_metrics_unique, which20260617000000_add_metrics_voteris recorded as having applied but which exists in no environment. Recreated here rather than by editing that migration, whose checksum is already recorded. Verified zero duplicate metrics groups in production and on the test branch first.Already applied to production, ahead of the merge. Every statement is additive and
IF NOT EXISTSguarded, so the current deploy is unaffected.Decisions worth flagging
decimals()/allowance()and requires a real decodingbalanceOf.nonceManager. It was added during implementation and removed during review: it consumes a nonce before signing and never returns it, so one failed broadcast gaps every later transaction, and its cache is a module singleton keyed by address, which silently reached the GoodDollar and metrics paths.nftConfigleaves the stored config untouched rather than rejecting, so a rename need not resend it. The invariant criterion 4 protects is "never partial-merge", which holds either way.Known limitations
nft-statusis anonymous and unthrottled. Read-only and spends no gas, but worth an IP rate limit and a short cache.ALREADY_ADDEDand is handled as success.includeMembers=0, so it no longer ships every voter address.Testing
pnpm lint,pnpm typecheck,pnpm prettierclean. 502 unit and 296 integration tests passing.Tests were written before the implementation, from the spec. Two were changed afterwards, both deliberately: one asserted rollback on a broadcast transaction, reversed above; the other hardcoded an instant that only fell inside the signature freshness window on one specific day and expected a millisecond precision the signed message deliberately omits.
Still needs manual smoke: a real Safe completing a claim (criterion 7). Automated coverage asserts that a contract account takes the on-chain verifier path, which is the mechanism ERC-1271 depends on, but only a real Safe on a testnet proves it end to end.