[#1304] Add TWAP minimum samples validation#1309
Conversation
Finalize script now requires >=5 daily price samples for TWAP (throws with clear message if insufficient). Warns for 5-6 samples (expected 7). Operator override via AIRDROP_FINALIZE_ALLOW_PARTIAL_TWAP=1 for emergency partial data. Closes #1304 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
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
| Criterion | Status |
|---|---|
| <5 samples → throws with clear message | ✅ |
| 5-6 samples → warns, proceeds | ✅ |
| 7 samples → clean | ✅ |
Override: AIRDROP_FINALIZE_ALLOW_PARTIAL_TWAP=1 |
✅ |
| Version 1.41.0 → 1.41.1 (patch) | ✅ |
Clean 14-line addition in the right place (after data fetch, before TWAP computation). Error message includes actionable guidance. No issues found.
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The script-level minimum sample guard is in the right place, but the PR does not satisfy the issue acceptance criteria for settlement-integrity coverage. This code gates final airdrop settlement math, so the boundary behavior needs automated verification rather than inspection only.
Findings
-
[medium] Missing TWAP sample-count tests required by #1304 acceptance criteria.
- File:
scripts/airdrop-finalize.ts:37 - Suggestion: Add focused tests for the requested cases: 1 sample throws without
AIRDROP_FINALIZE_ALLOW_PARTIAL_TWAP, 5 samples proceeds and warns, 7 samples proceeds cleanly, and the override path permits <5 samples while still warning. If this script is hard to test directly, extract the sample-count validation into a small pure helper and cover that helper.
- File:
-
[low] Override documentation acceptance item is not addressed in the PR.
- File:
scripts/airdrop-finalize.ts:52 - Suggestion: Document
AIRDROP_FINALIZE_ALLOW_PARTIAL_TWAP=1in the relevant T6.1/operator settlement notes, or add an explicit PR note/comment linking where that emergency override is documented.
- File:
Decision
Request changes. The main guard looks correct, but #1304 explicitly requires tests for the settlement boundary cases and documentation of the emergency override.
Extract validateTwapSamples to lib/airdrop/twap.ts for testability. 7 tests: 0 samples rejects, 1/4 rejects without override, 1 with override succeeds, 5/6 warn, 7 clean. Finalize script uses helper. Override env documented in airdrop-contracts.md settlement section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested #1304 follow-up is now covered: TWAP sample validation is extracted into a focused helper, the finalize script uses it, boundary tests cover the insufficient/override/warning/clean cases, and the emergency override is documented for settlement.
Findings
- None.
Decision
Approve. CI is green for lint/typecheck/unit tests and E2E on run 26452072662.
2 participants
Summary
AIRDROP_FINALIZE_ALLOW_PARTIAL_TWAP=1env var for operator to proceed with partial dataVersion
1.41.0 → 1.41.1
Closes #1304
🤖 Generated with Claude Code