fix: nonce, s3 config overrides #203
Conversation
WalkthroughIntroduces chain-specific config overrides in buildConfig, updates package version and dependencies, and adds a batching function to the CounterAppGateway test contract that repeatedly invokes existing increment logic. Changes
Sequence Diagram(s)sequenceDiagram
actor Tester
participant Gateway as CounterAppGateway
participant InstanceA as Counter Instance(s)
Tester->>Gateway: incrementCountersMultiple(instances[], count)
loop count times
Gateway->>Gateway: incrementCounters(instances[])
par For each address in instances[]
Gateway->>InstanceA: increment()
Note over Gateway,InstanceA: Uses existing logic and onIncrementComplete flow
end
end
Gateway-->>Tester: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
hardhat-scripts/s3Config/buildConfig.ts (1)
18-23: Document the override values.The eventBlockRange and eventBlockRangePerCron are reduced from 5000 to 100 and 200 respectively for EVMX_CHAIN_ID. Add a comment explaining why these specific values are chosen (e.g., chain characteristics, performance requirements, testing constraints).
+// EVMX chain requires smaller block ranges due to [reason] const CHAIN_CONFIG_OVERRIDES: Record<number, Partial<ChainConfig>> = { [EVMX_CHAIN_ID]: { + // Reduced from default 5000 to avoid [specific issue] eventBlockRange: 100, + // Adjusted for EVMX chain's [characteristic] eventBlockRangePerCron: 200, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
hardhat-scripts/s3Config/buildConfig.ts(2 hunks)package.json(3 hunks)test/apps/app-gateways/counter/CounterAppGateway.sol(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hardhat-scripts/s3Config/buildConfig.ts (2)
src/types.ts (1)
ChainConfig(61-70)hardhat-scripts/config/config.ts (1)
EVMX_CHAIN_ID(100-100)
🔇 Additional comments (4)
package.json (3)
12-12: LGTM on version bump.Standard patch version increment from 1.1.37 to 1.1.39.
36-36: LGTM on @types/pg addition.The types package correctly supports the existing
pgdevDependency at line 48.
57-59: Remove unused uuid dependency. No import or require usage found; if it's not referenced elsewhere (e.g., tests, scripts), remove it from package.json to avoid unnecessary bloat.hardhat-scripts/s3Config/buildConfig.ts (1)
58-58: LGTM on override application.The spread operator correctly merges chain-specific overrides after defaults, allowing per-chain customization of eventBlockRange and eventBlockRangePerCron values.
| function incrementCountersMultiple(address[] memory instances_, uint256 count) public { | ||
| for (uint256 i = 0; i < count; i++) { | ||
| incrementCounters(instances_); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical race condition on async state.
This function calls incrementCounters (an async function) multiple times in a loop without awaiting completion. Each iteration overwrites:
onCompleteData(line 64 inincrementCounters)incrementedflag (line 58 inincrementCounters)
This causes nondeterministic behavior where only the last callback is registered, and intermediate async operations may not complete as expected.
This should either:
- Add the
asyncmodifier and use proper sequencing/batching - Document that only the final iteration's callback is intentional
- Redesign to track completion of all batched operations
Additionally, the count parameter is unbounded, creating a gas DoS vector.
Apply this diff to add validation:
function incrementCountersMultiple(address[] memory instances_, uint256 count) public {
+ require(count > 0 && count <= 100, "Invalid count");
for (uint256 i = 0; i < count; i++) {
incrementCounters(instances_);
}
}📝 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.
| function incrementCountersMultiple(address[] memory instances_, uint256 count) public { | |
| for (uint256 i = 0; i < count; i++) { | |
| incrementCounters(instances_); | |
| } | |
| } | |
| function incrementCountersMultiple(address[] memory instances_, uint256 count) public { | |
| require(count > 0 && count <= 100, "Invalid count"); | |
| for (uint256 i = 0; i < count; i++) { | |
| incrementCounters(instances_); | |
| } | |
| } |
🤖 Prompt for AI Agents
In test/apps/app-gateways/counter/CounterAppGateway.sol around lines 67–71, the
loop calling incrementCounters repeatedly creates a race where onCompleteData
and the incremented flag are overwritten by concurrent async callbacks; fix by
making the function sequence or aggregate operations instead of fire-and-forget:
either add an async modifier and await each incrementCounters call (or refactor
incrementCounters to accept a batch/count and process all items internally), or
change incrementCounters to return a per-call completion token and track
completion for each token so callbacks don’t clobber shared state; additionally
add input validation to prevent unbounded loops by introducing a MAX_BATCH
constant and require(count > 0 && count <= MAX_BATCH) at the start of the
function.
2 participants
Summary by CodeRabbit
New Features
Chores