[#1267] Add claim deadline + sweepUnclaimed to MerkleClaim#1279
Conversation
Rename PLOTAirdrop → MerkleClaim. Add immutable owner (deployer), claimDeadline constructor arg, claim-after-deadline revert, and owner-gated sweepUnclaimed function with Swept event. Bootstrap Foundry: foundry.toml scoped under contracts/, setup.sh for fresh clone deps, deploy script template. 8 Foundry tests cover claim before/after deadline, invalid proof, double claim, sweep before/after deadline, non-owner sweep, and double sweep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
Security review of MerkleClaim.sol:
- Checks-effects-interactions pattern in
claim():claimed[msg.sender] = trueset BEFOREPLOT.transfer()— no reentrancy risk ✅ - Double-hash leaf:
keccak256(bytes.concat(keccak256(abi.encode(msg.sender, amount))))— standard OZ pattern prevents second-preimage attacks ✅ - All state vars
immutable(owner, PLOT, merkleRoot, claimDeadline) — no admin key rotation, no root changes. Minimal attack surface ✅ - Deadline boundary: claims allowed at
<=, sweep at>— no overlap, no gap ✅ sweepUnclaimedtransfers full balance viaPLOT.transfer()— safe for ERC20 tokens, handles 0-balance gracefully ✅- No pause, no upgrade keys, no setMerkleRoot, no partial sweep — per spec ✅
Acceptance criteria:
| Criterion | Status |
|---|---|
| Renamed PLOTAirdrop → MerkleClaim | ✅ |
| Owner (immutable, deployer) + onlyOwner modifier | ✅ |
| claimDeadline as constructor arg (uint256 unix timestamp) | ✅ |
| claim() reverts after deadline | ✅ |
| sweepUnclaimed: owner-only, after deadline | ✅ |
| Swept event emitted | ✅ |
| Foundry bootstrapped (foundry.toml + tests + scripts) | ✅ |
libs = ["contracts/lib"] not ["lib"] |
✅ |
contracts/setup.sh with forge install commands |
✅ |
| .gitignore for Foundry artifacts | ✅ |
| .env.example appended (not overwritten) | ✅ |
| All 8 unit tests present | ✅ |
| Version 1.30.5 → 1.31.0 (feature) | ✅ |
Test review:
- 2-leaf Merkle tree with correct leaf ordering (smaller first per OZ convention)
- All 8 required test scenarios present with proper
vm.warp,vm.prank, andvm.expectRevert - Double-sweep test confirms 0-transfer behavior
Minor note (non-blocking): Issue #1267 lists "README or docs documenting setup step" as acceptance — not present in this PR. Can be a follow-up.
No blocking issues found.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The Solidity contract changes are broadly aligned with #1267: MerkleClaim has immutable owner/deadline fields, claim deadline enforcement, owner-only post-deadline sweep, and the required Foundry tests are present. A few explicit acceptance items are still missing or stale.
Findings
-
[medium] The fresh-clone Foundry setup step is not documented. #1267 explicitly requires README or
docs/airdrop-contracts.mddocumentation telling operators to runbash contracts/setup.shbefore testing/deploying, but this PR adds the script without any README/docs change.- File:
contracts/setup.sh:1 - Suggestion: Add a short README section covering
bash contracts/setup.sh,forge test -vv, and the deploy env vars/command.
- File:
-
[medium]
package-lock.jsonis stale after the package version bump.package.jsonmoves to1.31.0, but the lockfile root still reports1.30.5, which leaves install metadata inconsistent for CI/fresh clones.- File:
package-lock.json:3 - Suggestion: Regenerate or update
package-lock.jsonso the root package version matchespackage.json.
- File:
-
[low] The required gas snapshot/comparison is not included or documented. #1267 asks for
forge snapshotand confirmation that the claim path stays within +5% of v1.- File:
contracts/test/MerkleClaim.t.sol:56 - Suggestion: Add the gas snapshot artifact if that is the repo convention, or document the
forge snapshotresult in the PR/README so reviewers can verify the acceptance item.
- File:
Decision
Requesting changes until the explicit setup documentation, lockfile consistency, and gas-snapshot acceptance item are addressed. CI was still pending at review time.
Add docs/airdrop-contracts.md with setup, test, gas snapshot, and deploy instructions. Fix package-lock.json version to 1.31.0. Claim gas: 83,648 (deadline check adds ~2% vs v1 baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The lockfile version and fresh-clone setup documentation from the previous review are fixed. Two remaining acceptance/security issues need cleanup before approval.
Findings
-
[medium] The required gas snapshot/comparison is still not documented in the PR-controlled files. The new docs show the
forge snapshotcommand, but they do not record the claim-path gas result or the +5% comparison requested by #1267.- File:
docs/airdrop-contracts.md:17 - Suggestion: Add the measured claim gas and baseline comparison to the docs or commit a gas snapshot artifact if that is the preferred repo convention.
- File:
-
[medium] The deploy env examples include a concrete
PLOT_TOKEN_ADDRESSvalue. Per the reviewer rules for sensitive data, committed code/docs and PR output should use placeholders for on-chain addresses unless the operator explicitly asks for the real value.- File:
.env.example:90 - Suggestion: Replace concrete address values in
.env.exampleanddocs/airdrop-contracts.mdwith a placeholder such as<PLOT_TOKEN_ADDRESS>.
- File:
Decision
Requesting changes until the gas comparison is actually documented and the concrete address is replaced with a placeholder. CI currently shows only Vercel status contexts for the latest push.
Document claim gas: 83,648 (~+2% vs v1, within +5% threshold). Replace concrete PLOT token address in .env.example and docs with placeholder per sensitive-data review rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested fixes are addressed. The contract implements the #1267 owner/deadline/sweep requirements, Foundry is scoped under contracts/, the eight required tests are present, fresh-clone setup and deploy docs are included, the lockfile version matches package.json, and the gas comparison is documented within the +5% threshold.
Findings
- None.
Decision
Approved. Latest visible checks are Vercel status contexts only, so wait for any required branch checks before merge.
Summary
PLOTAirdrop→MerkleClaim(matches filename + spec)immutable ownerset to deployer in constructor,onlyOwnermodifierimmutable claimDeadline(unix timestamp constructor arg),claim()reverts after deadlinesweepUnclaimed(to)— owner-only, after deadline, transfers full PLOT balance, emitsSweptFoundry bootstrap
foundry.tomlwith all paths scoped undercontracts/(libs = ["contracts/lib"])contracts/setup.shfor fresh clone: installs OZ v5.0.0 + forge-stdcontracts/script/DeployMerkleClaim.s.soldeploy template.gitignoreupdated forcontracts/out/,contracts/cache/,contracts/lib/,broadcast/.env.exampleappended with Foundry/deploy vars (existing vars untouched)Tests (8/8 pass via
forge test -vv)test_claim_BeforeDeadline_Succeedstest_claim_AfterDeadline_Revertstest_claim_InvalidProof_Revertstest_claim_DoubleClaim_Revertstest_sweep_BeforeDeadline_Revertstest_sweep_AfterDeadline_BySomeone_Revertstest_sweep_AfterDeadline_ByOwner_Succeedstest_sweep_DoubleSweep_TransfersZeroVersion
1.30.5 → 1.31.0 (feature)
🤖 Generated with Claude Code