feat(governance): finalize membership lifecycle and runtime QA (#80)#99
Conversation
edehvictor
changed the title
Guard registration, unstake, and voting receipts by wallet scope and prevent duplicate submissions. Enforce current house stake thresholds and chain-time locks, expand real-adapter coverage, and stabilize screenshot evidence.
pheobeayo
left a comment
pheobeayo
left a comment
There was a problem hiding this comment.
Really solid work overall, the adapter split is a genuine composition boundary (781→278 lines with no forwarding files), the receipt/guard handling is careful, and the stale-state tests around account switching cover the hard cases. Requesting changes on three items only: the vote ID indexing assumption in voteStartTimeFromSchedule (needs confirmation against GoodProtocol #299 since it affects on-chain submission correctness), the zero-stake transferAndCall path in register, and the disabledReason clobbering in setVoteAllocation. The rest are non-blocking suggestions. Happy to re-review quickly once those land.
| currentBlockTime: number | null | ||
| } | ||
|
|
||
| export function voteStartTimeFromSchedule( |
There was a problem hiding this comment.
This derives the fallback vote start as cycleStartTime + voteId * termDuration, which assumes getCurrentVoteId is 0-indexed relative to the cycle start. Can you confirm against the GoodDaoHouses implementation in GoodProtocol #299 that vote IDs start at 0? If they're 1-based, the pre-snapshot provisional filtering below (L189–L212) computes the wrong window, one full term late and could submit recipients the contract will reject, or wrongly exclude eligible ones.
The unit test at adapter-logic.spec.ts L389 encodes the same assumption rather than verifying it, so it passes either way. If the contract exposes the current vote's start directly (or the indexing is confirmed), a one-line comment here citing the contract source would prevent this question resurfacing. Since voteConfig.startTime takes precedence when it exists, this only bites in the exact window the fallback was written for before the first ballot creates the snapshot which is why I'd like it nailed down before merge.
| return | ||
| } | ||
|
|
||
| const stakeAmountWei = membership?.minimumStakes[selectedHouse] ?? 0n |
There was a problem hiding this comment.
If the membership reads haven't landed yet (or were invalidated by an account switch), this falls back to 0n and prompts the wallet for a transferAndCall with zero stake, a guaranteed revert. Suggest an early return before transactionGuard.begin():
if (!membership) {
setState((previous) => ({
...previous,
error: 'Membership data is still loading. Please try again in a moment.',
}))
return
}
const stakeAmountWei = membership.minimumStakes[selectedHouse]
This keeps the "never send a transaction we know will fail" property the rest of the PR is careful about.
| disabledReason: allocationTotalBps === 10_000 && previous.canVote | ||
| ? undefined | ||
| : allocationTotalBps === 10_000 | ||
| ? previous.disabledReason | ||
| : 'Allocation totals must equal exactly 10,000 basis points.', | ||
| } |
There was a problem hiding this comment.
The stored disabledReason gets clobbered here. Sequence: an ineligible user (or any user pre-eligibility-check) edits an allocation with total ≠ 10,000 → the original reason ("Only active members can vote", "Verify your GoodID…", etc.) is overwritten with the bps message. When the total later reaches exactly 10,000 with canVote still false, the previous.disabledReason you retain is now the bps message; so the UI tells them the total is wrong when it isn't.
Since canVote and the eligibility reasons are already computed in createVotingState, I'd derive disabledReason at read time (eligibility reason first, then the bps check as a final overlay) rather than mutating it in state. That makes the reason a pure function of state and this class of ordering bug impossible.
| switchToCelo, | ||
| ], | ||
| ) | ||
| const transaction = membership.transaction.kind === 'unstake' |
There was a problem hiding this comment.
kind === 'unstake' wins unconditionally here, even when it's a stale confirmed from a previous lifecycle and voting.transaction is actively in wallet_confirmation. In practice the user is usually back in onboarding after an unstake so it rarely surfaces, but the priority should probably be "most recently active" rather than "unstake always wins." If #4 above lands (resetting to idle after refresh), this simplifies to preferring whichever transaction is non-idle, which reads more honestly.
| const provisionalRecords = await Promise.all( | ||
| activeAlignment.map(async (recipient) => ({ | ||
| recipient, | ||
| member: mapMemberRecord( | ||
| await publicClient.readContract({ | ||
| address: housesAddress, | ||
| abi: GOODDAO_HOUSES_ABI, | ||
| functionName: 'getMember', |
There was a problem hiding this comment.
Two observations, neither blocking:
This is N+1 getMember reads for every alignment member, and because useGovernanceVoting's refresh re-fires whenever the membership poll produces new object identities (~every 30s), it runs continuously while no snapshot exists. Fine at current HoA sizes, but a multicall batch would make it a single RPC round-trip and future-proof it.
The 30s cadence coupling itself is implicit; voting has no interval of its own and effectively polls by riding membership's identity churn through the schedule/member deps. Worth a one-line comment in useGovernanceVoting so nobody "fixes" the missing interval or memoizes the reads and silently kills vote-window updates.
| return 'failed' | ||
| } | ||
|
|
||
| export function statusFromMember(member: GovernanceMemberRecord | null): GovernanceWidgetStatus { |
There was a problem hiding this comment.
pending + citizenship falls through to onboarding_required. Is that state impossible by contract construction (citizenship activates immediately on registration)? If it can occur, a pending citizen would be shown the join flow and could attempt a second registration. If it's impossible, a short comment saying so would be enough, this function is the routing table for the whole widget, so silent fall-throughs deserve a note
|
Hi @pheobeayo , thank you for the detailed review. I addressed the three requested changes and the relevant non-blocking observations in signed commit cb85c4e. The review also helped uncover and fix the vote-ID-zero first-window edge case. All 72 governance tests pass with zero retries. Could you please re-review PR #99 when convenient? |
pheobeayo
left a comment
pheobeayo
left a comment
There was a problem hiding this comment.
All review items addressed; thanks for the fast turnaround. Verified the vote ID derivation and Pending-status claims directly against GoodDaoHouses.sol in GoodProtocol #299; both code comments are accurate, and the explicit voteId-0 handling in voteStartTimeFromSchedule also fixes a latent gap where the first vote window would have skipped provisional recipient filtering. Nice catch turning resolveRegistrationStake and selectGovernanceTransaction into tested pure functions.
Local verification note: the adapter-logic suite passes on my machine (receipts, ballots, unstake boundary, stake resolution, transaction selection all green).
One optional follow-up: batch the provisional getMember reads via multicall if the HoA list grows
|
Hey @pheobeayo, Thanks for the re-review and independent verification. I’ll keep the provisional-member multicall optimization as a future scalability follow-up since it is non-blocking for the current HoA size. |
|
Hi @L03TJ3, PR #99 has completed re-review. @pheobeayo confirmed that all requested changes are addressed and the focused tests pass. Can you kindly do a final review |
|
@L03TJ3 Awaiting your final review on this |
3 participants
Closes #80
Related: #75, #76, #77, GoodProtocol #299
Summary
Finalizes the contract-backed GovernanceWidget membership lifecycle and runtime boundaries introduced in #77. Active members now receive a truthful, receipt-aware
unstake()flow; no-member, pending Alignment, and revoked states follow the GoodDaoHouses contract; voting/funding edge cases are enforced before submission; and the adapter is split into maintainable widget-local hooks.The page hierarchy, header, onboarding stepper, membership cards, and transaction surfaces were aligned with the supplied governance UI references while keeping the existing GoodWidget design system and avoiding the out-of-scope detailed voting/dashboard redesign.
Changes
unstake()handling, successful-receipt refresh, and contextual wallet-confirmation, submitted, rejected, reverted, failed, and confirmed states.None/unstaked members, GoodID verification for Citizenship, HoA registry gating for Alignment, the pending Alignment state, and a non-actionable revoked state.useGovernanceAdapterto composition over dedicated membership, voting, and funding hooks without moving contract calls into UI components.Acceptance criteria
Membership lifecycle
unstake()call and wait for a successful receipt.member.updatedAt + termDuration; the locked state explains when it becomes available.None/unstaked members can join Citizenship; Alignment remains gated bygetHoaEligibility(account).isEligible.unrevokeworkaround was added.Runtime correctness and maintainability
useGovernanceAdapterremains a small composition boundary.packages/governance-widget; presentational cards and onboarding components stay contract-agnostic.Tests, evidence, and comments
tests/widgets/governance-widget/test-results/were regenerated for the latest UI states.Verification
eslint packages/governance-widget/srcplus changed governance specs)tsup)git diff --check/ staged diff checkpnpm lintcontinues to report the unrelated, pre-existing root failures required to be disclosed by #80:packages/core/src/detect.ts: unusedHostEnvironment.packages/embed/src/createMiniAppElement.tsx: unused_parameter.The final filtered Turbo invocation was blocked by the local non-interactive pnpm wrapper attempting to purge
node_modules(ERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY). The already-installed toolchain was therefore used directly; the governance package, including generated declarations, built successfully.Evidence
Scope confirmation
packages/ui,packages/core, orpackages/embedAPI changes.