fix: support earlier registrationBlock for dynamic contracts (addresses #1188)#1354
fix: support earlier registrationBlock for dynamic contracts (addresses #1188)#1354duckling69 wants to merge 3 commits into
Conversation
enviodev#1188) Previously, registerDynamicContracts would skip + warn if an address was already known with a later effectiveStartBlock. This could happen because: - partitions fetch concurrently - source responses (HyperSync/RPC) are not guaranteed sorted within a page - within-batch items could be processed out of block order Result: the "first seen" (often later) registrationBlock won, causing the warning "Contract address is already registered at a later block number" and missed history for the address (effectiveStartBlock too high, and persisted row wrong). Changes (minimal): - Take the min effectiveStartBlock when a strictly earlier sighting arrives (both vs. indexingAddresses and within the current batch via registeringAddresses). - On correction: log at info level instead of warn, record the early value, do not splice the DC (so the early sighting reaches setBatchDcs). - Within-batch: chosenDcItemByAddress + splice superseded later DCs from their items so only the earliest sighting for an addr keeps its DC attached. - Use the dc.registrationBlock (the sighting value) when persisting. - For already-tracked addrs that get an earlier sighting, lower the owning partition latestFetchedBlock so getNextQuery will cover the gap (client filters already drop pre-start events for other addrs sharing the partition). - Added/updated targeted tests that exercise the min, splicing, and correction paths (including order-inverted within-batch and successive calls). The min is also enforced at the sighting level, so later/higher sightings naturally get shouldRemove=true and never clobber the persisted row. Tested: pnpm-driven rescript on the envio package parsed+compiled 124 modules clean with these changes; the lib FetchState registerDynamicContracts tests (now pass the updated expectations). Refs: enviodev#1188
|
Hey, just opened this for #1188. The warning was firing because we were treating the first registration sighting we saw for an address as authoritative. With partitions running in parallel and responses not being strictly ordered by block, a later registration could get processed first, and then any earlier one would get skipped with that exact message. effectiveStartBlock ended up too high and the row we persisted to envio_addresses was also the wrong one. What I did:
Only the bits that actually change the behavior + the tests that were asserting the old behavior. Build is clean and the specific tests for this path pass now. Would love eyes on it, especially around whether lowering the partition LFB is the right way to drive the backfill or if we should be creating a dedicated short gap partition with a mergeBlock. Also any reorg/rollback gotchas I might have missed. Happy to adjust based on feedback. Thanks! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDynamic contract correction flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🧹 Nitpick comments (3)
packages/envio/src/InMemoryStore.res (1)
181-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the comment to the invariant only.
This comment has useful context, but the
#1188/splicing narrative belongs in PR history. Keep the surprising behavior:dc.registrationBlockcan differ from the event item’s block after correction.Suggested wording
- // Use the registrationBlock carried on the dc (set at contractRegister time from the - // triggering event's block). The registerDynamicContracts logic + splicing of superseded - // later sightings ensures that only the earliest sighting for an address reaches here - // with its DC still attached. This gives us the min registrationBlock for persistence - // (over the composite PK), which is the core of the `#1188` fix. + // `dc.registrationBlock` is the canonical registration block; after + // out-of-order dynamic-contract sightings it may be earlier than this event item.As per coding guidelines,
.rescomments should capture non-obvious behavior and should not narrate refactors or issue history.🤖 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/InMemoryStore.res` around lines 181 - 185, Trim the comment in InMemoryStore.res to only state the non-obvious invariant: that dc.registrationBlock may differ from the event item’s block after correction, and that this value is the one used for persistence. Remove the `#1188` reference and the registerDynamicContracts/splicing narrative, keeping the note focused on the behavior around the dc registrationBlock near the relevant persistence logic.Source: Coding guidelines
packages/envio/src/FetchState.res (1)
1077-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop refactor/history narration from comments.
// Updated from previous FIXME: now choose min block (no longer early-exit skip).narrates the change history rather than explaining a non-obvious invariant; it belongs in the commit message. Keep only the comment that explains why min selection is correct (out-of-order sightings).As per coding guidelines: "Never narrate the refactor itself ... That belongs in the commit message, not the code."
🤖 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/FetchState.res` around lines 1077 - 1082, Remove the history/refactor narration from the comment near the out-of-order sighting handling in FetchState.res and keep only the invariant-focused explanation. In the block that updates effectiveStartBlock and lets the DC through, delete the sentence about “Updated from previous FIXME” and leave a brief comment describing why taking the min block is correct for out-of-order or unsorted responses.Source: Coding guidelines
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
1144-1192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for multiple earlier sightings of an already-indexed address in one batch.
The new tests exercise the not-yet-indexed within-batch path (1194-1230) and the single existing-address correction, but none feed two earlier sightings (ascending order, e.g. blocks 5 then 2) for an address already in
indexingAddressesin a singleregisterDynamicContractscall. That case currently both leaves duplicate DCs on the items and records the non-min correction (see FetchState.res 1076-1106). A test here would catch it.Want me to draft this test case once the existing-address branch is fixed?
🤖 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/lib_tests/FetchState_test.res` around lines 1144 - 1192, Add a test in FetchState.registerDynamicContracts coverage for an address already present in indexingAddresses when two earlier sightings arrive in the same batch (for example, blocks 5 then 2). Exercise the existing-address correction path using FetchState.registerDynamicContracts, makeDynContractRegistration, and dcToItem, and assert the effectiveStartBlock resolves to the minimum earlier block while only the corrected registration is retained for persistence. Also verify the state does not keep duplicate DCs on the items and that the earlier batch ordering does not prevent the min correction from being applied.
🤖 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/FetchState.res`:
- Around line 1076-1106: The existing-address correction path in
FetchState.register is order-dependent and can leave duplicate dynamic contracts
in the batch. Update the branch that handles already-indexed addresses to mirror
the None-path logic: compare against any prior sighting recorded in
registeringAddresses/correctedAddresses, keep the earliest effectiveStartBlock
via min, and splice out the superseded DC through chosenDcItemByAddress when a
better earlier sighting is found. Ensure only the kept DC remains eligible for
setBatchDcs so duplicate envio_addresses writes cannot occur.
- Around line 1100-1104: Remove the dead `earliestRegisteringEventBlockNumber`
tracking from `FetchState.res`; it is initialized and updated in the
partitioning logic but never read anywhere else. Delete its declaration and all
assignments, and keep the existing `progressBlockNumber` and loop-based logic in
`FetchState` as the source of truth.
---
Nitpick comments:
In `@packages/envio/src/FetchState.res`:
- Around line 1077-1082: Remove the history/refactor narration from the comment
near the out-of-order sighting handling in FetchState.res and keep only the
invariant-focused explanation. In the block that updates effectiveStartBlock and
lets the DC through, delete the sentence about “Updated from previous FIXME” and
leave a brief comment describing why taking the min block is correct for
out-of-order or unsorted responses.
In `@packages/envio/src/InMemoryStore.res`:
- Around line 181-185: Trim the comment in InMemoryStore.res to only state the
non-obvious invariant: that dc.registrationBlock may differ from the event
item’s block after correction, and that this value is the one used for
persistence. Remove the `#1188` reference and the
registerDynamicContracts/splicing narrative, keeping the note focused on the
behavior around the dc registrationBlock near the relevant persistence logic.
In `@scenarios/test_codegen/test/lib_tests/FetchState_test.res`:
- Around line 1144-1192: Add a test in FetchState.registerDynamicContracts
coverage for an address already present in indexingAddresses when two earlier
sightings arrive in the same batch (for example, blocks 5 then 2). Exercise the
existing-address correction path using FetchState.registerDynamicContracts,
makeDynContractRegistration, and dcToItem, and assert the effectiveStartBlock
resolves to the minimum earlier block while only the corrected registration is
retained for persistence. Also verify the state does not keep duplicate DCs on
the items and that the earlier batch ordering does not prevent the min
correction from being applied.
🪄 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: 9a101c53-5526-4331-b1c1-3da65af87034
📒 Files selected for processing (3)
packages/envio/src/FetchState.respackages/envio/src/InMemoryStore.resscenarios/test_codegen/test/lib_tests/FetchState_test.res
… address corrections (enviodev#1188) The "address already present in indexingAddresses" path (both the events-with-startBlock and no-events arms) was not participating in the within-batch min selection that the new-address path already had via registeringAddresses + chosenDcItemByAddress + splicing. Consequence (when two earlier sightings for the same already-indexed address appear in one registerDynamicContracts call): - both DCs could survive to setBatchDcs (duplicate persistence rows) - correctedAddresses would contain the last-seen value instead of the true min This change makes the correction path mirror the None-path logic: - consult the batch view (registeringAddresses) for a prior sighting in this call - when replacing a worse batch sighting, splice its DC off the old item - always record the current best item in chosenDcItemByAddress so a later worse sighting can find and splice it No new functions were introduced (the small splice block is inlined in the two places that need it, keeping the change localised). Also removed the dead earliestRegisteringEventBlockNumber (written in two places, never read after the loop). Added senior-level comments at the top of the function and inside the decision arms explaining the cross-path invariant ("only the single earliest sighting in the call keeps a DC") and why corrections must participate in the batch dedup. Only this file was touched. Rescript build was clean in prior verification. Refs: enviodev#1188
Rephrase the batch registration comment to be neutral and factual. No functional changes.
|
Hi, thanks for contributing. Unfortunately, I think lowering LFB won't work in this case, since partition might already be fetching when we get a new registration + it might have other addresses which already got logs for the block range, so reducing LFB might result in duplicated logs. Probably the right approach here will be to create a new partition with a start and end block to catch up to the already existing partition. But I'm letting you know that I'm currently working on version 3.3.0, which changes the way we store addresses a lot, so the PR will have many conflicts there. I suggest you to stop working on it in the meantime. I understood that this is a problem for you, so I'll try to include support for the case in the internal redesign I work on for 3.3.0 🙏 |
2 participants
Hey folks,
Been running into the "Skipping contract registration: Contract address is already registered at a later block number" warning in a few setups with dynamic contracts. The root seems to be that we key on address in indexingAddresses and just take whatever registration we see first. Because partitions are concurrent and the items coming back from a source query aren't strictly guaranteed to be in block order (plus within a batch), a later sighting could win and lock in a too-high effectiveStartBlock for that address. Then we skip the earlier one and the persisted row in envio_addresses ends up with the wrong (later) block.
This patch makes us prefer the earliest registrationBlock we encounter for an address. When we see a strictly earlier sighting we update the record, log a correction at info level instead of the scary warning, and keep the DC attached so it gets persisted with the right (early) block. For stuff we already had indexed we also lower the latestFetchedBlock on the owning partition(s) so we actually fetch the missing earlier range (the client-side filters already drop pre-effective events for other addresses in the same partition). Added a bit of bookkeeping with chosenDcItemByAddress so within a single register call the min wins even if the array isn't perfectly sorted.
Only touched the three files needed for the actual fix + the test updates that were exercising the old (broken) expectations.
Built clean with rescript (124 modules) and the FetchState registerDynamicContracts tests now pass with the new behavior.
Fixes #1188. Would really appreciate a review, especially on the backfill approach (lowering LFB vs. emitting an explicit short gap partition) and anything reorg-related I might have missed. Happy to tweak.
Thanks!
Summary by CodeRabbit
registerDynamicContractstest coverage for earlier-registration corrections, within-batch ordering, and successive out-of-order updates.