Implement GoodDaoHouses MVP smart contract build#299
Open
Copilot wants to merge 14 commits into
Open
Conversation
Copilot
AI
changed the title
[WIP] Implement MVP outline for GoodDaoHouses
Add maintainer-reviewable GoodDaoHouses MVP implementation plan
Jun 10, 2026
Contributor
Author
Contributor
The deliverible is: the smart-contract build. |
Author
... Addressed in 478c1c7: removed the committed skills payload and replaced the plan-only change with the GoodDaoHouses contract implementation, FlowSplitter integration surface, and focused governance tests. |
Copilot
AI
changed the title
Add maintainer-reviewable GoodDaoHouses MVP implementation plan
Implement GoodDaoHouses MVP smart contract build
Jun 10, 2026
Implement GoodDaoHouses MVP smart contract build
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
sirpy
requested changes
Jun 11, 2026
sirpy
left a comment
sirpy
left a comment
Contributor
There was a problem hiding this comment.
- the fields should be written on chain. for distribution strategy it should be a url to a post on our forum.
- quarterly/term vote: modulo timestamp by termDuration if result is between 0 and votingTermLength then we are in voting period time frame, first vote will "create" the vote record by calling an internal createAlignmentVote, the voteId can simply be the result of timestamp divided by termDuration..
- no need for _activeAlignment or _activeCitizens.
- no need for _voteAlignmentVoters,_voteCitizensVoters, _voteWeightSnapshot assume voting weight for citizen/alignment wont change during vote. check member join date to verify he can participate in current vote.
- totalUnits + totalWeights not required for "vote" record in createAlignmentVote
- voteRecipientWeightedVotes should be divided by BASIS_POINTS
7, finalizeAlignmentVote step is not required. - in executeVote each member units is simply the value he got in voteRecipientWeightedVotes
Contributor
|
Author
Addressed in 6fb9881. |
This was referenced Jun 16, 2026
Contributor
|
Last fixes include:
Related to 'removing flowsplitter logic': the contract should only govern the update of units in a flowsplitter pool and adding/removing members. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
registerAndStake,amountcan become negative when the caller’s existing stake already exceeds the new minimum, which then gets cast touintand fed into_registerMember; this will overflow and revert in 0.8.x — consider clamping the delta to zero (or reverting explicitly) whenmembers[msg.sender].stakedAmount >= minimumStake[house]before passing it along. - The
isVotingPeriodcheck uses<= votingTermLength, which makes the voting window effectivelyvotingTermLength + 1seconds long; if you want an exact-length window, consider switching this to a strict< votingTermLengthcomparison.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `registerAndStake`, `amount` can become negative when the caller’s existing stake already exceeds the new minimum, which then gets cast to `uint` and fed into `_registerMember`; this will overflow and revert in 0.8.x — consider clamping the delta to zero (or reverting explicitly) when `members[msg.sender].stakedAmount >= minimumStake[house]` before passing it along.
- The `isVotingPeriod` check uses `<= votingTermLength`, which makes the voting window effectively `votingTermLength + 1` seconds long; if you want an exact-length window, consider switching this to a strict `< votingTermLength` comparison.
## Individual Comments
### Comment 1
<location path="contracts/governance/GoodDaoHouses.sol" line_range="234-243" />
<code_context>
+ string calldata distributionStrategy
+ ) external whenNotPaused {
+ // Collect only the missing delta between current stake and the house minimum.
+ int amount = int(minimumStake[house]) -
+ int(members[msg.sender].stakedAmount);
+ if (amount > 0) {
+ require(
+ _goodDollar().transferFrom(msg.sender, address(this), uint256(amount)),
+ "G$ transferFrom"
+ );
+ }
+
+ _registerMember(
+ msg.sender,
+ house,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid casting potentially negative `int` to `uint` in `registerAndStake`.
If a member’s existing `stakedAmount` ≥ `minimumStake[house]`, `amount` is ≤ 0. You correctly skip the transfer when `amount <= 0`, but still pass `uint(amount)` to `_registerMember`, which will revert for negative `amount` due to the `int -> uint` cast in Solidity 0.8.
Compute a nonnegative delta before casting, e.g.:
```solidity
int256 delta = int256(minimumStake[house]) - int256(members[msg.sender].stakedAmount);
uint256 stakeDelta = delta > 0 ? uint256(delta) : 0;
if (stakeDelta > 0) {
require(
_goodDollar().transferFrom(msg.sender, address(this), stakeDelta),
"G$ transferFrom"
);
}
_registerMember(
msg.sender,
house,
stakeDelta,
name,
socialLinks,
projectWebpage,
missionStatement,
distributionStrategy
);
```
This avoids casting negative values and prevents over-staked calls from reverting.
</issue_to_address>
### Comment 2
<location path="contracts/governance/GoodDaoHouses.sol" line_range="454-456" />
<code_context>
+ require(!vote.executed, "Vote executed");
+
+ // Translate finalized weighted votes into FlowSplitter unit updates.
+ for (uint256 i = 0; i < count; i++) {
+ address recipient = recipients[i];
+ uint128 units = uint128(voteRecipientWeightedVotes[voteId][recipient]);
+ flowMembers[i] = IFlowSplitter.Member({
+ account: recipient,
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting accumulated vote weights from `uint256` to `uint128` can silently truncate.
Here `voteRecipientWeightedVotes[voteId][recipient]` is a `uint256` but is cast to `uint128` without validation. If total weights exceed `2**128 - 1`, this will silently truncate and produce incorrect weights for the FlowSplitter.
To avoid this, either:
- Enforce and document an invariant that total weights always fit in `uint128`; or
- Add an explicit bound check before casting:
```solidity
uint256 raw = voteRecipientWeightedVotes[voteId][recipient];
require(raw <= type(uint128).max, "Units overflow");
uint128 units = uint128(raw);
```
so that overflow fails loudly instead of corrupting the result.
</issue_to_address>
### Comment 3
<location path="contracts/governance/GoodDaoHouses.sol" line_range="464-465" />
<code_context>
+ weights[i] = units;
+ }
+
+ IFlowSplitter(flowConfig.splitter).updateMembersUnits(
+ flowConfig.poolId,
+ flowMembers
+ );
</code_context>
<issue_to_address>
**issue:** Add validation that FlowSplitter configuration is present before executing a vote.
`executeVote` assumes `flowConfig.splitter` and `flowConfig.poolId` are initialized. If `configureFlowSplitter` wasn’t called, `splitter` will be `address(0)` and the call will revert with a low-level error; a zero `poolId` may also be rejected by the FlowSplitter.
Add explicit guards, e.g.:
```solidity
require(flowConfig.splitter != address(0), "FlowSplitter not configured");
require(flowConfig.poolId != 0, "Pool not configured");
```
so misconfiguration fails clearly instead of via a generic revert.
</issue_to_address>
### Comment 4
<location path="test/governance/GoodDaoHouses.test.ts" line_range="153" />
<code_context>
+ expect(activeAlignmentMembers).to.deep.equal([alignmentOne.address]);
+ });
+
+ it("lets admin or committee set the voting schedule anchor and term lengths", async () => {
+ const { admin, committee, houses } = await loadFixture(fixture);
+ const latestBlock = await ethers.provider.getBlock("latest");
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative-path tests for voting schedule configuration
Currently only the happy path for `setVotingSchedule` is tested. Please also cover:
- Calls from non-admin / non-committee signers reverting as expected.
- Invalid parameter combinations (e.g. `votingTermLength > termDuration`, zero durations, or disallowed past cycle start) reverting with the correct error.
This will better lock in access control and validation behavior.
Suggested implementation:
```typescript
const cycleStartTime = (await houses.cycleStartTime()).toNumber();
const termDuration = (await houses.termDuration()).toNumber();
const delta =
latestBlock.timestamp < cycleStartTime
? cycleStartTime - latestBlock.timestamp
: (() => {
const offset = (latestBlock.timestamp - cycleStartTime) % termDuration;
return offset === 0 ? 1 : termDuration - offset + 1;
})();
const newCycleStartTime = latestBlock.timestamp + delta;
const validVotingTermLength = Math.floor(termDuration / 2);
// happy-path: admin configures schedule
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
)
.to.emit(houses, "VotingScheduleUpdated")
.withArgs(newCycleStartTime, termDuration, validVotingTermLength);
// negative-path: non-admin / non-committee cannot configure schedule
const { stranger } = await loadFixture(fixture);
await expect(
houses
.connect(stranger)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "OnlyAdminOrCommittee");
// negative-path: voting term longer than term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, termDuration + 1),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, 0, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero voting term length
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, 0),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: cycle start time in the past
await expect(
houses
.connect(admin)
.setVotingSchedule(latestBlock.timestamp - 1, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
```
To make this compile and match your actual contracts/test helpers you’ll likely need to:
1. Adjust the `setVotingSchedule` call signature and argument ordering if it differs (e.g. if it accepts `(cycleStart, votingTermLength, termDuration)` or additional parameters).
2. Replace `"VotingScheduleUpdated"` with the actual event name (or drop the `.to.emit` if no event is emitted).
3. Replace the custom error identifiers `"OnlyAdminOrCommittee"` and `"InvalidVotingSchedule"` with the actual custom error names (or `.to.be.revertedWith` messages) used in `GoodDaoHouses`.
4. Ensure `fixture` returns a `stranger` or similar non-privileged signer (e.g. `{ admin, committee, houses, stranger }`). If it currently only returns `{ admin, committee, houses }`, extend it accordingly or derive `stranger` via `ethers.getSigners()`.
5. If you prefer separate test cases instead of a single expanded one, you can extract the negative-path sections into their own `it(...)` blocks reusing the same fixture setup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| expect(activeAlignmentMembers).to.deep.equal([alignmentOne.address]); | ||
| }); | ||
|
|
||
| it("lets admin or committee set the voting schedule anchor and term lengths", async () => { |
There was a problem hiding this comment.
suggestion (testing): Add negative-path tests for voting schedule configuration
Currently only the happy path for setVotingSchedule is tested. Please also cover:
- Calls from non-admin / non-committee signers reverting as expected.
- Invalid parameter combinations (e.g.
votingTermLength > termDuration, zero durations, or disallowed past cycle start) reverting with the correct error.
This will better lock in access control and validation behavior.
Suggested implementation:
const cycleStartTime = (await houses.cycleStartTime()).toNumber();
const termDuration = (await houses.termDuration()).toNumber();
const delta =
latestBlock.timestamp < cycleStartTime
? cycleStartTime - latestBlock.timestamp
: (() => {
const offset = (latestBlock.timestamp - cycleStartTime) % termDuration;
return offset === 0 ? 1 : termDuration - offset + 1;
})();
const newCycleStartTime = latestBlock.timestamp + delta;
const validVotingTermLength = Math.floor(termDuration / 2);
// happy-path: admin configures schedule
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
)
.to.emit(houses, "VotingScheduleUpdated")
.withArgs(newCycleStartTime, termDuration, validVotingTermLength);
// negative-path: non-admin / non-committee cannot configure schedule
const { stranger } = await loadFixture(fixture);
await expect(
houses
.connect(stranger)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "OnlyAdminOrCommittee");
// negative-path: voting term longer than term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, termDuration + 1),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, 0, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero voting term length
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, 0),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: cycle start time in the past
await expect(
houses
.connect(admin)
.setVotingSchedule(latestBlock.timestamp - 1, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");To make this compile and match your actual contracts/test helpers you’ll likely need to:
- Adjust the
setVotingSchedulecall signature and argument ordering if it differs (e.g. if it accepts(cycleStart, votingTermLength, termDuration)or additional parameters). - Replace
"VotingScheduleUpdated"with the actual event name (or drop the.to.emitif no event is emitted). - Replace the custom error identifiers
"OnlyAdminOrCommittee"and"InvalidVotingSchedule"with the actual custom error names (or.to.be.revertedWithmessages) used inGoodDaoHouses. - Ensure
fixturereturns astrangeror similar non-privileged signer (e.g.{ admin, committee, houses, stranger }). If it currently only returns{ admin, committee, houses }, extend it accordingly or derivestrangerviaethers.getSigners(). - If you prefer separate test cases instead of a single expanded one, you can extract the negative-path sections into their own
it(...)blocks reusing the same fixture setup.
There was a problem hiding this comment.
Pull request overview
This PR introduces an MVP implementation of the GoodDaoHouses governance contract, including a minimal FlowSplitter integration interface, a mock FlowSplitter for testing, and a dedicated Hardhat test suite covering membership and term-based voting/execution flows.
Changes:
- Added
GoodDaoHousesupgradeable governance contract with Citizens/Alignment membership, staking, and term voting/execution that updates FlowSplitter units. - Added a minimal
IFlowSplitterinterface plus aMockFlowSplitterimplementation for deterministic governance testing. - Added targeted governance tests for registration/staking, approval/revocation/unstake flows, vote lifecycle, and FlowSplitter unit updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| contracts/governance/GoodDaoHouses.sol | New governance contract implementing house membership, staking, term voting, and FlowSplitter execution boundary. |
| contracts/governance/IFlowSplitter.sol | Minimal interface surface used by GoodDaoHouses to drive pool member unit updates. |
| contracts/mocks/MockFlowSplitter.sol | Mock FlowSplitter + pool used to test execution behavior without external dependencies. |
| test/governance/GoodDaoHouses.test.ts | Test suite validating membership lifecycle, voting window behavior, and FlowSplitter unit updates/cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
|
@copilot fix unresolved code review conversations. make sure fixes are matched by relevant unit tests |
Author
Fixed in the latest commit. Here's what was addressed:
Each fix is accompanied by a new unit test in |
Contributor
|
@copilot add the suggested tests Currently only the happy path for
Suggested implementation: const cycleStartTime = (await houses.cycleStartTime()).toNumber();
const termDuration = (await houses.termDuration()).toNumber();
const delta =
latestBlock.timestamp < cycleStartTime
? cycleStartTime - latestBlock.timestamp
: (() => {
const offset = (latestBlock.timestamp - cycleStartTime) % termDuration;
return offset === 0 ? 1 : termDuration - offset + 1;
})();
const newCycleStartTime = latestBlock.timestamp + delta;
const validVotingTermLength = Math.floor(termDuration / 2);
// happy-path: admin configures schedule
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
)
.to.emit(houses, "VotingScheduleUpdated")
.withArgs(newCycleStartTime, termDuration, validVotingTermLength);
// negative-path: non-admin / non-committee cannot configure schedule
const { stranger } = await loadFixture(fixture);
await expect(
houses
.connect(stranger)
.setVotingSchedule(newCycleStartTime, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "OnlyAdminOrCommittee");
// negative-path: voting term longer than term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, termDuration + 1),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero term duration
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, 0, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: zero voting term length
await expect(
houses
.connect(admin)
.setVotingSchedule(newCycleStartTime, termDuration, 0),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");
// negative-path: cycle start time in the past
await expect(
houses
.connect(admin)
.setVotingSchedule(latestBlock.timestamp - 1, termDuration, validVotingTermLength),
).to.be.revertedWithCustomError(houses, "InvalidVotingSchedule");To make this compile and match your actual contracts/test helpers you’ll likely need to:
@copilot fix and add tests Here To avoid this, either:
uint256 raw = voteRecipientWeightedVotes[voteId][recipient];
require(raw <= type(uint128).max, "Units overflow");
uint128 units = uint128(raw);so that overflow fails loudly instead of corrupting the result. |
Author
... Done in the latest commit. Here's what was added/changed: Contract (
Tests (
|
Contributor
|
@copilot add the negative test case executeVote: uint128 cast safety |
Author
Done in the latest commit. Since normal voting accumulation can never reach The negative test (
|
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
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.
This PR implements the
GoodDaoHousesMVP as an actual smart-contract deliverable, replacing the earlier planning-only artifact. It adds the governance contract, the FlowSplitter integration surface it depends on, a mock for focused testing, and targeted governance tests covering the core membership and vote execution flows.GoodDaoHouses contract
contracts/governance/GoodDaoHouses.sol.Citizens/Alignmenthouse model with per-house minimum stake configuration.transferAndCallregistration, direct register-and-stake, additional staking, approval, revocation, and unstake flows.name/socialLinks, and alignmentname/projectWebpage/missionStatement/distributionStrategy.DAOUpgradeableContract,NameService, access control, pausing, and avatar-authorized upgrades.Membership and voting model
None,Pending,Active,Revoked,Unstaked) and alignment eligibility records onchain.Citizensmembers immediately after staking and gatesAlignmentmembers behind committee-managed eligibility and approval.termDurationandvotingTermLength, where the first ballot cast during the voting window lazily creates the vote record and usestimestamp / termDurationas thevoteId.joinedAtrelative to the current term vote start.voteRecipientWeightedVotes, dividing weighted allocations byBASIS_POINTS.FlowSplitter execution boundary
contracts/governance/IFlowSplitter.solto mirror the pool-based integration surface needed by the MVP.voteRecipientWeightedVotesvalues intoIFlowSplitter.Member[]at the integration boundary.Tests and supporting mock
contracts/mocks/MockFlowSplitter.solfor focused governance testing.test/governance/GoodDaoHouses.test.ts.Cleanup
skills-lock.json.Testing
yarn compile(blocked in this sandbox because Hardhat could not reachbinaries.soliditylang.orgto download the Solidity compiler)yarn hardhat test test/governance/GoodDaoHouses.test.ts(blocked for the same compiler-download reason)solccompile check forcontracts/governance/GoodDaoHouses.soltest/governance/GoodDaoHouses.test.tsExample of the implemented execution boundary:
Summary by Sourcery
Introduce the GoodDaoHouses governance contract MVP with member-staking houses and FlowSplitter-based vote execution.
New Features:
Enhancements:
Tests: