Refactor dynamic contract registry to separate addresses table - #1076
Merged
Conversation
Remove registering_event_block_timestamp, registering_event_src_address, registering_event_name, registering_event_contract_name (derivable from block+log_index). Remove contract_address in favor of id. Rename registering_event_block_number to registering_event_block. Make registering_event_log_index nullable for future block handler support. Use composite primary key (id, chain_id). https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Remove EnvioAddresses from the entity framework entirely. The id column now stores the raw address string. Addresses are persisted via direct SQL INSERT with ON CONFLICT DO NOTHING and rolled back via DELETE WHERE envio_checkpoint_id > target. A new envio_checkpoint_id column tracks when each address was registered. In-memory: InMemoryStore.addressesToWrite accumulates per-batch entries. TestIndexer.addressesByChain provides per-chain lookup (chainId -> address -> entry) to avoid id collision across chains. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace the DynamicContractRegistry entity with a new EnvioAddresses entity; thread an Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Event Handler
participant Memory as InMemoryStore
participant Persist as Persistence
participant Storage as PgStorage
participant DB as PostgreSQL
Handler->>Memory: setBatchDcs(batch)
Memory->>Memory: build EnvioAddresses.t entries
Memory->>Memory: append to addressesToWrite
Handler->>Persist: writeBatch(..., addressesToWrite)
Persist->>Storage: storage.writeBatch(..., addressesToWrite)
Storage->>DB: INSERT INTO envio_addresses via UNNEST (bulk)
DB-->>Storage: inserted / conflicts ignored
alt rollback target present
Storage->>DB: DELETE FROM envio_addresses WHERE envio_checkpoint_id > target
DB-->>Storage: deleted rows
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Keep upstream runChainWorker/runChains sequential execution refactor, apply addressesByChain and addressesToWrite changes on top. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scenarios/test_codegen/test/rollback/Rollback_test.res (1)
752-755: Prefer full-value assertions instead of count-only checks.Line 752–755 and Line 815–820 only validate count; they can still pass with wrong address rows. Assert exact projected values (as you already do in Line 736–745 / Line 837–846) for stronger rollback coverage.
Based on learnings: Applies to **/*.{test.res,test.js,test.ts,test.tsx,spec.res,spec.js,spec.ts,spec.tsx} : Always use single assert to check the whole value instead of multiple asserts for every field.
Also applies to: 815-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/rollback/Rollback_test.res` around lines 752 - 755, The test currently checks only the count using (await (indexerMock.queryAddresses()))->Array.length which can pass with incorrect address rows; replace these count-only assertions (the ones using indexerMock.queryAddresses() and comparing Array.length) with a single full-value assertion that compares the entire projected address array to the expected array of objects (same shape as the existing full assertions used around lines where you already assert full projected values), e.g., call indexerMock.queryAddresses(), await the result, map/format it the same way the other full-value tests do, and assert equality against the exact expected array so the rollback test validates all fields, not just the count.packages/envio/src/db/InternalTable.res (1)
190-197: StabilizedynamicContractsordering in the JSON aggregate.Line 190–197 uses
json_agg(...)withoutORDER BY, which can return non-deterministic array ordering across runs/plans.♻️ Suggested SQL adjustment
( - SELECT COALESCE(json_agg(json_build_object( + SELECT COALESCE(json_agg(json_build_object( 'address', "id", 'contractName', "contract_name", 'startBlock', "registering_event_block", 'registrationBlock', "registering_event_block" - )), '[]'::json) + ) ORDER BY "registering_event_block", "registering_event_log_index", "id"), '[]'::json) FROM "${pgSchema}"."${EnvioAddresses.table.tableName}" WHERE "chain_id" = chains."${(`#id`: field :> string)}" ) as "dynamicContracts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/db/InternalTable.res` around lines 190 - 197, The json_agg(...) call that builds the dynamicContracts array is unordered and can return non-deterministic results; update the SELECT so the aggregation is deterministic by adding an ORDER BY inside the aggregate (e.g., json_agg(json_build_object(... ) ORDER BY "id" OR ORDER BY "registering_event_block") when querying "${pgSchema}"."${EnvioAddresses.table.tableName}" filtered by chains."${(`#id`: field :> string)}"; ensure you pick a stable column (like "id" or "registering_event_block") for the ordering so the JSON array is produced in a consistent order across runs.scenarios/test_codegen/test/lib_tests/PgStorage_test.res (1)
268-269: Consider indexingenvio_checkpoint_idfor rollback delete performance.Given rollback deletes by checkpoint threshold, a dedicated index (or
(chain_id, envio_checkpoint_id)if queried by chain too) can prevent full scans asenvio_addressesgrows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res` around lines 268 - 269, Add an index to speed up rollback deletes on envio_addresses: create an index on envio_checkpoint_id (or a composite index on (chain_id, envio_checkpoint_id) if rollbacks are scoped by chain) so DELETE ... WHERE envio_checkpoint_id <= ? (and optional chain_id = ?) can use the index instead of scanning the whole "test_schema"."envio_addresses" table; modify the migration/DDL that defines "envio_addresses" to include CREATE INDEX ... ON "test_schema"."envio_addresses"(envio_checkpoint_id) or CREATE INDEX ... ON "test_schema"."envio_addresses"(chain_id, envio_checkpoint_id) as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/envio/src/Config.res`:
- Around line 107-120: The table definition for envio_addresses (created via
Table.mkTable bound to variable table) only has a composite primary key (id,
chain_id), causing queries filtering solely by chain_id or by
envio_checkpoint_id to do sequential scans; add explicit indexes for chain_id
and envio_checkpoint_id (using your Table.mkIndex / index helper alongside the
existing Table.mkField calls) so dynamic-contract bootstrap queries on chain_id
and rollback deletions on envio_checkpoint_id use indexes; update the table
definition to include these two indexes referencing the "chain_id" and
"envio_checkpoint_id" fields.
In `@packages/envio/src/TestIndexer.res`:
- Around line 245-272: The test indexer currently overwrites previously stored
addresses in state.addressesByChain and emits address changes even when
PostgreSQL would have ignored duplicates; to fix, make address writes
first-write-wins by checking existence before accepting/emitting: when building
addressSets from addressesToWrite, skip any addr whose id already exists in
state.addressesByChain for that addr.chainId (and also skip duplicates already
accepted earlier in this batch), and when storing into state.addressesByChain in
the addressesToWrite->Array.forEach loop only insert (Js.Dict.set) the addr if
the chainDict does not already contain addr.id; refer to addressesToWrite,
addressSets, state.addressesByChain, chainDict and change to locate the relevant
code to update.
In `@scenarios/test_codegen/test/helpers/Mock.res`:
- Around line 431-440: The queryAddresses implementation unsafely uses
Utils.magic to cast raw SQL rows to Config.EnvioAddresses.t; change it to use
schema-based decoding like queryCheckpoints/queryRaw: after running the
Postgres.unsafe(PgStorage.makeLoadAllQuery(...)) call, pipe the result through
Postgres.decodeRows (or the existing schema parser used in queryCheckpoints)
with Config.EnvioAddresses.schema, then map/convert to
promise<array<Config.EnvioAddresses.t>> instead of using Utils.magic; update the
pipeline around queryAddresses, referencing queryAddresses,
InternalTable.EnvioAddresses.table.tableName, Config.EnvioAddresses.schema, and
remove Utils.magic usage.
---
Nitpick comments:
In `@packages/envio/src/db/InternalTable.res`:
- Around line 190-197: The json_agg(...) call that builds the dynamicContracts
array is unordered and can return non-deterministic results; update the SELECT
so the aggregation is deterministic by adding an ORDER BY inside the aggregate
(e.g., json_agg(json_build_object(... ) ORDER BY "id" OR ORDER BY
"registering_event_block") when querying
"${pgSchema}"."${EnvioAddresses.table.tableName}" filtered by chains."${(`#id`:
field :> string)}"; ensure you pick a stable column (like "id" or
"registering_event_block") for the ordering so the JSON array is produced in a
consistent order across runs.
In `@scenarios/test_codegen/test/lib_tests/PgStorage_test.res`:
- Around line 268-269: Add an index to speed up rollback deletes on
envio_addresses: create an index on envio_checkpoint_id (or a composite index on
(chain_id, envio_checkpoint_id) if rollbacks are scoped by chain) so DELETE ...
WHERE envio_checkpoint_id <= ? (and optional chain_id = ?) can use the index
instead of scanning the whole "test_schema"."envio_addresses" table; modify the
migration/DDL that defines "envio_addresses" to include CREATE INDEX ... ON
"test_schema"."envio_addresses"(envio_checkpoint_id) or CREATE INDEX ... ON
"test_schema"."envio_addresses"(chain_id, envio_checkpoint_id) as appropriate.
In `@scenarios/test_codegen/test/rollback/Rollback_test.res`:
- Around line 752-755: The test currently checks only the count using (await
(indexerMock.queryAddresses()))->Array.length which can pass with incorrect
address rows; replace these count-only assertions (the ones using
indexerMock.queryAddresses() and comparing Array.length) with a single
full-value assertion that compares the entire projected address array to the
expected array of objects (same shape as the existing full assertions used
around lines where you already assert full projected values), e.g., call
indexerMock.queryAddresses(), await the result, map/format it the same way the
other full-value tests do, and assert equality against the exact expected array
so the rollback test validates all fields, not just the count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 336ccb2c-f5f0-4c07-916b-c31cab3e74f0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/envio/src/Config.gen.tspackages/envio/src/Config.respackages/envio/src/InMemoryStore.respackages/envio/src/Persistence.respackages/envio/src/PgStorage.respackages/envio/src/TestIndexer.respackages/envio/src/TestIndexerProxyStorage.respackages/envio/src/db/InternalTable.gen.tspackages/envio/src/db/InternalTable.resscenarios/test_codegen/test/helpers/Mock.resscenarios/test_codegen/test/integration-raw-events.test.tsscenarios/test_codegen/test/lib_tests/Persistence_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/rollback/Rollback_test.res
…ding - Add indexes on chain_id and envio_checkpoint_id for envio_addresses table to avoid seq scans on bootstrap and rollback queries - Make TestIndexer address writes first-write-wins to match DB's ON CONFLICT DO NOTHING semantics - Use schema-based decoding in queryAddresses for type-safe bigint parsing of envio_checkpoint_id https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/envio/src/TestIndexer.res (1)
245-264: Minor gap: within-batch duplicates not tracked for emitted changes.The
addressSetsfiltering checksstate.addressesByChain(populated from previous batches), but ifaddressesToWritecontains duplicate addresses with differentcheckpointIds within the same batch, both would be emitted inprocessChangeseven though only the first is stored.This is a minor consistency gap since storage is correct. If test assertions rely on emitted changes matching actual persisted data, consider tracking accepted addresses within the batch iteration.
♻️ Optional: Track within-batch duplicates
+ // Track addresses accepted in this batch to avoid emitting duplicates + let acceptedInBatch: dict<bool> = Js.Dict.empty() + // Build combined checkpoint + entity changes objects for i in 0 to checkpointIds->Array.length - 1 { ... // Add address changes for this checkpoint (skip already-stored addresses to match ON CONFLICT DO NOTHING) let addressSets = addressesToWrite->Array.keepMap(addr => { if addr.checkpointId !== checkpointId { None } else { let chainIdStr = addr.chainId->Int.toString + let batchKey = chainIdStr ++ "_" ++ addr.id let alreadyExists = switch state.addressesByChain->Js.Dict.get(chainIdStr) { | Some(chainDict) => chainDict->Js.Dict.get(addr.id) !== None | None => false } - alreadyExists + let alreadyAccepted = acceptedInBatch->Js.Dict.get(batchKey) !== None + (alreadyExists || alreadyAccepted) ? None - : Some({"address": addr->Config.EnvioAddresses.getAddress, "contract": addr.contractName}) + : { + acceptedInBatch->Js.Dict.set(batchKey, true) + Some({"address": addr->Config.EnvioAddresses.getAddress, "contract": addr.contractName}) + } } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/TestIndexer.res` around lines 245 - 264, The emitted addressSets currently only checks state.addressesByChain (past batches) so duplicates inside the current addressesToWrite can still be emitted; fix by adding a local mutable set (e.g., seenInBatch) inside the addressesToWrite->Array.keepMap loop and, for each addr (use chainIdStr = addr.chainId->Int.toString and addr.id or Config.EnvioAddresses.getAddress to build a unique key), skip if that key is already in seenInBatch, otherwise insert the key into seenInBatch and include the addr in addressSets; this ensures addressSets only contains the first occurrence per batch while preserving the existing state.addressesByChain check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scenarios/test_codegen/test/helpers/Mock.res`:
- Around line 431-460: The cast after Postgres.unsafe in queryAddresses is wrong
— change the Utils.magic cast from "(Utils.magic: array<unknown> =>
array<unknown>)" to match queryCheckpoints by treating the incoming value as
unknown: use "(Utils.magic: unknown => array<unknown>)"; update this in the
queryAddresses Promise.thenResolve mapping so the Postgres.unsafe result is
first cast from unknown to array<unknown> before Js.Array2.map; reference
queryAddresses, queryCheckpoints, Utils.magic, and Postgres.unsafe when making
the change.
---
Nitpick comments:
In `@packages/envio/src/TestIndexer.res`:
- Around line 245-264: The emitted addressSets currently only checks
state.addressesByChain (past batches) so duplicates inside the current
addressesToWrite can still be emitted; fix by adding a local mutable set (e.g.,
seenInBatch) inside the addressesToWrite->Array.keepMap loop and, for each addr
(use chainIdStr = addr.chainId->Int.toString and addr.id or
Config.EnvioAddresses.getAddress to build a unique key), skip if that key is
already in seenInBatch, otherwise insert the key into seenInBatch and include
the addr in addressSets; this ensures addressSets only contains the first
occurrence per batch while preserving the existing state.addressesByChain check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23012fb7-fa67-4209-844e-77727e5ed2d5
📒 Files selected for processing (4)
packages/envio/src/Config.respackages/envio/src/TestIndexer.resscenarios/test_codegen/test/helpers/Mock.resscenarios/test_codegen/test/lib_tests/PgStorage_test.res
✅ Files skipped from review due to trivial changes (1)
- packages/envio/src/Config.res
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/test_codegen/test/lib_tests/PgStorage_test.res
- Fix queryAddresses cast from (array<unknown> => array<unknown>) to (unknown => array<unknown>) to match queryCheckpoints pattern - Track within-batch duplicate addresses via seenInBatch dict so emitted changes match ON CONFLICT DO NOTHING semantics https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/envio/src/TestIndexer.res (1)
236-243: Annotate the newUtils.magiccasts.The payload-building code added here reintroduces untyped
Utils.magiccalls forsets,deleted, andaddresses. Please add explicit input/output annotations on each new cast so the JS boundary stays inspectable.As per coding guidelines, "When using Utils.magic for type casting in ReScript, always add explicit type annotations:
value->(Utils.magic: inputType => outputType)"Also applies to: 271-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/TestIndexer.res` around lines 236 - 243, Replace the untyped Utils.magic casts with explicit input=>output annotations so the JS boundary is inspectable: for each occurrence (the casts on sets, deleted, and addresses used with Js.Dict.set and the similar casts around lines 271-274), change value->Utils.magic to value->(Utils.magic: inputType => outputType) using the actual ReScript types you expect (e.g., array/dict shapes or specific record types) as inputType and the corresponding JS representation as outputType; update the casts on entityObj->Js.Dict.set("sets", ...), entityObj->Js.Dict.set("deleted", ...), change->Js.Dict.set(entityName, ...), and the three similar casts at 271-274 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/envio/src/TestIndexer.res`:
- Around line 41-45: toIndexingContract currently preserves registrationBlock
but the proxy WriteBatch payload omits rollbackTargetCheckpointId, so
rolled-back addresses never get pruned from state.addressesByChain causing
alreadyExists to block re-registrations; fix by threading
rollbackTargetCheckpointId (rollback target checkpoint id) into the WriteBatch
payload (the code paths that build the batch in TestIndexerProxyStorage
functions) and, before performing duplicate checks and final insert into
state.addressesByChain, remove/prune any existing address rows whose
checkpointId is greater than the provided rollback target; apply the same change
to the other insert paths referenced (around the other occurrences noted: the
115-124 and 248-294 regions) so pruning happens consistently prior to duplicate
detection and insertion, and ensure the inserted/returned
Internal.indexingContract includes the rollback target (e.g., add
rollbackTargetCheckpointId to the data passed through
toIndexingContract/WriteBatch).
---
Nitpick comments:
In `@packages/envio/src/TestIndexer.res`:
- Around line 236-243: Replace the untyped Utils.magic casts with explicit
input=>output annotations so the JS boundary is inspectable: for each occurrence
(the casts on sets, deleted, and addresses used with Js.Dict.set and the similar
casts around lines 271-274), change value->Utils.magic to value->(Utils.magic:
inputType => outputType) using the actual ReScript types you expect (e.g.,
array/dict shapes or specific record types) as inputType and the corresponding
JS representation as outputType; update the casts on
entityObj->Js.Dict.set("sets", ...), entityObj->Js.Dict.set("deleted", ...),
change->Js.Dict.set(entityName, ...), and the three similar casts at 271-274
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c8ab17-655f-4ac4-aaa7-b4c214c476df
📒 Files selected for processing (2)
packages/envio/src/TestIndexer.resscenarios/test_codegen/test/helpers/Mock.res
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/test_codegen/test/helpers/Mock.res
Comment on lines
+41
to
+45
| let toIndexingContract = (dc: Config.EnvioAddresses.t): Internal.indexingContract => { | ||
| address: dc->Config.EnvioAddresses.getAddress, | ||
| contractName: dc.contractName, | ||
| startBlock: dc.registeringEventBlockNumber, | ||
| registrationBlock: Some(dc.registeringEventBlockNumber), | ||
| startBlock: dc.registeringEventBlock, | ||
| registrationBlock: Some(dc.registeringEventBlock), |
Contributor
There was a problem hiding this comment.
Thread rollback targets into the address write path.
toIndexingContract still preserves registrationBlock for rollback-aware dynamic contracts, but state.addressesByChain only ever grows here. Because the proxy WriteBatch payload still omits rollbackTargetCheckpointId (packages/envio/src/TestIndexerProxyStorage.res:23-43, packages/envio/src/TestIndexerProxyStorage.res:140-177), rolled-back addresses will stay in memory, alreadyExists will block legitimate re-registrations, and later process() calls will bootstrap contracts that PgStorage has already removed. Please prune rows with checkpointId > target before the duplicate checks and final insert once that target is threaded through.
Also applies to: 115-124, 248-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envio/src/TestIndexer.res` around lines 41 - 45, toIndexingContract
currently preserves registrationBlock but the proxy WriteBatch payload omits
rollbackTargetCheckpointId, so rolled-back addresses never get pruned from
state.addressesByChain causing alreadyExists to block re-registrations; fix by
threading rollbackTargetCheckpointId (rollback target checkpoint id) into the
WriteBatch payload (the code paths that build the batch in
TestIndexerProxyStorage functions) and, before performing duplicate checks and
final insert into state.addressesByChain, remove/prune any existing address rows
whose checkpointId is greater than the provided rollback target; apply the same
change to the other insert paths referenced (around the other occurrences noted:
the 115-124 and 248-294 regions) so pruning happens consistently prior to
duplicate detection and insertion, and ensure the inserted/returned
Internal.indexingContract includes the rollback target (e.g., add
rollbackTargetCheckpointId to the data passed through
toIndexingContract/WriteBatch).
Annotate all Utils.magic casts in handleWriteBatch with explicit input => output types per project guidelines. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/envio/src/TestIndexer.res (1)
115-124:⚠️ Potential issue | 🟠 MajorAddress cache is not rollback-aware yet.
state.addressesByChainonly grows, and duplicate checks depend on it. Without pruning entries above a rollback target, valid re-registrations can be skipped and stale dynamic contracts can leak into later bootstrap state.Also applies to: 248-260, 285-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envio/src/TestIndexer.res` around lines 115 - 124, state.addressesByChain currently only grows and causes duplicate suppression across rollbacks; make the cache rollback-aware by tracking when an address was registered and pruning entries above a rollback target before performing duplicate checks in handleWriteBatch (and the other affected handlers). Concretely: change state.addressesByChain to store per-address metadata (e.g. lastSeenCheckpointChainId or tuple with checkpointChainId/checkpointBlockNumber), add a helper (e.g. pruneAddressesAboveRollbackTarget) that removes or ignores addresses whose recorded checkpoint is greater than the rollback target, call this prune helper at the start of handleWriteBatch (and the other duplicate-check sites), and ensure rollback logic updates/removes entries so re-registrations after rollbacks are not skipped. Ensure duplicate-check logic uses the pruned/filtered view so stale addresses do not leak into later bootstrap state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/envio/src/TestIndexer.res`:
- Around line 115-124: state.addressesByChain currently only grows and causes
duplicate suppression across rollbacks; make the cache rollback-aware by
tracking when an address was registered and pruning entries above a rollback
target before performing duplicate checks in handleWriteBatch (and the other
affected handlers). Concretely: change state.addressesByChain to store
per-address metadata (e.g. lastSeenCheckpointChainId or tuple with
checkpointChainId/checkpointBlockNumber), add a helper (e.g.
pruneAddressesAboveRollbackTarget) that removes or ignores addresses whose
recorded checkpoint is greater than the rollback target, call this prune helper
at the start of handleWriteBatch (and the other duplicate-check sites), and
ensure rollback logic updates/removes entries so re-registrations after
rollbacks are not skipped. Ensure duplicate-check logic uses the pruned/filtered
view so stale addresses do not leak into later bootstrap state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a6bee82-2d98-46f9-a9f7-2064dcef65b0
📒 Files selected for processing (1)
packages/envio/src/TestIndexer.res
Split envio_addresses storage into two tables: - envio_addresses: current state (no envio_checkpoint_id column) - envio_history_envio_addresses: append-only log with envio_checkpoint_id as part of the composite PK On write, both tables receive the new address rows. On rollback: 1. DELETE from main table any address whose only history is past the rollback target (i.e. was only registered after the target) 2. DELETE history rows past the target This mirrors the entity framework's history pattern while supporting composite (id, chain_id) primary keys. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Undo the custom persistence path for envio_addresses. EnvioAddresses is
once again a regular entity in Persistence.allEntities, which means:
- auto-generated envio_history_envio_addresses via the entity framework
- write/rollback go through the same InMemoryTable.Entity + EntityHistory
paths as every other entity
- the in-memory id is a composite {chainId}-{address} string to stay
unique across chains; getAddress extracts the raw address
No custom INSERT/DELETE in PgStorage, no addressesToWrite on InMemoryStore,
no per-chain dict in TestIndexer.
https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
- getAddress now uses a single indexOf('-') lookup instead of formatting
chainId to a string and measuring its length
- Add 'keep in sync with makeId / getAddress' comments on getAddress and
on the SUBSTRING SQL in makeGetInitialStateQuery so future changes to
the composite-id format flag both sites
- Rename castFromDcRegistry -> castToEnvioAddresses to match the new
module name (the old name referenced the pre-rename DynamicContractRegistry)
https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
- Remove @Gentype from EnvioAddresses.t. The generated TS type wasn't imported anywhere; dropping it deletes Config.gen.ts and removes the unused EnvioAddresses_t export from InternalTable.gen.ts. - Rename registering_event_block -> registration_block and registering_event_log_index -> registration_log_index. Matches the existing Internal.indexingContract.registrationBlock name and reads more naturally without the awkward "registering_event" prefix. - registration_log_index goes from optional to a plain int with -1 as the sentinel for "registered from a block handler" (no log index). Log indices are always >= 0, so -1 is unambiguous. Mirrors the convention in envio_chains where uninitialized block numbers are -1. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Upstream (#1103) migrated Js.* bindings to the ReScript 12 stdlib (Js.Array2/Js.Dict/Js.null -> Array/Dict/Null, Js.String2 -> String, etc.) and bumped rescript to 12.2.0. Conflicts were all mechanical rewrites of my PR code to the new stdlib, keeping the EnvioAddresses rename, simplified schema, and field renames from this branch. Also replaced Js.String2.indexOf/sliceToEnd in getAddress with String.indexOf + String.slice to clear the deprecation warnings. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Side effect of rebuilding with rescript 12.2.0: genType now emits relative imports for RescriptSchema.gen.js instead of the package-name import. No runtime change. https://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
This was referenced Apr 17, 2026
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the dynamic contract registry system by extracting address tracking into a dedicated
EnvioAddressestable, removing the internalDynamicContractRegistryentity, and simplifying the data model for tracking dynamically registered contracts.Key Changes
New
EnvioAddressesmodule: ReplacesDynamicContractRegistrywith a simpler structure containing only essential fields:id(address as string),chainId,contractName,registeringEventBlock,registeringEventLogIndex, andcheckpointIdregisteringEventBlockTimestamp,registeringEventContractName,registeringEventName, andregisteringEventSrcAddressDatabase schema updates:
envio_addressestable toPgStorageinitializationCheckpointsquery to fetch dynamic contracts from the new tableStorage layer changes:
writeBatchto acceptaddressesToWriteparameterInMemoryStoreto accumulate addresses inaddressesToWritearray instead of storing as entitiesTestIndexer updates:
addressesByChaindictionary to track addresses per chaintoIndexingContractconversion to work withEnvioAddresses.tDynamicContractRegistryentityTest updates:
queryAddresses()instead of querying the registry entityImplementation Details
idfield inEnvioAddressesdirectly stores the address string (viaAddress.toString)registeringEventLogIndexis now optional to handle cases where it may not be available(id, chain_id)prevents duplicate address entries per chainhttps://claude.ai/code/session_01XQDfh8ZyX5TSyVDPKbMzx2
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests