Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Simplify Contract Subscriptions block range #744

Merged
merged 6 commits into from
Nov 6, 2024
Merged

Conversation

arcoraven
Copy link
Contributor

@arcoraven arcoraven commented Oct 30, 2024

PR-Codex overview

This PR focuses on improving the handling of transaction receipts and event logs within a blockchain indexing worker. It includes enhancements to logging, error handling, and the estimation of block times, while also refactoring some code for better clarity and efficiency.

Detailed summary

  • Added logging for inserted receipts in processTransactionReceiptsWorker.ts.
  • Changed parseInt to Number.parseInt in manageChainIndexers.ts.
  • Simplified access to properties in getChainIndexer.ts.
  • Implemented tests for getBlockTimeSeconds in getBlockTime.test.ts.
  • Removed CONTRACT_SUBSCRIPTIONS_DELAY_SECONDS from environment variables in env.ts.
  • Removed Redis dependency checks in chainIndexerListener.ts.
  • Updated block time estimation logic in chainIndexer.ts.
  • Refactored block timestamp retrieval in getBlockTime.ts.
  • Improved error handling and logging throughout the worker tasks.
  • Consolidated contract subscription handling in chainIndexer.ts.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@@ -69,10 +69,6 @@ export const env = createEnv({
SDK_BATCH_TIME_LIMIT: z.coerce.number().default(0),
SDK_BATCH_SIZE_LIMIT: z.coerce.number().default(100),
ENABLE_KEYPAIR_AUTH: boolEnvSchema(false),
CONTRACT_SUBSCRIPTIONS_DELAY_SECONDS: z.coerce
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experimental + undocumented. Not useful.

1: 12,
137: 2,
} as Record<number, number>;
export const getBlockTimeSeconds = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifies logic + moves to v5.

Previously this code was summing the timestamps for 100 blocks. Now it just takes the diff between the n and n-100 block, which should be equivalent.


// Compute block offset based on delay.
// Example: 10s delay with a 3s block time = 4 blocks offset
const toBlockOffset = env.CONTRACT_SUBSCRIPTIONS_DELAY_SECONDS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this. Unused.

Comment on lines -26 to -30
const blocksIn5Seconds = Math.round((1 / blockTimeSeconds) * 5);
const maxBlocksToIndex = Math.max(
config.maxBlocksToIndex,
blocksIn5Seconds * 4,
);
Copy link
Contributor Author

@arcoraven arcoraven Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was more complicated than it's worth. The goal was to find a good range of blocks to query. Honestly if we're realtime we're usually querying 5 seconds worth of blocks (2-20 depending on the block time). Hard coding this to 500: a value greater than this but less than most eth_getLogs limits should suffice.


const chainIndexerTask = async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes:

  • This function no longer returns a function, it just executes the logic.
  • Remove try/catch, already caught above.
  • Move to v5 SDK

@arcoraven arcoraven merged commit 224bd8b into main Nov 6, 2024
5 checks passed
@arcoraven arcoraven deleted the ph/simplifyCS branch November 6, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants