Repository navigation
Validate simulate srcAddress against wildcard registrations - #1357
Conversation
SimulateItems.validateSrcAddresses ran in the main thread against the
config from Config.loadWithoutRegistrations(), whose isWildcard reflects
config.yaml only. Events made wildcard via indexer.onEvent({ wildcard:
true }) read back as non-wildcard there, so createTestIndexer().process()
threw "resolved to address ... which isn't indexed" for a no-address
contract whose events are wildcard-handled. The main thread can't know
handler-level wildcard flags: registrations are applied inside the worker
and a real user never loads/flushes them in the test thread.
Move the srcAddress validation into the worker's patchConfig, where the
config already has registrations applied (so isWildcard is correct) and
the per-chain indexing addresses are available from initialState (matched
by chainId).
Make Main.start reject its promise on any fatal error instead of handling
the consequence inline: onError rejects a deferred (runUntilFatalError)
that start awaits, for both test and non-test runs. The caller logs and
decides what to do — Bin.res's catch logs the cause and exits with a
failure code; the test worker forwards the rejection to the parent via the
worker error event. This unifies setup and runtime failures onto one
channel (a rejected Main.start promise), drops the test-only setImmediate
from Main.start, and leaves logging to the caller so a fatal error is
logged once. A clean run leaves the deferred pending; the process still
exits via ExitOnCaughtUp (success) or naturally when the loop drains.
The pre-flight check was synchronous, so the existing "isn't indexed" test
now awaits a rejected promise.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0175WwoErWZNsba95RnrxRyb
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesWildcard simulate validation fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/envio/src/TestIndexer.res`:
- Around line 878-887: The preflight in TestIndexer.validateSrcAddresses is
using only initialState.chains indexingAddresses, so it misses addresses
registered by earlier simulated items in the same batch. Update the
simulate-batch validation flow so address checks happen incrementally as items
are applied, or otherwise use the evolving per-chain indexingAddresses state
before validating each simulated item. Keep the fix localized around
SimulateItems.validateSrcAddresses and the logic that reads from
initialState.chains so later items can see registrations produced earlier in the
run.
In `@scenarios/test_codegen/test/WildcardSimulate_test.res`:
- Line 36: The wildcard simulation tests are using t.expect on only
result.changes.length, which does not verify the full returned value and
violates the test guidelines. Update the affected assertions in
WildcardSimulate_test.res to use Assert directly on the returned value from the
wildcard simulation helpers, and replace the field-level checks in both test
cases with a single whole-value assertion so the expected result is validated
end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e9a0d9c-fc86-40f4-bea1-bfef6efae800
📒 Files selected for processing (5)
packages/envio/src/Main.respackages/envio/src/SimulateItems.respackages/envio/src/TestIndexer.resscenarios/test_codegen/test/EventHandler.test.tsscenarios/test_codegen/test/WildcardSimulate_test.res
| let indexingAddresses = | ||
| initialState.chains | ||
| ->Array.find(c => c.id == chainId) | ||
| ->Option.mapOr([], c => c.indexingAddresses) | ||
| SimulateItems.validateSrcAddresses( | ||
| ~simulateItems, | ||
| ~config, | ||
| ~chainConfig, | ||
| ~indexingAddresses, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This still rejects simulate batches that register an address earlier in the same run.
indexingAddresses comes from initialState.chains, which is frozen before any simulated item executes. If an earlier simulated item/handler registers a new address and a later simulated event uses that address, this preflight will fail even though the later event would be indexable once the earlier item runs. Consider validating incrementally as simulated items are applied, or deferring this check until it can see registrations produced by prior simulated items in the batch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/envio/src/TestIndexer.res` around lines 878 - 887, The preflight in
TestIndexer.validateSrcAddresses is using only initialState.chains
indexingAddresses, so it misses addresses registered by earlier simulated items
in the same batch. Update the simulate-batch validation flow so address checks
happen incrementally as items are applied, or otherwise use the evolving
per-chain indexingAddresses state before validating each simulated item. Keep
the fix localized around SimulateItems.validateSrcAddresses and the logic that
reads from initialState.chains so later items can see registrations produced
earlier in the run.
|
|
||
| // The handler ran in the worker, producing a checkpoint change instead of | ||
| // failing validation. | ||
| t.expect(result.changes->Array.length).toEqual(1) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Assert and assert the returned value directly.
Line 36 only checks changes.length, so the first case can pass without proving the wildcard path produced the expected result, and both tests use t.expect instead of the required Assert module. Assert the whole value with Assert here.
As per coding guidelines, **/*_test.res: “Always use single assert to check the whole value instead of multiple asserts for every field” and “In tests, never log — use Assert module for all verifications.”
Also applies to: 68-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scenarios/test_codegen/test/WildcardSimulate_test.res` at line 36, The
wildcard simulation tests are using t.expect on only result.changes.length,
which does not verify the full returned value and violates the test guidelines.
Update the affected assertions in WildcardSimulate_test.res to use Assert
directly on the returned value from the wildcard simulation helpers, and replace
the field-level checks in both test cases with a single whole-value assertion so
the expected result is validated end-to-end.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/SimulateDynamicAddress_test.res (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse concrete input types in these
Utils.magiccasts.These casts still hide the source side behind
'a/_, so they do not satisfy the repo rule for explicitUtils.magicannotations. Please introduce concrete raw payload types for the process config and inline simulate items before coercing them. As per coding guidelines, "When usingUtils.magicfor type casting in ReScript, always add explicit type annotations:value->(Utils.magic: inputType => outputType)".Also applies to: 47-47, 83-89, 93-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res` at line 27, The SimulateDynamicAddress_test.res `Utils.magic` casts still use hidden source types like `'a` and `_`, so update each affected cast to name the concrete raw input type before coercing to the target type. Use explicit annotations in the cast sites around the process config and inline simulate items, and keep the target types such as `Indexer.testIndexerProcessConfig` and the relevant simulate item types unchanged while making the source side concrete.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scenarios/test_codegen/test/SimulateDynamicAddress_test.res`:
- Line 27: The SimulateDynamicAddress_test.res `Utils.magic` casts still use
hidden source types like `'a` and `_`, so update each affected cast to name the
concrete raw input type before coercing to the target type. Use explicit
annotations in the cast sites around the process config and inline simulate
items, and keep the target types such as `Indexer.testIndexerProcessConfig` and
the relevant simulate item types unchanged while making the source side
concrete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97d8aea0-7c2c-4df3-a2f3-4bf8dd598396
📒 Files selected for processing (2)
scenarios/test_codegen/test/SimulateDynamicAddress_test.resscenarios/test_codegen/test/WildcardSimulate_test.res
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/test_codegen/test/WildcardSimulate_test.res
…n tests Address review feedback on the simulate srcAddress validation: - WildcardSimulate: assert the full returned change instead of just changes.length. - SimulateDynamicAddress: a srcAddress registered by an earlier process() call is accepted via the live indexing-address state; same-batch register-then-use stays rejected, documenting that the pre-flight check can't see registrations produced during the same run. Build the simulate items with the typed Indexer.makeSimulateItem helper so the tests need no Utils.magic on the input side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175WwoErWZNsba95RnrxRyb
b1b10d8 to
9593d60
Compare
onError now logs the failure once via ErrorHandling.log (preserving the contextual msg and chain-scoped child logger) and rejects the run wrapped in Main.FatalError. Callers recognise the wrapper as already-logged: Bin.res exits without re-logging (no double log), the test worker unwraps and re-throws the inner error to the parent so test assertions still see the real message. Setup throws (e.g. patchConfig validation) arrive raw and are logged by Bin.res / surfaced by the worker as before. Also read onErrorReject via Option.getUnsafe — it's filled synchronously by Promise.make before the indexer can call onError, so the None branch was unreachable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0175WwoErWZNsba95RnrxRyb
This branch was previously deployed
2 participants
Summary
Move srcAddress validation for simulate items from the main thread (before workers start) to the worker thread (after handler registrations are applied). This allows the validation to correctly recognize events registered as wildcards via
indexer.onEvent({ wildcard: true }), which are invisible to the main thread's config.Key Changes
TestIndexer.res: Removed upfront srcAddress validation before worker startup. Validation now runs in
patchConfigafter registrations are applied, whereconfig.isWildcardreflects handler-levelwildcard: truedeclarations.Main.res: Introduced
runUntilFatalErrorpromise to capture fatal errors (including validation failures) and reject it, allowing callers to handle setup and runtime failures uniformly. The test worker now receives validation errors as rejected promises instead of process exits.TestIndexerWorker: Enhanced error handling to forward validation failures from
patchConfigto the parent thread via the worker error event, usingsetImmediateto ensure uncaught exceptions surface properly.SimulateItems.res: Added clarifying comment that
configmust have registrations applied before validation, since wildcard status depends on handler-level declarations.EventHandler.test.ts: Updated test to use
assert.rejectsinstead ofassert.throws, reflecting that validation now runs asynchronously in the worker.WildcardSimulate_test.res: New test file with two cases:
Implementation Details
The core issue:
config.yamldoesn't captureindexer.onEvent({ wildcard: true }), so the main thread can't distinguish wildcard from non-wildcard events. Moving validation into the worker (viapatchConfig) runs after registrations are applied, giving access to the true wildcard status. Fatal errors frompatchConfignow flow throughMain.start's rejection path, which the test worker catches and forwards to the parent thread.https://claude.ai/code/session_0175WwoErWZNsba95RnrxRyb
Summary by CodeRabbit
srcAddressvalidation to reflect worker-applied handler registrations, including correct wildcard vs non-wildcard behavior and clearer rejection messaging.srcAddressvalidation and wildcard configuration interpretation.