Conversation
Cover ensureHex, splitBy, iso8601ToTimestamp, sumOf, zip, blockWindows, requireEnv, requiredArgs, and assertCondition. Made-with: Cursor
Test isKnownModality, verify EVMModality enum ordinals match the Solidity enum, and cross-check MODALITIES vesting timestamps against VESTING_PARAMS_FOR_MODALITY parameters. Export MODALITIES to enable the cross-check. Made-with: Cursor
Move the pure deduplication logic out of the tde.ts entry script so it can be imported without triggering side effects. Tests cover count-based matching, partial matches, and key discrimination by address, modality, and amount. Made-with: Cursor
Cover valid parsing, modality mapping, unknown modality rejection, invalid bigint handling, empty CSV, whitespace trimming, and address checksumming using temp files. Made-with: Cursor
Export both functions from batch.ts and test EIP-7702 delegation code matching and error classification for retry logic. Made-with: Cursor
Export both functions from abiChecker.ts and test signature generation for functions, events, and other types, plus artifact matching, missing name detection, and signature mismatch errors. Made-with: Cursor
Move the entry building logic (bid grouping, whale/normal split, modality assignment, sweep handling) into ccaEntries.ts so it can be tested without RPC side effects. Tests cover single/multi-owner bids, whale-only/normal-only/mixed, dust amounts, sweep appending, and pre-bonus CCA amount tracking. Made-with: Cursor
Cover single-page, multi-page concatenation, empty results, and single-block range. Made-with: Cursor
- Remove narrating comment from tde.ts - Rename tdePending.ts to findPendingRows.ts for consistency - Use beforeEach/afterEach in csv.test.ts instead of manual setup() - Add concurrency batching test for paginatedGetEvents - Remove low-value assertCondition and sumOf tests - Note hardcoded Solidity params limitation in modalities.test.ts - Clean up EVMModality import in ccaEntries.ts (drop the M alias) Made-with: Cursor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughConsolidates and reorganizes initial-distribution utilities: moves block-search into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/initial-distribution/src/abiChecker.ts (1)
108-116:⚠️ Potential issue | 🟡 MinorAdd
forge buildto initial-distribution job or conditionally skip ABI validation in tests.The
initial-distributionCI job runspnpm test, which importsbatch.ts, triggering the module-level assertion inabis.tsat line 109. This assertion fails because it looks for artifacts in the repository root'sout/directory (created byforge build), but theinitial-distributionjob never runsforge build.Either add
forge buildto theinitial-distributionjob beforepnpm test, or make theassertAbisMatchArtifactscall conditional to skip in test environments where build artifacts may not be available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/initial-distribution/src/abiChecker.ts` around lines 108 - 116, The CI failure comes from the module-level assertion in assertAbisMatchArtifacts (abis.ts) that throws when missing artifacts; make the ABI validation conditional so tests/this job can run without a forge build: update the call site (where assertAbisMatchArtifacts is invoked, e.g., from batch.ts or module initialization in abis.ts) to skip the check when running under test/CI without artifacts by checking an environment flag (e.g., process.env.SKIP_ABI_CHECK === "1" or NODE_ENV === "test") or a CI-specific var, and only call assertAbisMatchArtifacts when that flag is not set; alternatively, expose and check a new helper function (isAbiCheckEnabled) and use it to guard the existing throw logic so the error block (the throw new Error([...].join("\n")) in abiChecker.ts) is not executed during the initial-distribution job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/initial-distribution/src/csv.test.ts`:
- Line 35: Replace the magic numeric modality assertions with named constants or
the mapping lookup: import MODALITIES_TS_TO_EVM (or exported individual
constants) from the source that defines the modality mapping and assert using
those values (e.g. assert.equal(rows[0].modality, MODALITIES_TS_TO_EVM.DIRECT)
instead of 0); do the same for the other assertions (currently using 1 and 7)
referencing the appropriate keys, and update any test imports at the top of
csv.test.ts accordingly so tests remain self-documenting and resilient to
mapping changes.
---
Outside diff comments:
In `@script/initial-distribution/src/abiChecker.ts`:
- Around line 108-116: The CI failure comes from the module-level assertion in
assertAbisMatchArtifacts (abis.ts) that throws when missing artifacts; make the
ABI validation conditional so tests/this job can run without a forge build:
update the call site (where assertAbisMatchArtifacts is invoked, e.g., from
batch.ts or module initialization in abis.ts) to skip the check when running
under test/CI without artifacts by checking an environment flag (e.g.,
process.env.SKIP_ABI_CHECK === "1" or NODE_ENV === "test") or a CI-specific var,
and only call assertAbisMatchArtifacts when that flag is not set; alternatively,
expose and check a new helper function (isAbiCheckEnabled) and use it to guard
the existing throw logic so the error block (the throw new
Error([...].join("\n")) in abiChecker.ts) is not executed during the
initial-distribution job.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
script/initial-distribution/src/abiChecker.test.tsscript/initial-distribution/src/abiChecker.tsscript/initial-distribution/src/batch.test.tsscript/initial-distribution/src/batch.tsscript/initial-distribution/src/cca.tsscript/initial-distribution/src/ccaEntries.test.tsscript/initial-distribution/src/ccaEntries.tsscript/initial-distribution/src/csv.test.tsscript/initial-distribution/src/findFirstBlockAtOrAfter.test.tsscript/initial-distribution/src/findFirstBlockAtOrAfter.tsscript/initial-distribution/src/findPendingRows.test.tsscript/initial-distribution/src/findPendingRows.tsscript/initial-distribution/src/lib.test.tsscript/initial-distribution/src/lib.tsscript/initial-distribution/src/modalities.test.tsscript/initial-distribution/src/modalities.tsscript/initial-distribution/src/tde.ts
💤 Files with no reviewable changes (2)
- script/initial-distribution/src/findFirstBlockAtOrAfter.test.ts
- script/initial-distribution/src/findFirstBlockAtOrAfter.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-01T13:52:37.088Z
Learnt from: pkoch
Repo: idos-network/contracts PR: 19
File: script/initial-distribution/src/tde.ts:38-48
Timestamp: 2026-03-01T13:52:37.088Z
Learning: In script/initial-distribution, when using getContract({ client: walletClient }), it is possible to call read.* methods on the resulting contract even if walletClient is not explicitly extended with publicActions (as observed in tde.ts and cca.ts with viem 2.23.x). Treat this as an intentional pattern to verify: - confirm that the behavior is supported by the specific viem version and contract bindings, - document any assumptions about publicActions not being required for read operations, - ensure there are no security implications (e.g., unintended read access) and that access controls are still correct, - consider adding comments or tests that demonstrate expected usage and edge cases, and - if this behavior is relied upon in other files, apply the same pattern consistently within the script/initial-distribution directory.
Applied to files:
script/initial-distribution/src/modalities.test.tsscript/initial-distribution/src/lib.tsscript/initial-distribution/src/abiChecker.test.tsscript/initial-distribution/src/abiChecker.tsscript/initial-distribution/src/findPendingRows.tsscript/initial-distribution/src/csv.test.tsscript/initial-distribution/src/ccaEntries.tsscript/initial-distribution/src/batch.test.tsscript/initial-distribution/src/lib.test.tsscript/initial-distribution/src/batch.tsscript/initial-distribution/src/findPendingRows.test.tsscript/initial-distribution/src/cca.tsscript/initial-distribution/src/modalities.tsscript/initial-distribution/src/ccaEntries.test.tsscript/initial-distribution/src/tde.ts
📚 Learning: 2026-02-23T19:57:43.946Z
Learnt from: pkoch
Repo: idos-network/contracts PR: 13
File: src/WhaleDisburser.sol:23-28
Timestamp: 2026-02-23T19:57:43.946Z
Learning: In src/WhaleDisburser.sol, the disburse function intentionally has no access control. Anyone can call disburse(IERC20 token, address beneficiary, uint256 totalAmount, uint64 vestingStart), which is by design. The function uses safeTransferFrom(msg.sender, ...) so it only pulls from the caller's own balance, and no external funds are at risk.
Applied to files:
script/initial-distribution/src/tde.ts
🧬 Code graph analysis (9)
script/initial-distribution/src/modalities.test.ts (2)
script/initial-distribution/src/modalities.ts (5)
MODALITIES(2-17)isKnownModality(52-54)EVMModality(21-32)EVMModality(33-33)MODALITIES_TS_TO_EVM(35-50)script/initial-distribution/src/lib.ts (1)
iso8601ToTimestamp(44-51)
script/initial-distribution/src/abiChecker.test.ts (1)
script/initial-distribution/src/abiChecker.ts (2)
abiItemSignature(18-27)checkAbiAgainstArtifact(29-72)
script/initial-distribution/src/findPendingRows.ts (1)
script/initial-distribution/src/csv.ts (1)
DisbursementRow(11-15)
script/initial-distribution/src/csv.test.ts (1)
script/initial-distribution/src/csv.ts (1)
loadDisbursementCsv(34-55)
script/initial-distribution/src/batch.test.ts (1)
script/initial-distribution/src/batch.ts (2)
isDelegatedTo(46-51)isExecutionRevert(27-36)
script/initial-distribution/src/lib.test.ts (1)
script/initial-distribution/src/lib.ts (9)
requireEnv(3-7)ensureHex(10-16)requiredArgs(20-31)splitBy(33-40)iso8601ToTimestamp(44-51)zip(61-68)blockWindows(118-131)paginatedGetEvents(135-149)findFirstBlockAtOrAfter(86-102)
script/initial-distribution/src/findPendingRows.test.ts (3)
script/initial-distribution/src/modalities.ts (2)
EVMModality(21-32)EVMModality(33-33)script/initial-distribution/src/csv.ts (1)
DisbursementRow(11-15)script/initial-distribution/src/findPendingRows.ts (1)
findPendingRows(8-29)
script/initial-distribution/src/cca.ts (1)
script/initial-distribution/src/ccaEntries.ts (1)
buildExpectedEntries(25-85)
script/initial-distribution/src/ccaEntries.test.ts (2)
script/initial-distribution/src/ccaEntries.ts (3)
FilledBid(14-18)buildExpectedEntries(25-85)Sweep(20-23)script/initial-distribution/src/modalities.ts (2)
EVMModality(21-32)EVMModality(33-33)
🪛 GitHub Actions: CI
script/initial-distribution/src/abiChecker.ts
[error] 109-109: Missing artifact(s). Run forge build from the repo root. Missing: - CCA - Tracker - ERC20 - TDEDisbursement - BatchCaller
script/initial-distribution/src/batch.test.ts
[error] 1-1: Test failed. See logs above for details.
🔇 Additional comments (18)
script/initial-distribution/src/csv.test.ts (2)
1-26: LGTM! Clean test setup with proper isolation.The imports are minimal and appropriate. The temp directory setup with
beforeEach/afterEachensures test isolation, and the cleanup withforce: truehandles edge cases gracefully.
66-87: LGTM! Good coverage of edge cases.The tests for header-only CSV, whitespace trimming, and address checksumming are well-constructed. The whitespace test correctly exercises the
trim: trueparser option, and the checksum test properly verifies EIP-55 normalization.script/initial-distribution/src/modalities.ts (1)
2-2: LGTM!Exporting
MODALITIESenables direct test access and external consumption while maintaining the same data structure. This aligns well with the PR's goal of adding test coverage.script/initial-distribution/src/batch.ts (1)
27-36: LGTM!Exporting
isExecutionRevertandisDelegatedToenables unit testing of these utility functions. The implementations are sound:
isExecutionRevertproperly guards forErrorinstances before message inspectionisDelegatedTocorrectly validates EIP-7702 delegation code format with proper case normalizationAlso applies to: 46-51
script/initial-distribution/src/lib.ts (1)
86-102: LGTM!The binary search implementation correctly finds the first block with timestamp ≥
targetTimestamp. The algorithm is a standard leftmost binary search that convergesloto the answer.Note: When
targetTimestampexceeds all block timestamps in the range, this returnshi(the last block). The test suite confirms this is intentional behavior.script/initial-distribution/src/lib.test.ts (1)
1-323: Excellent test coverage!Comprehensive test suite covering all exported utilities from
lib.js. The tests properly:
- Handle environment variable cleanup with try/finally
- Cover edge cases (empty inputs, boundary conditions, error states)
- Use mock functions effectively for async operations
- Verify concurrency limits in
paginatedGetEvents- Exercise the binary search thoroughly with various timestamp scenarios
script/initial-distribution/src/modalities.test.ts (1)
1-106: Well-designed cross-language verification tests!The test suite provides valuable guardrails:
isKnownModalitytype guard coverage is thorough- EVMModality enum verification ensures TypeScript stays in sync with Solidity's
enum Modality- MODALITIES timestamp validation against hardcoded Solidity
VESTING_PARAMS_FOR_MODALITYcatches drift earlyThe comment at lines 50-53 appropriately documents the manual maintenance requirement for the Solidity params table.
script/initial-distribution/src/tde.ts (1)
2-2: Clean modularization!Extracting
findPendingRowsto a dedicated module improves testability and follows single responsibility principle. The import changes are appropriate, and the usage remains functionally identical.Also applies to: 14-14
script/initial-distribution/src/findPendingRows.test.ts (1)
1-64: Thorough test coverage for the count-based matching algorithm!The tests effectively verify:
- Empty/full match scenarios
- Partial matches with correct pending row identification
- Count-based duplicate handling (critical for idempotent re-runs)
- Independence of matching on address, modality, and amount dimensions
The helper functions
row()andlog()improve test readability.script/initial-distribution/src/batch.test.ts (1)
1-60: Tests are logically sound, but the CI failure is due to missing dependencies, not test issues.The test coverage for
isDelegatedToandisExecutionRevertis comprehensive and correct—the implementations match the test expectations exactly. The EIP-7702 prefix validation, address comparison, error message matching, and type checking are all properly tested.However, the test failure is caused by a missing
viempackage dependency in the environment, not by incorrect test logic. Ensure dependencies are installed before running tests (npm install).script/initial-distribution/src/abiChecker.test.ts (1)
1-98: LGTM! Comprehensive test coverage for ABI checker utilities.The test suite covers the key scenarios well:
- Function and event signature generation
- Fallback to JSON for other types
- Subset and exact matching validation
- Error handling for missing entries and signature mismatches
- Edge case for unnamed entries (constructors)
script/initial-distribution/src/cca.ts (1)
5-5: LGTM! Clean extraction of entry-building logic to dedicated module.The refactor correctly:
- Imports
buildExpectedEntriesandDisbursementEntryfrom the newccaEntriesmodule- Consolidates
findFirstBlockAtOrAfterimport throughlib.js- Replaces inline expected entries construction with a single call to the extracted helper
The data structures (
filledBids,sweep) align with the expected interfaces inccaEntries.ts.Also applies to: 12-12, 236-236
script/initial-distribution/src/findPendingRows.ts (2)
8-28: LGTM! Clean extraction with correct counting logic.The algorithm correctly handles:
- Multiple identical disbursements (via count decrementing)
- Rows without matching logs (added to pending)
- Efficient O(n+m) time complexity
4-6: Address normalization is already handled correctly.The composite key uses string concatenation of addresses, which is safe because both data sources already provide checksummed addresses. The CSV loader explicitly uses
getAddress()to normalize addresses (csv.ts:51), and viem'sgetContractEventsreturns checksummed addresses by default. The test suite confirms this works as expected with matching checksummed addresses.No action required.
script/initial-distribution/src/abiChecker.ts (1)
18-18: LGTM! Exporting utilities enables unit testing.The visibility change from internal to exported for
abiItemSignatureandcheckAbiAgainstArtifactis appropriate to enable the new test coverage.Also applies to: 29-29
script/initial-distribution/src/ccaEntries.test.ts (1)
1-149: LGTM! Excellent test coverage for entry building logic.The test suite thoroughly validates:
- Entry generation for different bid types (whale/normal/mixed)
- Whale bonus calculation (1.2x multiplier verified at lines 35-38, 146-147)
- Sorting by owner address
- Edge cases: dust amounts, boundary block classification, zero sweep
- Aggregation of multiple bids from same owner
- Proper separation of
ccaAmountvstransferAmountThe helper functions
whaleBidandnormalBidmake tests readable and maintainable.script/initial-distribution/src/ccaEntries.ts (2)
1-85: LGTM! Well-structured module with clear separation of concerns.The implementation:
- Correctly groups and sorts bids by owner for deterministic output
- Properly classifies bids as whale (
< phaseBoundaryBlock) or normal (>= phaseBoundaryBlock)- Conditionally creates entries only when amounts are positive (avoiding zero-amount entries)
- Separates
ccaAmount(pre-bonus) fromtransferAmount(post-bonus) for tracking purposes
2-2: Module exists with correct exports, but parameter names differ from stated.The
computeDisbursementmodule exists and exports correctly, but the function signature usescomputeDisbursement(ccaWhale: bigint, ccaNormal: bigint)rather than thewhaleTokensandnormalTokensparameter names mentioned. The return type is properly typed asBidderDisbursementinterface and includes all expected properties:ccaWhaleImmediate,ccaWhaleVested,disbursableWhaleImmediate,disbursableWhaleVested,ccaNormal, anddisbursableNormal.Likely an incorrect or invalid review comment.
pkoch
force-pushed
the
feat/add-reasonable-test-coverage
branch
2 times, most recently
from
a827480 to
d2e144f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
script/initial-distribution/src/abiChecker.ts (1)
62-70: 🧹 Nitpick | 🔵 TrivialRedundant check after
findalready matched by signature.The
findon line 52 locates a candidate whereabiItemSignature(candidate) === expected. Ifbuiltis found, thenobserved(line 62) is guaranteed to equalexpected, making this conditional unreachable.♻️ Proposed simplification
const built = builtCandidates.find((candidate) => abiItemSignature(candidate) === expected); if (!built) { throw new Error( [ `${key} signature differs:`, ` expected (abis.ts): ${expected}`, ` observed candidates (artifact): ${builtCandidates.map((candidate) => abiItemSignature(candidate)).join(", ")}`, ].join("\n"), ); } - const observed = abiItemSignature(built); - if (expected !== observed) - throw new Error( - [ - `${key} signature differs:`, - ` expected (abis.ts): ${expected}`, - ` observed (artifact): ${observed}`, - ].join("\n"), - ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/initial-distribution/src/abiChecker.ts` around lines 62 - 70, The conditional comparing expected vs observed after locating built with the find that already matched by abiItemSignature is redundant; remove the `observed` calculation and the if/throw block and instead assert presence of the `built` candidate (e.g., throw a clear error if `built` is undefined) so the code relies on the fact that `find` returned a matching item; reference the `find` call that produces `built`, the `abiItemSignature` helper, and the variables `expected`/`built` when making the minimal change.script/initial-distribution/src/tde.ts (1)
39-49: 🧹 Nitpick | 🔵 TrivialRefactor to use viem's documented patterns for contract read operations.
This code relies on
getContract({ client: walletClient })to call.read.*methods, but viem's official documentation requires Public Actions for read operations. According to viem docs, you should either:
Extend walletClient with publicActions:
const walletClient = createWalletClient({ account, chain, transport }) .extend(publicActions)Or pass both clients to getContract:
getContract({ address, abi, client: { public: publicClient, wallet: walletClient } })While this currently works in viem 2.23.x, relying on undocumented behavior risks silent breakage in future viem upgrades. Aligning with viem's documented patterns will protect against this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/initial-distribution/src/tde.ts` around lines 39 - 49, The getContract usage relies on walletClient providing public read actions; update the code to follow viem docs by either (A) creating walletClient with public actions via createWalletClient(...).extend(publicActions) so tdeDisbursementContract.read.IDOS_TOKEN() uses documented public methods, or (B) call getContract with both clients by passing client: { public: publicClient, wallet: walletClient } so tdeDisbursementContract and tokenContract use the correct publicClient for reads; update the tdeDisbursementContract and tokenContract creation accordingly and ensure publicClient/publicActions symbols are used where read calls occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/initial-distribution/src/abiChecker.ts`:
- Around line 108-109: Add a short inline comment above the early return that
explains why the validation is skipped when process.env.CI is truthy and
present.length === 0: note this bypass allows CI runs that don't build artifacts
(test-only jobs) to pass without flagging ABI drift, but also call out the
trade-off (risk of masking real ABI changes) and suggest when to remove or
tighten the condition. Place the comment next to the if block referencing
missing, present, and the CI check so future maintainers understand the intent.
In `@script/initial-distribution/src/findPendingRows.test.ts`:
- Around line 21-62: Tests in findPendingRows.test.ts use raw modality numbers
(e.g., 0 and 1) in the test fixtures passed to row(...) and log(...); replace
those numeric literals with the corresponding EVMModality constants to make
tests clearer and robust. Update every call to row(...) and log(...) in the file
that currently uses 0 or 1 to use the matching EVMModality.<CONSTANT_FOR_0> and
EVMModality.<CONSTANT_FOR_1> (as defined in your EVMModality enum) so the tests
(including the cases referencing row, log, and the findPendingRows assertions)
use the named constants instead of raw numbers.
---
Outside diff comments:
In `@script/initial-distribution/src/abiChecker.ts`:
- Around line 62-70: The conditional comparing expected vs observed after
locating built with the find that already matched by abiItemSignature is
redundant; remove the `observed` calculation and the if/throw block and instead
assert presence of the `built` candidate (e.g., throw a clear error if `built`
is undefined) so the code relies on the fact that `find` returned a matching
item; reference the `find` call that produces `built`, the `abiItemSignature`
helper, and the variables `expected`/`built` when making the minimal change.
In `@script/initial-distribution/src/tde.ts`:
- Around line 39-49: The getContract usage relies on walletClient providing
public read actions; update the code to follow viem docs by either (A) creating
walletClient with public actions via
createWalletClient(...).extend(publicActions) so
tdeDisbursementContract.read.IDOS_TOKEN() uses documented public methods, or (B)
call getContract with both clients by passing client: { public: publicClient,
wallet: walletClient } so tdeDisbursementContract and tokenContract use the
correct publicClient for reads; update the tdeDisbursementContract and
tokenContract creation accordingly and ensure publicClient/publicActions symbols
are used where read calls occur.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
script/initial-distribution/src/abiChecker.tsscript/initial-distribution/src/batch.test.tsscript/initial-distribution/src/cca.tsscript/initial-distribution/src/ccaEntries.test.tsscript/initial-distribution/src/ccaEntries.tsscript/initial-distribution/src/csv.test.tsscript/initial-distribution/src/findFirstBlockAtOrAfter.test.tsscript/initial-distribution/src/findFirstBlockAtOrAfter.tsscript/initial-distribution/src/findPendingRows.test.tsscript/initial-distribution/src/lib.test.tsscript/initial-distribution/src/lib.tsscript/initial-distribution/src/modalities.tsscript/initial-distribution/src/tde.ts
💤 Files with no reviewable changes (2)
- script/initial-distribution/src/findFirstBlockAtOrAfter.ts
- script/initial-distribution/src/findFirstBlockAtOrAfter.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-01T13:52:37.088Z
Learnt from: pkoch
Repo: idos-network/contracts PR: 19
File: script/initial-distribution/src/tde.ts:38-48
Timestamp: 2026-03-01T13:52:37.088Z
Learning: In script/initial-distribution, when using getContract({ client: walletClient }), it is possible to call read.* methods on the resulting contract even if walletClient is not explicitly extended with publicActions (as observed in tde.ts and cca.ts with viem 2.23.x). Treat this as an intentional pattern to verify: - confirm that the behavior is supported by the specific viem version and contract bindings, - document any assumptions about publicActions not being required for read operations, - ensure there are no security implications (e.g., unintended read access) and that access controls are still correct, - consider adding comments or tests that demonstrate expected usage and edge cases, and - if this behavior is relied upon in other files, apply the same pattern consistently within the script/initial-distribution directory.
Applied to files:
script/initial-distribution/src/findPendingRows.test.tsscript/initial-distribution/src/ccaEntries.tsscript/initial-distribution/src/lib.tsscript/initial-distribution/src/tde.tsscript/initial-distribution/src/batch.test.tsscript/initial-distribution/src/abiChecker.tsscript/initial-distribution/src/lib.test.tsscript/initial-distribution/src/cca.tsscript/initial-distribution/src/modalities.tsscript/initial-distribution/src/csv.test.tsscript/initial-distribution/src/ccaEntries.test.ts
📚 Learning: 2026-02-23T19:57:43.946Z
Learnt from: pkoch
Repo: idos-network/contracts PR: 13
File: src/WhaleDisburser.sol:23-28
Timestamp: 2026-02-23T19:57:43.946Z
Learning: In src/WhaleDisburser.sol, the disburse function intentionally has no access control. Anyone can call disburse(IERC20 token, address beneficiary, uint256 totalAmount, uint64 vestingStart), which is by design. The function uses safeTransferFrom(msg.sender, ...) so it only pulls from the caller's own balance, and no external funds are at risk.
Applied to files:
script/initial-distribution/src/tde.ts
🧬 Code graph analysis (6)
script/initial-distribution/src/findPendingRows.test.ts (3)
script/initial-distribution/src/modalities.ts (2)
EVMModality(21-32)EVMModality(33-33)script/initial-distribution/src/csv.ts (1)
DisbursementRow(11-15)script/initial-distribution/src/findPendingRows.ts (1)
findPendingRows(8-29)
script/initial-distribution/src/batch.test.ts (1)
script/initial-distribution/src/batch.ts (2)
isDelegatedTo(46-51)isExecutionRevert(27-36)
script/initial-distribution/src/lib.test.ts (1)
script/initial-distribution/src/lib.ts (9)
requireEnv(3-7)ensureHex(10-16)requiredArgs(20-31)splitBy(33-40)iso8601ToTimestamp(44-51)zip(61-68)blockWindows(118-131)paginatedGetEvents(135-149)findFirstBlockAtOrAfter(86-102)
script/initial-distribution/src/cca.ts (1)
script/initial-distribution/src/ccaEntries.ts (1)
buildExpectedEntries(25-85)
script/initial-distribution/src/csv.test.ts (1)
script/initial-distribution/src/csv.ts (1)
loadDisbursementCsv(34-55)
script/initial-distribution/src/ccaEntries.test.ts (2)
script/initial-distribution/src/ccaEntries.ts (3)
FilledBid(14-18)buildExpectedEntries(25-85)Sweep(20-23)script/initial-distribution/src/modalities.ts (2)
EVMModality(21-32)EVMModality(33-33)
🔇 Additional comments (13)
script/initial-distribution/src/abiChecker.ts (1)
18-27: LGTM!The export of
abiItemSignatureis appropriate for enabling unit testing. The function correctly delegates to viem'stoFunctionSignatureandtoEventSignaturefor the respective types, with a reasonableJSON.stringifyfallback for other ABI entry types.script/initial-distribution/src/modalities.ts (1)
2-2: LGTM!Clean export addition enabling external modules and tests to import
MODALITIESdirectly.script/initial-distribution/src/batch.test.ts (1)
1-60: LGTM!Comprehensive test coverage for
isDelegatedToandisExecutionRevertutilities. Tests correctly exercise edge cases including undefined/empty inputs, prefix mismatches, and theContractFunctionRevertedErrorconstructor name check.script/initial-distribution/src/csv.test.ts (2)
35-35: Consider using named constants for modality assertions.The magic numbers (0, 1, 7) with inline comments work but importing
MODALITIES_TS_TO_EVMorEVMModalitywould make tests more maintainable if mappings change.
1-88: LGTM!Solid test coverage for CSV loading: parsing, modality mapping, error handling, whitespace trimming, and address checksumming. Good use of temp directory setup/teardown.
script/initial-distribution/src/ccaEntries.ts (1)
25-85: LGTM!Clean implementation of
buildExpectedEntries. The logic correctly:
- Deduplicates and sorts owners
- Splits bids into whale (before boundary) and normal (at/after boundary)
- Emits entries only when amounts are positive
- Appends sweep entry conditionally
The upstream caller (
cca.ts) normalizes addresses withgetAddress()before passing to this function, so the string-based Set deduplication and sort are safe.script/initial-distribution/src/lib.ts (1)
86-102: LGTM!Correct lower-bound binary search implementation for finding the first block at or after a target timestamp. The edge case where the target is past all blocks (returns
hi) is documented in the test suite.script/initial-distribution/src/lib.test.ts (1)
1-323: LGTM!Comprehensive and well-structured test suite covering all exported utilities from
lib.js. Highlights:
- Proper environment variable cleanup with try/finally
- Good edge case coverage (empty inputs, boundary conditions, invalid formats)
- Effective mocking for async functions (
paginatedGetEvents,findFirstBlockAtOrAfter)- Concurrency limit verification for
paginatedGetEventsscript/initial-distribution/src/ccaEntries.test.ts (1)
1-149: LGTM!Excellent test coverage for
buildExpectedEntries:
- Clear helper functions (
whaleBid,normalBid) that encode boundary semantics- Validates whale bonus calculation (600n → 720n = 20% bonus)
- Correctly tests boundary condition (exact boundary block → normal, not whale)
- Good dust handling test with clear calculation comment
- Comprehensive sweep and empty-input scenarios
script/initial-distribution/src/cca.ts (2)
5-5: LGTM!Clean refactor extracting entry-building logic to a dedicated, testable module.
236-236: LGTM!The
filledBidsarray structure (withowner,bidBlockNumber,tokensFilled) aligns with theFilledBidinterface, andsweepmatches theSweepinterface. This single-line replacement maintains the same behavior while enabling comprehensive unit testing of the entry-building logic in isolation.script/initial-distribution/src/findPendingRows.test.ts (1)
19-64: Strong scenario coverage forfindPendingRows.This suite exercises the key matching and counting paths (including duplicate accounting) and gives good confidence in the extraction.
script/initial-distribution/src/tde.ts (1)
14-14: Good extraction to shared pending-row logic.Importing
findPendingRowsfrom the dedicated module removes duplication and keeps this script focused on orchestration.
pkoch
force-pushed
the
feat/add-reasonable-test-coverage
branch
from
d2e144f to
3db83e2
Compare
1 participant
Summary by CodeRabbit
Tests
Refactor
New Features