fix: prevent unlock indexer quorum future-checkpoint crash#644
Merged
Conversation
Summary:
- clamp quorum lookups to a safe past checkpoint when a requested
proposal snapshot is still in the future
- log the clamp decision and cover the future-checkpoint behavior in
unit tests
Rationale:
- unlock-dao was dying on ProposalCreated because quorum(proposalSnapshot)
deterministically reverted before the snapshot checkpoint existed
- clamping preserves proposal indexing progress without changing the
existing near-head fallback behavior for normal quorum reads
Tests:
- ./node_modules/.bin/jest --runInBand __tests__/unit/chaintool.test.ts
- npx -y tsx -e 'import { ChainTool } from "./src/internal/chaintool.ts"; void (async () => { const tool = new ChainTool(); const result = await tool.quorum({ chainId: 8453, contractAddress: "0x65bA0624403Fc5Ca2b20479e9F626eD4D78E0aD9", governorTokenAddress: "0xaC27fa800955849d6D17cC8952Ba9dD6EAA66187", governorTokenStandard: "ERC20", timepoint: 1775179697n }); console.log(JSON.stringify({ clockMode: result.clockMode, quorum: result.quorum.toString(), decimals: result.decimals.toString() })); })();'
- ./node_modules/.bin/tsc -p tsconfig.json --noEmit (fails on existing generated model/type baseline unrelated to this change)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 642adcd576
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents the indexer from crashing when a Governor’s quorum(timepoint) is queried with a checkpoint that is still in the future (deterministic revert), by clamping to a safe past timepoint and adding a regression test to cover this path.
Changes:
- Add helper functions to derive “stable” quorum timepoints and clamp future checkpoints to a past timepoint.
- Allow
currentClock()to accept an optional precomputedclockModeto avoid redundant reads. - Add a unit test asserting future quorum checkpoints are clamped and logged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/indexer/src/internal/chaintool.ts | Clamp future quorum() timepoints; refactor stable timepoint calculation; optimize currentClock() by accepting optional clockMode. |
| packages/indexer/tests/unit/chaintool.test.ts | Add regression coverage for the future-checkpoint clamping path and adjust existing tests for the new currentClock() usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary:
- retry explicit snapshot quorum reads only after a deterministic revert proves the checkpoint is too new
- cache clamped quorum data under the effective queried checkpoint and cover the behavior in unit tests
Rationale:
- review feedback identified that the first fix always paid an extra currentClock RPC and could cache clamped quorum data under the original future snapshot key
- this keeps the unlock-dao crash fix while preserving correct snapshot semantics for repeated quorum reads
Tests:
- ./node_modules/.bin/jest --runInBand __tests__/unit/chaintool.test.ts
- npx -y tsx -e 'import { ChainTool } from ./src/internal/chaintool.ts; void (async () => { const tool = new ChainTool(); const result = await tool.quorum({ chainId: 8453, contractAddress: 0x65bA0624403Fc5Ca2b20479e9F626eD4D78E0aD9, governorTokenAddress: 0xaC27fa800955849d6D17cC8952Ba9dD6EAA66187, governorTokenStandard: ERC20, timepoint: 1775179697n }); console.log(JSON.stringify({ clockMode: result.clockMode, quorum: result.quorum.toString(), decimals: result.decimals.toString() })); })();'
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd109b919a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
fix #642 |
Summary:
- widen future-checkpoint clamping to the same safety margin used by the
near-head quorum path for timestamp and block clocks
- keep stale entries under the effective clamped cache key available when a
retry fetch fails
- extend quorum unit coverage for the wider clamp target and stale clamped
cache fallback behavior
Rationale:
- a one-second timestamp clamp could still be future on a lagging RPC, which
reintroduced the fatal deterministic revert path for unlock-dao indexing
- clamped cache entries should remain usable as stale fallback data so
transient retry failures do not bubble up as processor-fatal errors
Tests:
- ./node_modules/.bin/jest --runInBand __tests__/unit/chaintool.test.ts
- npx -y tsx -e 'import { ChainTool } from "./src/internal/chaintool.ts"; void (async () => { const tool = new ChainTool(); const result = await tool.quorum({ chainId: 8453, contractAddress: "0x65bA0624403Fc5Ca2b20479e9F626eD4D78E0aD9", governorTokenAddress: "0xaC27fa800955849d6D17cC8952Ba9dD6EAA66187", governorTokenStandard: "ERC20", timepoint: 1775179697n }); console.log(JSON.stringify({ clockMode: result.clockMode, quorum: result.quorum.toString(), decimals: result.decimals.toString() })); })();'
Collaborator
Author
|
[codex] Changes since last review:
|
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
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
ChainTool.quorum()to a safe past checkpoint when the requested quorum timepoint is still in the futureProblem
unlock-daowas stuck at block44021688in productionProposalCreatedat block44022375triggeredquorum(proposalSnapshot)with a future timestamp checkpoint (1775179697)Validation
./node_modules/.bin/jest --runInBand __tests__/unit/chaintool.test.tsnpx -y tsx -e 'import { ChainTool } from "./src/internal/chaintool.ts"; void (async () => { const tool = new ChainTool(); const result = await tool.quorum({ chainId: 8453, contractAddress: "0x65bA0624403Fc5Ca2b20479e9F626eD4D78E0aD9", governorTokenAddress: "0xaC27fa800955849d6D17cC8952Ba9dD6EAA66187", governorTokenStandard: "ERC20", timepoint: 1775179697n }); console.log(JSON.stringify({ clockMode: result.clockMode, quorum: result.quorum.toString(), decimals: result.decimals.toString() })); })();'./node_modules/.bin/tsc -p tsconfig.json --noEmitcurrently fails on pre-existing generated model/type baseline issues unrelated to this PR