-
Notifications
You must be signed in to change notification settings - Fork 27
node/exts/evm-sync: Add Arbitrum One, Arbitrum Sepolia and Hardhat chain support with StartBlock configuration #1642
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
base: main
Are you sure you want to change the base?
node/exts/evm-sync: Add Arbitrum One, Arbitrum Sepolia and Hardhat chain support with StartBlock configuration #1642
Conversation
WalkthroughAdds three chains (Hardhat, Arbitrum Sepolia, Arbitrum One), enables per-chain BlockSyncChunkSize and StartBlock configuration, propagates StartBlock into chainConfig, changes block/log logging level, and updates last-seen height even when no logs are found. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node/exts/evm-sync/chains/chains.go (1)
57-62: Consider naming consistency for chain identifiers.The ArbitrumSepolia constant uses
"arbitrumsepolia"(no separator) while BaseSepolia uses"base-sepolia"(hyphen). While this doesn't break functionality, consistent naming conventions improve clarity. Consider using either"arbitrum-sepolia"or updating BaseSepolia to"basesepolia"for consistency.node/exts/evm-sync/listener.go (1)
199-203: Consider clarifying the log message for better debugging.Line 199 logs "StartBlock" but actually logs the
startBlockvariable from eventstore (before applying the configured StartBlock). Consider logging after the StartBlock logic is applied, or clarifying the message to indicate it's the "initial startBlock from eventstore" to avoid confusion during debugging.Apply this diff to improve clarity:
startBlock, err := getLastSeenHeight(ctx, eventstore, i.orderedSyncTopic) if err != nil { return fmt.Errorf("failed to get last seen height: %w", err) } - logger.Infof("StartBlock of %s: %d", i.orderedSyncTopic, startBlock) // If eventstore has no last seen height (returns 0), use StartBlock from config if configured if startBlock == 0 && i.chainConf.StartBlock > 0 { startBlock = i.chainConf.StartBlock } + logger.Infof("Starting sync for %s from block: %d", i.orderedSyncTopic, startBlock)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/config.go(1 hunks)node/exts/evm-sync/chains/chains.go(3 hunks)node/exts/evm-sync/listener.go(6 hunks)
🔇 Additional comments (8)
config/config.go (1)
619-619: LGTM! StartBlock field addition is well-structured.The new field is properly annotated with TOML tags and includes a clear comment explaining its purpose and format. The validation is appropriately handled at the usage site in
listener.go.node/exts/evm-sync/chains/chains.go (2)
38-47: LGTM! New chain definitions are correctly configured.Both Hardhat (local development chain) and Arbitrum Sepolia (L2 testnet) are properly defined with accurate chain IDs and appropriate confirmation requirements.
68-75: LGTM! Valid() method correctly includes new chains.The validation logic properly recognizes Hardhat and ArbitrumSepolia as valid chains.
node/exts/evm-sync/listener.go (5)
124-124: Excellent fix! BlockSyncChunkSize now correctly uses per-chain configuration.This change ensures each chain uses its own configured chunk size rather than a hardcoded key, which is essential for supporting chains with different block production rates and history sizes.
138-149: LGTM! StartBlock parsing and validation is robust.The implementation properly reads the per-chain StartBlock configuration, provides sensible defaults, validates the value, and includes clear error messages with chain context.
169-171: LGTM! StartBlock field is well-documented.The field addition includes clear documentation explaining its purpose and default behavior.
254-254: LGTM! Log level change reduces noise.Changing the "received new block" message to Debug level is appropriate since block receipts are frequent and don't require Info-level visibility during normal operation.
280-282: Excellent bug fix! Progress tracking now works correctly for empty block ranges.This change ensures the last seen height is updated even when no logs are found in a block range. Without this fix, the listener could repeatedly process the same empty range, stalling progress and wasting resources. This is particularly important for chains or time periods with sparse events.
Time Submission Status
|
|
Hi @sapience, thank you for contributing! I can see from the PR description what you're trying to fix. Since this involves both adding a feature and fixing a bug, could you create a GitHub issue with more details? That way we can have a proper discussion about it. Also, if the PR is still in progress, please mark it as a draft since I see you're still actively committing changes. |
Description
This PR adds support for Arbitrum One, Arbitrum Sepolia and Hardhat chains to the EVM sync extension, and introduces a
StartBlockconfiguration option to specify the starting block number for chain synchronization. Additionally, it includes several bug fixes and improvements to the listener implementation.Key Changes:
hardhatchain (chain ID: 31337) with 12 required confirmationsarbitrum_onechain (chain ID: 42161) with 4 required confirmationsarbitrum_sepoliachain (chain ID: 421614) with 2 required confirmationsStartBlockconfiguration option toERC20BridgeConfigfor per-chain starting block configurationBlockSyncChuckSizeto use the actual chain parameter instead of hardcoded Ethereum referenceprocessEventsto update last seen height even when no logs are foundMotivation and Context
This change is required to:
Support additional EVM chains: Enable kwil-db to sync with Arbitrum One, Arbitrum Sepolia testnet and Hardhat local development networks, expanding the supported chain ecosystem.
Flexible sync configuration: The
StartBlockconfiguration allows operators to specify a custom starting block for initial synchronization or recovery scenarios, rather than always starting from block 0. This is particularly useful when:Bug fixes: Addresses issues where chain-specific configurations were incorrectly hardcoded and where the last seen height wasn't updated in certain edge cases.
How Has This Been Tested?
task test:unittask test:acttask fmttask lintTypes of changes
Checklist:
Checklist Explanation:
StartBlockconfiguration option is documented inline in the code with a comment. If there's external documentation that needs updating, please let me know.How to Review this PR:
Review Order:
config/config.go: Review the newStartBlockconfiguration field and its TOML commentnode/exts/evm-sync/chains/chains.go: Verify the new chain definitions (Hardhat and Arbitrum Sepolia) and chain validation logicnode/exts/evm-sync/listener.go: Focus on:StartBlockconfiguration parsing and validation logicBlockSyncChuckSize(line ~124: changed from hardcodedchains.Ethereum.String()tochain.String())Key Design Decisions:
StartBlockdefaults to 0 if not configured, maintaining backward compatibilityStartBlockis only used when eventstore has no last seen height (returns 0), ensuring it doesn't override existing sync stateAdditional Information:
Configuration Example:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.