test: add Moloch DAO-style systematic test suite (104 tests)#29
test: add Moloch DAO-style systematic test suite (104 tests)#29jeremylongshore wants to merge 1 commit intomainfrom
Conversation
Adopt Moloch DAO testing methodology: trigger every require, test every modifier, verify all state transitions, and test boundary conditions systematically. Increases test count from 448 to 552 with zero contract source changes. New files: - test/moloch/RequireAudit.t.sol (51 tests) - every untested revert path - test/moloch/BoundaryTests.t.sol (28 tests) - 0/1/MAX boundary conditions - test/moloch/ModifierTests.t.sol (17 tests) - modifier allow/reject pairs - test/moloch/StateTransitions.t.sol (8 tests) - comprehensive state verification - test/helpers/VerificationHelpers.sol - reusable state-checking assertions - test/helpers/MockETHRejecter.sol - ETH transfer failure helper - test/README.md - testing philosophy and naming conventions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds comprehensive test infrastructure for the IRSB Protocol, including test suite documentation, reusable verification helpers, and extensive test coverage for boundary conditions, modifiers, state transitions, and requirement audits across SolverRegistry, IntentReceiptHub, DisputeModule, and EscrowVault. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the protocol's test suite by integrating a robust set of 104 new tests inspired by the Moloch DAO testing methodology. These additions ensure critical contract logic, edge cases, and state transitions are thoroughly validated, covering all revert paths, boundary conditions, and modifier interactions. The improved test coverage aims to bolster the overall reliability and security of the system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an impressive and comprehensive test suite following the Moloch DAO testing methodology, significantly increasing test coverage with 104 new tests. The new tests for revert paths, boundary conditions, modifiers, and state transitions are well-structured and greatly enhance the robustness of the protocol. I've provided a few suggestions to improve the accuracy of the test documentation and the precision of a couple of test cases.
| │ ├── RequireAudit.t.sol # Every untested revert path (~43 tests) | ||
| │ ├── StateTransitions.t.sol # Comprehensive state verification (~10 tests) | ||
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~27 tests) | ||
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests) |
There was a problem hiding this comment.
The test counts mentioned in the Directory Structure section are inconsistent with the actual number of tests in the new files and the PR description. This can be misleading for developers looking at the documentation.
RequireAudit.t.sol: README says~43, but there are 51 tests.StateTransitions.t.sol: README says~10, but there are 8 tests.BoundaryTests.t.sol: README says~27, but there are 28 tests.
Please update these counts to be accurate for better documentation clarity.
| │ ├── RequireAudit.t.sol # Every untested revert path (~43 tests) | |
| │ ├── StateTransitions.t.sol # Comprehensive state verification (~10 tests) | |
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~27 tests) | |
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests) | |
| │ ├── RequireAudit.t.sol # Every untested revert path (51 tests) | |
| │ ├── StateTransitions.t.sol # Comprehensive state verification (8 tests) | |
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (28 tests) | |
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (17 tests) |
| function test_modifier_onlyHub_allows_hub() public { | ||
| bytes32 escrowId = keccak256("escrow"); | ||
| vault.createEscrow{ value: 1 ether }( | ||
| escrowId, keccak256("receipt"), address(this), uint64(block.timestamp + 1 hours) | ||
| ); | ||
|
|
||
| // Test contract is authorized hub (set in setUp) | ||
| vault.release(escrowId, address(this)); | ||
| // Success - no revert | ||
| } |
There was a problem hiding this comment.
The test test_modifier_onlyHub_allows_hub is intended to verify that an authorized hub can call a function with the onlyHub modifier. However, the test currently passes because the caller (address(this)) is the contract owner, which is also permitted by the modifier. The comment on line 196 is incorrect as address(this) is not configured as an authorized hub in the setUp function.
To correctly test the intended scenario, you should use vm.prank to make the call from the hub address, which is configured as an authorized hub. The test_modifier_onlyHub_allows_owner test already covers the owner path, so this change will make this test correctly target the authorized hub path.
function test_modifier_onlyHub_allows_hub() public {
bytes32 escrowId = keccak256("escrow");
vault.createEscrow{ value: 1 ether }(
escrowId, keccak256("receipt"), address(this), uint64(block.timestamp + 1 hours)
);
// Test that the authorized hub (set in setUp) can release the escrow
vm.prank(address(hub));
vault.release(escrowId, address(this));
// Success - no revert
}
| /// @notice setAuthorizedCaller reverts for non-owner | ||
| function test_requireFail_SolverRegistry_setAuthorizedCaller_nonOwner() public { | ||
| vm.prank(unauthorized); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
The test test_requireFail_SolverRegistry_setAuthorizedCaller_nonOwner uses a generic vm.expectRevert() to check for the revert from the onlyOwner modifier. While this works, it's less precise. For Solidity ^0.8.20, OpenZeppelin's Ownable contract reverts with the custom error OwnableUnauthorizedAccount(address account).
To make the test more specific and robust, you should expect this specific custom error. This ensures that the test fails if the revert reason changes unexpectedly.
You will also need to import Ownable at the top of the file:
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, unauthorized));
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/moloch/ModifierTests.t.sol`:
- Around line 189-198: The test test_modifier_onlyHub_allows_hub is currently
calling vault.release as the test owner instead of as the authorized hub; update
the test to invoke the call under vm.prank(address(hub)) so the release call
exercises the onlyHub modifier path (locate the test_modifier_onlyHub_allows_hub
function and wrap the vault.release(escrowId, address(this)) invocation with
vm.prank(address(hub)) to simulate the hub caller).
In `@test/moloch/StateTransitions.t.sol`:
- Around line 124-138: The test call lines exceed the configured line_length;
reformat the long verifyPostDeposit invocation arguments onto multiple lines to
satisfy forge fmt (line_length = 120). Locate the two verifyPostDeposit calls
(after registry.depositBond and vm.prank) and split their argument list so each
parameter (registry, solverId, MINIMUM_BOND - 1, Types.SolverStatus.Inactive,
totalBondedBefore + MINIMUM_BOND - 1) and the second call's parameters each
appear on their own or logically wrapped lines, preserving the same values and
order and keeping surrounding vm.prank/registry.depositBond calls unchanged.
In `@test/README.md`:
- Line 16: Update the markdown code fence that currently reads just ``` to
include a language identifier (e.g., change the opening fence to ```text) so the
directory tree block has a specified language; locate the plain code fence in
README.md and replace the opening backticks with ```text to satisfy
markdownlint.
- Around line 33-36: The test counts in test/README.md for RequireAudit.t.sol,
StateTransitions.t.sol, BoundaryTests.t.sol, and ModifierTests.t.sol are
outdated; update the approximate test numbers next to those filenames to match
the current PR totals (replace the stale counts ~43, ~10, ~27, ~17 with the
actual counts from the test suite run) so the README reflects the real suite
size and avoids confusion.
| /// @notice onlyHub allows authorized hub | ||
| function test_modifier_onlyHub_allows_hub() public { | ||
| bytes32 escrowId = keccak256("escrow"); | ||
| vault.createEscrow{ value: 1 ether }( | ||
| escrowId, keccak256("receipt"), address(this), uint64(block.timestamp + 1 hours) | ||
| ); | ||
|
|
||
| // Test contract is authorized hub (set in setUp) | ||
| vault.release(escrowId, address(this)); | ||
| // Success - no revert |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the test file
find . -name "ModifierTests.t.sol" -type fRepository: intent-solutions-io/irsb-protocol
Length of output: 110
🏁 Script executed:
# Check the structure of the repository
git ls-files | head -20Repository: intent-solutions-io/irsb-protocol
Length of output: 639
🏁 Script executed:
# Read the test file around lines 189-198 and surrounding context
wc -l ./test/moloch/ModifierTests.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 114
🏁 Script executed:
# Read the relevant section of the test file
sed -n '180,210p' ./test/moloch/ModifierTests.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 1263
🏁 Script executed:
# Search for the vault.release() implementation to understand onlyHub modifier
find . -name "*.sol" -type f | grep -v test | grep -v node_modulesRepository: intent-solutions-io/irsb-protocol
Length of output: 1521
🏁 Script executed:
# Read the EscrowVault.sol file to find the onlyHub modifier and release() function
wc -l ./src/EscrowVault.solRepository: intent-solutions-io/irsb-protocol
Length of output: 102
🏁 Script executed:
# Search for the onlyHub modifier definition
rg "modifier onlyHub" ./src/EscrowVault.sol -A 10Repository: intent-solutions-io/irsb-protocol
Length of output: 393
🏁 Script executed:
# Search for the release function in EscrowVault
rg "function release" ./src/EscrowVault.sol -B 3 -A 15Repository: intent-solutions-io/irsb-protocol
Length of output: 772
🏁 Script executed:
# Now let's check the setUp function to understand how the test is initialized
sed -n '1,100p' ./test/moloch/ModifierTests.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 3752
🏁 Script executed:
# Search for authorizedHubs setup in the test
rg "authorizedHubs" ./test/moloch/ModifierTests.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 59
🏁 Script executed:
# Check EscrowVault initialization and authorizedHubs setup
rg "authorizedHubs" ./src/EscrowVault.sol -B 2 -A 2Repository: intent-solutions-io/irsb-protocol
Length of output: 747
🏁 Script executed:
# Let me verify one more detail - check the second onlyHub test to confirm pattern
sed -n '199,210p' ./test/moloch/ModifierTests.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 508
onlyHub "allows hub" test isn't invoking as the hub.
This call currently executes as the owner, so it doesn't validate the hub authorization path. Add a vm.prank(address(hub)) to exercise the modifier properly.
🔧 Suggested fix
- // Test contract is authorized hub (set in setUp)
- vault.release(escrowId, address(this));
+ vm.prank(address(hub));
+ vault.release(escrowId, address(this));🤖 Prompt for AI Agents
In `@test/moloch/ModifierTests.t.sol` around lines 189 - 198, The test
test_modifier_onlyHub_allows_hub is currently calling vault.release as the test
owner instead of as the authorized hub; update the test to invoke the call under
vm.prank(address(hub)) so the release call exercises the onlyHub modifier path
(locate the test_modifier_onlyHub_allows_hub function and wrap the
vault.release(escrowId, address(this)) invocation with vm.prank(address(hub)) to
simulate the hub caller).
| // Deposit below minimum — stays Inactive | ||
| vm.prank(operator); | ||
| registry.depositBond{ value: MINIMUM_BOND - 1 }(solverId); | ||
|
|
||
| verifyPostDeposit( | ||
| registry, solverId, MINIMUM_BOND - 1, Types.SolverStatus.Inactive, totalBondedBefore + MINIMUM_BOND - 1 | ||
| ); | ||
|
|
||
| // Deposit 1 more wei — activates | ||
| vm.prank(operator); | ||
| registry.depositBond{ value: 1 }(solverId); | ||
|
|
||
| verifyPostDeposit( | ||
| registry, solverId, MINIMUM_BOND, Types.SolverStatus.Active, totalBondedBefore + MINIMUM_BOND | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file
find . -name "StateTransitions.t.sol" -type fRepository: intent-solutions-io/irsb-protocol
Length of output: 113
🏁 Script executed:
# Check if foundry.toml exists and review forge fmt settings
find . -name "foundry.toml" -o -name ".prettierrc" -type f | head -20Repository: intent-solutions-io/irsb-protocol
Length of output: 91
🏁 Script executed:
# Read the specific file to see the current state
if [ -f "test/moloch/StateTransitions.t.sol" ]; then
sed -n '120,145p' test/moloch/StateTransitions.t.sol
fiRepository: intent-solutions-io/irsb-protocol
Length of output: 1176
🏁 Script executed:
# Check foundry.toml for forge fmt settings
cat foundry.tomlRepository: intent-solutions-io/irsb-protocol
Length of output: 931
🏁 Script executed:
# Verify the exact line numbers match the review comment
wc -l test/moloch/StateTransitions.t.solRepository: intent-solutions-io/irsb-protocol
Length of output: 115
🏁 Script executed:
# Check if forge fmt would report this as a violation
# First, let's see what the exact formatting rules are
cd . && forge fmt --help 2>&1 | head -50Repository: intent-solutions-io/irsb-protocol
Length of output: 120
🏁 Script executed:
# Try to locate forge or check if it's installed elsewhere
which forge || find /usr -name forge -type f 2>/dev/null | head -5Repository: intent-solutions-io/irsb-protocol
Length of output: 120
🏁 Script executed:
# Let's check the exact character count of the problematic lines
python3 << 'EOF'
lines = [
" registry, solverId, MINIMUM_BOND - 1, Types.SolverStatus.Inactive, totalBondedBefore + MINIMUM_BOND - 1",
" registry, solverId, MINIMUM_BOND, Types.SolverStatus.Active, totalBondedBefore + MINIMUM_BOND"
]
for i, line in enumerate(lines, 1):
print(f"Line {i}: {len(line)} chars")
print(f" Content: {line}")
EOFRepository: intent-solutions-io/irsb-protocol
Length of output: 356
🏁 Script executed:
# Check if there are other similar function calls in the file to see formatting patterns
rg "verifyPostDeposit" test/moloch/StateTransitions.t.sol -A 1Repository: intent-solutions-io/irsb-protocol
Length of output: 355
🏁 Script executed:
# Look for other multi-argument function calls in test files to see how they're formatted
rg -A 2 "^\s+\w+\(" test/moloch/StateTransitions.t.sol | head -40Repository: intent-solutions-io/irsb-protocol
Length of output: 2010
Apply forge fmt to split verifyPostDeposit arguments across lines.
The configured line_length = 120 in foundry.toml is exceeded by the current argument formatting. Split arguments to separate lines as shown:
🧹 Suggested formatting
- verifyPostDeposit(
- registry, solverId, MINIMUM_BOND - 1, Types.SolverStatus.Inactive, totalBondedBefore + MINIMUM_BOND - 1
- );
+ verifyPostDeposit(
+ registry,
+ solverId,
+ MINIMUM_BOND - 1,
+ Types.SolverStatus.Inactive,
+ totalBondedBefore + MINIMUM_BOND - 1
+ );
@@
- verifyPostDeposit(
- registry, solverId, MINIMUM_BOND, Types.SolverStatus.Active, totalBondedBefore + MINIMUM_BOND
- );
+ verifyPostDeposit(
+ registry,
+ solverId,
+ MINIMUM_BOND,
+ Types.SolverStatus.Active,
+ totalBondedBefore + MINIMUM_BOND
+ );📝 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.
| // Deposit below minimum — stays Inactive | |
| vm.prank(operator); | |
| registry.depositBond{ value: MINIMUM_BOND - 1 }(solverId); | |
| verifyPostDeposit( | |
| registry, solverId, MINIMUM_BOND - 1, Types.SolverStatus.Inactive, totalBondedBefore + MINIMUM_BOND - 1 | |
| ); | |
| // Deposit 1 more wei — activates | |
| vm.prank(operator); | |
| registry.depositBond{ value: 1 }(solverId); | |
| verifyPostDeposit( | |
| registry, solverId, MINIMUM_BOND, Types.SolverStatus.Active, totalBondedBefore + MINIMUM_BOND | |
| ); | |
| // Deposit below minimum — stays Inactive | |
| vm.prank(operator); | |
| registry.depositBond{ value: MINIMUM_BOND - 1 }(solverId); | |
| verifyPostDeposit( | |
| registry, | |
| solverId, | |
| MINIMUM_BOND - 1, | |
| Types.SolverStatus.Inactive, | |
| totalBondedBefore + MINIMUM_BOND - 1 | |
| ); | |
| // Deposit 1 more wei — activates | |
| vm.prank(operator); | |
| registry.depositBond{ value: 1 }(solverId); | |
| verifyPostDeposit( | |
| registry, | |
| solverId, | |
| MINIMUM_BOND, | |
| Types.SolverStatus.Active, | |
| totalBondedBefore + MINIMUM_BOND | |
| ); |
🧰 Tools
🪛 GitHub Actions: CI
[error] 136-138: forge fmt --check: formatting mismatch in verifyPostDeposit call.
🤖 Prompt for AI Agents
In `@test/moloch/StateTransitions.t.sol` around lines 124 - 138, The test call
lines exceed the configured line_length; reformat the long verifyPostDeposit
invocation arguments onto multiple lines to satisfy forge fmt (line_length =
120). Locate the two verifyPostDeposit calls (after registry.depositBond and
vm.prank) and split their argument list so each parameter (registry, solverId,
MINIMUM_BOND - 1, Types.SolverStatus.Inactive, totalBondedBefore + MINIMUM_BOND
- 1) and the second call's parameters each appear on their own or logically
wrapped lines, preserving the same values and order and keeping surrounding
vm.prank/registry.depositBond calls unchanged.
|
|
||
| ## Directory Structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify a language for the directory tree fence.
markdownlint flags this fence because it lacks a language identifier.
📝 Suggested change
-```
+```text📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@test/README.md` at line 16, Update the markdown code fence that currently
reads just ``` to include a language identifier (e.g., change the opening fence
to ```text) so the directory tree block has a specified language; locate the
plain code fence in README.md and replace the opening backticks with ```text to
satisfy markdownlint.
| │ ├── RequireAudit.t.sol # Every untested revert path (~43 tests) | ||
| │ ├── StateTransitions.t.sol # Comprehensive state verification (~10 tests) | ||
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~27 tests) | ||
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests) |
There was a problem hiding this comment.
Update the Moloch test counts to match the new suite size.
The listed approximate counts look stale relative to the PR totals; aligning them avoids confusion.
📝 Suggested change
-│ ├── RequireAudit.t.sol # Every untested revert path (~43 tests)
-│ ├── StateTransitions.t.sol # Comprehensive state verification (~10 tests)
-│ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~27 tests)
-│ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests)
+│ ├── RequireAudit.t.sol # Every untested revert path (~51 tests)
+│ ├── StateTransitions.t.sol # Comprehensive state verification (~8 tests)
+│ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~28 tests)
+│ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests)📝 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.
| │ ├── RequireAudit.t.sol # Every untested revert path (~43 tests) | |
| │ ├── StateTransitions.t.sol # Comprehensive state verification (~10 tests) | |
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~27 tests) | |
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests) | |
| │ ├── RequireAudit.t.sol # Every untested revert path (~51 tests) | |
| │ ├── StateTransitions.t.sol # Comprehensive state verification (~8 tests) | |
| │ ├── BoundaryTests.t.sol # 0/1/MAX boundary conditions (~28 tests) | |
| │ └── ModifierTests.t.sol # Modifier allow/reject pairs (~17 tests) |
🤖 Prompt for AI Agents
In `@test/README.md` around lines 33 - 36, The test counts in test/README.md for
RequireAudit.t.sol, StateTransitions.t.sol, BoundaryTests.t.sol, and
ModifierTests.t.sol are outdated; update the approximate test numbers next to
those filenames to match the current PR totals (replace the stale counts ~43,
~10, ~27, ~17 with the actual counts from the test suite run) so the README
reflects the real suite size and avoids confusion.
1 participant
Summary
New Test Files
test/moloch/RequireAudit.t.soltest/moloch/BoundaryTests.t.soltest/moloch/ModifierTests.t.soltest/moloch/StateTransitions.t.solHelper Files
test/helpers/VerificationHelpers.sol— 5 reusable state-checking assertion functionstest/helpers/MockETHRejecter.sol— Contract withoutreceive()for ETH transfer failure teststest/README.md— Testing philosophy, naming conventions, run commandsTest plan
forge test— all 552 tests pass (448 existing + 104 new)forge test --match-path "test/moloch/*"— 104/104 pass🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation