feat: add skip_block_scan parameter #3578
Conversation
gartnera
added
UPGRADE_TESTS
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request replaces legacy deposit functions with new deployer methods for Ether and ERC20 tokens across multiple test and runner modules. It also introduces a new boolean field, Changes
Sequence Diagram(s)sequenceDiagram
participant TestRoutine
participant E2ERunner
participant ETHDeposit
TestRoutine->>E2ERunner: DepositEtherDeployer()
E2ERunner->>E2ERunner: Acquire Lock
E2ERunner->>ETHDeposit: Call ETHDeposit() with deposit amount
ETHDeposit-->>E2ERunner: Return Transaction Hash
E2ERunner-->>TestRoutine: Return Deposited Tx Hash
sequenceDiagram
participant AppModule
participant Migrator
participant MigrationV11
AppModule->>Migrator: Register Migration (v10 → v11)
Migrator->>MigrationV11: MigrateStore(ctx, observerKeeper)
MigrationV11-->>Migrator: Updated Chain Params (with SkipBlockScan)
Migrator-->>AppModule: Migration Complete
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
force-pushed
the
observer-skip-block-scan
branch
from
179b307 to
9b26732
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3578 +/- ##
===========================================
- Coverage 64.69% 64.68% -0.01%
===========================================
Files 466 467 +1
Lines 32477 32499 +22
===========================================
+ Hits 21010 21021 +11
- Misses 10512 10520 +8
- Partials 955 958 +3
|
gartnera
force-pushed
the
observer-skip-block-scan
branch
from
3d4f3e8 to
218b519
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
x/observer/types/chain_params.go (1)
446-464:⚠️ Potential issueChainParamsEqual missing check for SkipBlockScan field
The
ChainParamsEqualfunction should be updated to include a comparison of theSkipBlockScanfield to ensure proper equality checks.func ChainParamsEqual(params1, params2 ChainParams) bool { return params1.ChainId == params2.ChainId && params1.ConfirmationCount == params2.ConfirmationCount && params1.ZetaTokenContractAddress == params2.ZetaTokenContractAddress && params1.ConnectorContractAddress == params2.ConnectorContractAddress && params1.Erc20CustodyContractAddress == params2.Erc20CustodyContractAddress && params1.InboundTicker == params2.InboundTicker && params1.OutboundTicker == params2.OutboundTicker && params1.WatchUtxoTicker == params2.WatchUtxoTicker && params1.GasPriceTicker == params2.GasPriceTicker && params1.OutboundScheduleInterval == params2.OutboundScheduleInterval && params1.OutboundScheduleLookahead == params2.OutboundScheduleLookahead && params1.BallotThreshold.Equal(params2.BallotThreshold) && params1.MinObserverDelegation.Equal(params2.MinObserverDelegation) && params1.IsSupported == params2.IsSupported && params1.GatewayAddress == params2.GatewayAddress && + params1.SkipBlockScan == params2.SkipBlockScan && confirmationParamsEqual(params1.ConfirmationParams, params2.ConfirmationParams) }x/observer/module.go (1)
185-185:⚠️ Potential issueConsensusVersion needs to be updated
The
ConsensusVersion()method still returns10despite adding a migration from 10 to 11. This should be updated to return11to reflect the latest version after migration.// ConsensusVersion implements ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 10 } +func (AppModule) ConsensusVersion() uint64 { return 11 }
🧹 Nitpick comments (4)
docs/openapi/openapi.swagger.yaml (1)
59835-59840: Well-Defined Schema Addition forskip_block_scan
The new boolean property is clearly defined with an appropriate description detailing its effect, notably disabling transfers directly to the TSS address when block scanning is skipped. This addition aligns with the PR objectives and appears consistent with the overall API schema design.Suggestions:
- Default Value Consideration: If a default behavior (e.g., always enabled on local networks) is desired, including a
defaultfield could enhance clarity for API consumers.- Consistency Verification: Ensure that the naming and formatting of
skip_block_scanare consistent with similar properties across the specification.x/observer/migrations/v11/migrate.go (2)
43-51: Comment doesn't match implementation purpose.The comment "set new fields to the same value as 'confirmation_count'" doesn't match what the code is actually doing, which is setting the SkipBlockScan field based on chain ID.
Update the comment to better reflect the actual implementation:
- // set new fields to the same value as 'confirmation_count' + // set SkipBlockScan field to true for chains in the skipBlockScanChainIDs list🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: x/observer/migrations/v11/migrate.go#L46
Added line #L46 was not covered by tests
35-62:❓ Verification inconclusive
Implementation of migration logic with missing test coverage for error scenarios.
The migration function properly implements the required changes to set SkipBlockScan based on chain IDs. However, several error paths lack test coverage.
Consider adding test coverage for the following scenarios:
- GetChainParamsList returning not found
- Handling nil chain parameters
- Validation errors when updating chain parameters
🏁 Script executed:
#!/bin/bash # Check test coverage for error paths in migrate.go # Find the test file and check if it covers error scenarios rg -A 3 -B 3 "ErrChainParamsNotFound|continue|Validate" x/observer/migrations/v11/migrate_test.goLength of output: 99
Please add comprehensive test coverage for error scenarios in the migration logic.
The migration code in
x/observer/migrations/v11/migrate.gocorrectly handles settingSkipBlockScanbased on chain IDs. However, our preliminary search did not reveal tests for critical error paths. Specifically, consider enhancing your test suite to cover the following cases:
- Case 1: When
observerKeeper.GetChainParamsList(ctx)returns not found.- Case 2: When a chain parameter entry is
nilso that the loop correctly skips processing.- Case 3: When the updated list of chain parameters fails validation via
allChainParams.Validate().Please verify that these error conditions are appropriately tested to prevent potential regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 40-41: x/observer/migrations/v11/migrate.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 46-46: x/observer/migrations/v11/migrate.go#L46
Added line #L46 was not covered by tests
[warning] 55-56: x/observer/migrations/v11/migrate.go#L55-L56
Added lines #L55 - L56 were not covered by testsx/observer/migrations/v11/migrate_test.go (1)
26-57: Comprehensive happy path test for migration.The test thoroughly verifies that the migration function correctly sets the SkipBlockScan parameter for the appropriate chains. It checks both positive (should be true) and negative (should be false) cases.
Consider adding tests for error scenarios such as:
- Empty chain params list
- Invalid chain params that fail validation
- Nil chain params in the list
This would improve the overall test coverage of the migration function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
typescript/zetachain/zetacore/observer/params_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/observer/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/params.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (19)
cmd/zetae2e/local/admin.go(1 hunks)cmd/zetae2e/local/bitcoin.go(1 hunks)cmd/zetae2e/local/performance.go(1 hunks)cmd/zetae2e/stress.go(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)docs/spec/observer/messages.md(1 hunks)e2e/e2etests/test_migrate_chain_support.go(1 hunks)e2e/runner/evm.go(2 hunks)e2e/runner/legacy_setup_evm.go(1 hunks)proto/zetachain/zetacore/observer/params.proto(1 hunks)proto/zetachain/zetacore/observer/tx.proto(1 hunks)x/observer/keeper/migrator.go(2 hunks)x/observer/keeper/msg_server_update_operational_chain_params.go(1 hunks)x/observer/keeper/msg_server_update_operational_chain_params_test.go(3 hunks)x/observer/migrations/v11/migrate.go(1 hunks)x/observer/migrations/v11/migrate_test.go(1 hunks)x/observer/module.go(1 hunks)x/observer/types/chain_params.go(1 hunks)zetaclient/chains/evm/observer/inbound.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/observer/params.protoproto/zetachain/zetacore/observer/tx.proto
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_update_operational_chain_params.gox/observer/module.gox/observer/keeper/msg_server_update_operational_chain_params_test.gox/observer/keeper/migrator.goe2e/runner/legacy_setup_evm.goe2e/e2etests/test_migrate_chain_support.gox/observer/types/chain_params.gocmd/zetae2e/local/admin.gocmd/zetae2e/local/bitcoin.gocmd/zetae2e/stress.gox/observer/migrations/v11/migrate_test.gox/observer/migrations/v11/migrate.gozetaclient/chains/evm/observer/inbound.gocmd/zetae2e/local/performance.goe2e/runner/evm.go
🪛 markdownlint-cli2 (0.17.2)
docs/spec/observer/messages.md
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: codecov/patch
x/observer/migrations/v11/migrate.go
[warning] 40-41: x/observer/migrations/v11/migrate.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 46-46: x/observer/migrations/v11/migrate.go#L46
Added line #L46 was not covered by tests
[warning] 55-56: x/observer/migrations/v11/migrate.go#L55-L56
Added lines #L55 - L56 were not covered by tests
zetaclient/chains/evm/observer/inbound.go
[warning] 144-144: zetaclient/chains/evm/observer/inbound.go#L144
Added line #L144 was not covered by tests
[warning] 150-150: zetaclient/chains/evm/observer/inbound.go#L150
Added line #L150 was not covered by tests
[warning] 153-159: zetaclient/chains/evm/observer/inbound.go#L153-L159
Added lines #L153 - L159 were not covered by tests
[warning] 163-163: zetaclient/chains/evm/observer/inbound.go#L163
Added line #L163 was not covered by tests
🔇 Additional comments (24)
cmd/zetae2e/local/performance.go (1)
92-92: Method migration to deployer pattern looks good.The change from
LegacyDepositEther()toDepositEtherDeployer()aligns with the PR objective to replace legacy deposit functions. This modification supports the addition of theskip_block_scanparameter functionality.cmd/zetae2e/local/admin.go (1)
48-49: Consistent implementation of the deployer pattern.The replacement of legacy deposit methods with their deployer equivalents follows the same pattern as in other files. This ensures consistency across the codebase and supports the new
skip_block_scanparameter functionality.cmd/zetae2e/stress.go (1)
145-145: Legacy method replacement is appropriate.The change from
LegacyDepositEther()toDepositEtherDeployer()in the stress test aligns with the pattern applied throughout the codebase. This modification properly integrates with the addition of theskip_block_scanparameter.cmd/zetae2e/local/bitcoin.go (1)
161-162: Method updates align with overall migration strategy.The changes to use deployer-based deposit methods are consistent with modifications in other files. This standardizes the approach across different test scenarios and supports the new block scanning behavior configuration.
e2e/e2etests/test_migrate_chain_support.go (1)
185-185: Excellent migration to the new depositor API.The change from
LegacyDepositERC20()toDepositERC20Deployer()aligns with the PR objective of replacing legacy deposit functions with new deployer methods. This ensures consistency with the newskip_block_scanparameter implementation.e2e/runner/evm.go (3)
26-28: Good thread safety implementation.Adding mutex locks to the
ETHDepositmethod ensures thread safety during concurrent access. This is a critical improvement considering these methods might be called from multiple goroutines in test scenarios.
44-49: Well-structured new deployer method.The
DepositEtherDeployermethod is clean and focused, handling the deposit of Ether into ZEVM using V2 protocol contracts. It sets a reasonable deposit amount (100 ETH) and properly returns the transaction hash for further tracking.
51-60: Clean implementation of ERC20 deposit deployer.The
DepositERC20Deployermethod properly handles ERC20 deposits by:
- Logging the operation
- Approving the ERC20 token on EVM
- Setting the deposit amount
- Returning the transaction hash
This implementation aligns with the PR's objective of replacing legacy deposit functions with modern deployer methods.
docs/spec/observer/messages.md (1)
204-204: The new parameter adds flexibility to the chain observation mechanism.The introduction of the
skip_block_scanboolean parameter appropriately extends the operational chain parameters to allow fine-grained control over block scanning behavior. This optimization will likely reduce resource consumption in environments where scanning individual blocks is unnecessary.Note: Replace the hard tab with spaces to maintain consistent formatting throughout the document.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
204-204: Hard tabs
Column: 1(MD010, no-hard-tabs)
x/observer/keeper/msg_server_update_operational_chain_params.go (1)
44-44: Properly implemented the new parameter handling.The implementation correctly assigns the
SkipBlockScanvalue from the incoming message to the chain parameters. This follows the established pattern for other operational parameters and ensures the feature is consistently applied.proto/zetachain/zetacore/observer/params.proto (1)
37-40: Well-documented protocol extension with clear purpose.The new
skip_block_scanfield is properly positioned as the last field in the message (field number 19) and includes informative comments about its purpose and behavior. The documentation clarifies that this primarily affects transfers directly to the TSS address, which is valuable information for implementers and users.x/observer/keeper/migrator.go (2)
7-7: Import added for new migration module.The import for the v11 migration package is correctly added to support the new migration function.
66-69: Migration function follows established pattern.The
Migrate10to11function correctly follows the established pattern in this file, delegating to the specialized migration module while passing the appropriate context and keeper reference. This maintains code consistency and separation of concerns.x/observer/types/chain_params.go (1)
424-424: New field SkipBlockScan added as per PR requirementsThe addition of the
SkipBlockScanfield toGetDefaultGoerliLocalnetChainParamsis aligned with the PR objective to introduce a parameter that allows bypassing of block scanning operations. Setting it totruefor the local network is appropriate.proto/zetachain/zetacore/observer/tx.proto (1)
88-88: Appropriate addition of skip_block_scan fieldThe addition of the
skip_block_scanfield to theMsgUpdateOperationalChainParamsmessage is correctly implemented with proper field numbering (10). This matches the PR objective of introducing this parameter for controlling block scanning behavior.e2e/runner/legacy_setup_evm.go (1)
140-140: Setting SkipBlockScan to false for legacy testsSetting
SkipBlockScantofalsein the legacy setup aligns with the PR objective of ensuring comprehensive test coverage across different parameter combinations. This configuration ensures that block scanning functionality is fully tested in legacy scenarios.x/observer/module.go (1)
158-160: Migration registration added correctlyThe addition of the migration from version 10 to 11 is correctly implemented, following the same pattern as previous migrations.
x/observer/keeper/msg_server_update_operational_chain_params_test.go (3)
4-5: Organizational improvement in import arrangement.The import statement for "testing" has been moved to its own line, which improves code organization and readability. This is a minor but positive change to the import block structure.
148-148: Added support for the new SkipBlockScan parameter.The SkipBlockScan parameter has been appropriately included in the test message structure, setting it to true in this test case.
166-166: Comprehensive validation of the new SkipBlockScan parameter.A new assertion has been added to verify that the original SkipBlockScan value differs from the updated value in the message. This ensures proper testing of the parameter's modification functionality.
x/observer/migrations/v11/migrate.go (2)
13-16: Well-defined interface for the observer keeper.The interface is concisely defined with only the necessary methods required for the migration, adhering to the interface segregation principle.
18-33: Clear documentation of chains with block scanning disabled.The skipBlockScanChainIDs variable explicitly documents which chains will have block scanning disabled, with appropriate categorization into localnet, testnets, and mainnets.
zetaclient/chains/evm/observer/inbound.go (1)
143-151: Initialization change for lastScannedTssRecvd and error variable.The initialization of lastScannedTssRecvd has been changed to toBlock, and an error variable has been added to the variable declaration block in preparation for the conditional logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-144: zetaclient/chains/evm/observer/inbound.go#L144
Added line #L144 was not covered by tests
[warning] 150-150: zetaclient/chains/evm/observer/inbound.go#L150
Added line #L150 was not covered by testsx/observer/migrations/v11/migrate_test.go (1)
14-24: Well-structured test data setup function.The getTestChainParams function provides a clear and concise way to create test data, using a variety of chain types for comprehensive testing.
| // we now only take these actions on specific configurable chains | ||
| if !ob.ChainParams().SkipBlockScan { | ||
| // query the incoming tx to TSS address (read at most 100 blocks in one go) | ||
| lastScannedTssRecvd, err = ob.ObserveTSSReceive(ctx, startBlock, toBlock) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error observing TSS received gas asset") | ||
| } | ||
|
|
||
| // filter the outbounds from TSS address to supplement outbound trackers | ||
| // TODO: make this a separate go routine in outbound.go after switching to smart contract V2 | ||
| ob.FilterTSSOutbound(ctx, startBlock, toBlock) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🛠️ Refactor suggestion
❓ Verification inconclusive
Implementation of conditional block scanning based on chain configuration.
This change adds an important optimization by conditionally executing block scanning operations based on the SkipBlockScan parameter. However, there's a lack of test coverage for this significant change.
- The code lacks test coverage which is essential for a critical optimization like this.
- Consider adding a comment explaining the performance implications and expected behavior when block scanning is skipped.
🏁 Script executed:
#!/bin/bash
# Check for tests covering the new SkipBlockScan functionality in observer
# Look for tests that might be testing this functionality
rg -A 3 "SkipBlockScan.*Observer|TestObserve.*SkipBlockScan" zetaclient/Length of output: 74
Missing Test Coverage for Conditional Block Scanning
The new conditional block scanning controlled by the SkipBlockScan flag introduces a significant optimization. However, our recent search did not yield any tests that explicitly cover this new behavior. It is important to address the following:
- Test Coverage: Please add tests to validate both scenarios when
SkipBlockScanis set to true (skipping block scan) and false (performing block scan). - Documentation: Consider including an inline comment in
zetaclient/chains/evm/observer/inbound.go(lines 153–164) that explains the performance implications and expected behavior when block scanning is skipped.
These additions will strengthen confidence in this critical optimization.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 153-159: zetaclient/chains/evm/observer/inbound.go#L153-L159
Added lines #L153 - L159 were not covered by tests
[warning] 163-163: zetaclient/chains/evm/observer/inbound.go#L163
Added line #L163 was not covered by tests
|
|
||
| // Skip actions that require scanning the contents of each block. | ||
| // The main thing this disables is transfers directly to the TSS address. | ||
| bool skip_block_scan = 19; |
There was a problem hiding this comment.
Wouldn't disable better wording than skip?
Skip sounds like skipping an optional step in a process for me, while this disable a feature by itself
|
|
||
| var skipBlockScanChainIDs = []int64{ | ||
| // localnet | ||
| chains.GoerliLocalnet.ChainId, |
There was a problem hiding this comment.
I suggest to keep it for the local evm because parsing all blocks will still be supported for at least one evm chains on each live network
There was a problem hiding this comment.
As I said in the description it's covered by the legacy tests. We reenable block scanning when running the legacy tests. The legacy tests continue to use the legacy eth and erc20 deposit methods.
Is this not sufficient?
update operational chain params
gartnera
force-pushed
the
observer-skip-block-scan
branch
from
fc4fdf1 to
1b755c0
Compare
| @@ -201,6 +201,7 @@ message MsgUpdateOperationalChainParams { | |||
| int64 outbound_schedule_interval = 7; | |||
| int64 outbound_schedule_lookahead = 8; | |||
| ConfirmationParams confirmation_params = 9; | |||
| bool disable_block_scan = 10; | |||
There was a problem hiding this comment.
Maybe disable_direct_tss_deposit is a more explicit name for this purpose?
There was a problem hiding this comment.
We are also skipping filterTSSOutbound. Doesn't really matter to me.
There was a problem hiding this comment.
I think filterTSSOutbound is a pure supplement to outbound trackers. This can be moved out to outbound.go in future and just use cached blocks (without RPC call).
There was a problem hiding this comment.
You'd need to make sure you only run it after you've already run ob.observeTSSReceive or they would race populating the block cache.
If you made filterTSSOutbound only read from the cache you'd still have to make sure it only ran after ob.observeTSSReceive had been run.
e2e/runner/evm.go
Outdated
| @@ -38,6 +41,24 @@ func (r *E2ERunner) ETHDeposit( | |||
| return tx | |||
| } | |||
|
|
|||
| // DepositEther sends Ethers into ZEVM using V2 protocol contracts | |||
There was a problem hiding this comment.
| // DepositEther sends Ethers into ZEVM using V2 protocol contracts | |
| // DepositEtherDeployer sends Ethers into ZEVM using V2 protocol contracts |
| return errorsmod.Wrap(types.ErrChainParamsNotFound, "failed to get chain params") | ||
| } | ||
|
|
||
| // set new fields to the same value as 'confirmation_count' |
There was a problem hiding this comment.
please update the comment to reflect the logic
|
|
||
| // Skip actions that require scanning the contents of each block. | ||
| // The main thing this disables is transfers directly to the TSS address. | ||
| bool disable_block_scan = 19; |
There was a problem hiding this comment.
Would disable_tss_inbound , be more appropriate.
Will we use this param for any other checks in the future?
There was a problem hiding this comment.
I'm having the same discussion on this thread: #3578 (comment)
If you and charlie want more like disable_tss_inbound that is fine by me.
| chains.Amoy.ChainId, | ||
|
|
||
| // mainnets | ||
| // the other mainnets will be deprecated later |
There was a problem hiding this comment.
Does this need to be tracked via an issue ?
| require.True(t, found) | ||
| require.Len(t, newChainParams.ChainParams, len(oldChainParams.ChainParams)) | ||
|
|
||
| for _, params := range newChainParams.ChainParams { |
There was a problem hiding this comment.
NIT: Maybe add a check to make other values are identical
Add observer parameter to skip actions which require scanning individual blocks.
Set this by default on localnet. Remove legacy deposit usage in normal tests. Set
skip_block_scan = falsewhen running the legacy tests. This should give us full coverage of the combinations of this parameter.Migrate the observer params so that the parameter is set correctly for all chains. We will immediately turn off block scanning for everything on testnet except ethereum. We will announce deprecation timeline on mainnet and process via normal group proposals.
Closes #3549
Closes #3579
Related to #3525
Summary by CodeRabbit
New Features
Documentation
Tests & Chores