feat: add feature flag for multiple evm calls #4288
Conversation
|
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a feature flag to allow multiple EVM calls within a single transaction and threads context through EVM inbound parsing. Adds per-transaction event filtering when the flag is disabled. Updates config to include default feature flags, logs them at orchestrator startup, adjusts tests to expect more inbounds, and updates TestDAppV2 bytecode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EVM as EVM Node
participant Obs as Observer
participant Cfg as Config
participant Trk as Inbound Tracker
rect rgb(245,245,255)
note over Obs,Cfg: Feature flag lookup
EVM->>Obs: Logs (Deposited / Called / DepositedAndCalled)
Obs->>Cfg: IsEnableMultipleCallsEnabled()
Cfg-->>Obs: bool allowMultiple
end
alt allowMultiple == true
note right of Obs: No per-tx filtering
Obs->>Obs: Parse all events with ctx
Obs->>Trk: PostVoteInbound for each event
Trk-->>Obs: ack/error (proceed regardless)
else allowMultiple == false
note right of Obs: Per-tx filtering (keep first event per tx)
Obs->>Obs: Filter events by tx hash
Obs->>Trk: PostVoteInbound (filtered set)
Trk-->>Obs: On error, halt processing
end
sequenceDiagram
autonumber
participant App as App
participant Orc as Orchestrator
participant Cfg as Config
App->>Orc: Start()
Orc->>Cfg: GetFeatureFlags()
Cfg-->>Orc: FeatureFlags{EnableMultipleCalls: bool}
Orc->>Orc: logFeatureFlags(...)
Orc-->>App: Startup continues
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4288 +/- ##
===========================================
- Coverage 66.08% 65.98% -0.11%
===========================================
Files 461 462 +1
Lines 33639 33720 +81
===========================================
+ Hits 22232 22249 +17
- Misses 10425 10488 +63
- Partials 982 983 +1
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
zetaclient/orchestrator/orchestrator.go (1)
389-402: Consider consolidating log messages.The method logs feature flags twice: once with structured fields (line 393-395) and again with conditional human-readable messages (lines 397-401). Consider consolidating into a single log entry with both structured and human-readable information.
Apply this diff to consolidate logging:
func (oc *Orchestrator) logFeatureFlags(config config.Config) { flags := config.GetFeatureFlags() - - oc.logger.Info(). - Bool("enable_multiple_calls", flags.EnableMultipleCalls). - Msg("feature flags status") - + + var msg string if config.IsEnableMultipleCallsEnabled() { - oc.logger.Info().Msg("EnableMultipleCalls is enabled - multiple calls from same tx will be allowed") + msg = "EnableMultipleCalls is enabled - multiple calls from same tx will be allowed" } else { - oc.logger.Info().Msg("EnableMultipleCalls is disabled - multiple calls from same tx will be filtered (only first event)") + msg = "EnableMultipleCalls is disabled - multiple calls from same tx will be filtered (only first event)" } + + oc.logger.Info(). + Bool("enable_multiple_calls", flags.EnableMultipleCalls). + Msg(msg) }e2e/contracts/testdappv2/TestDAppV2.sol (2)
276-287: Consume the full amount and avoid wei dust from integer division.Splitting
amountinto fouramount/4transfers can leaveamount % 4wei unspent. Allocate the remainder to the last transfer to keep accounting exact.- IGatewayEVM(gateway).deposit{value: amount / 4 }(dst, amount / 4, RevertOptions(msg.sender, false, address(0), "", 0)); + uint256 q = amount / 4; + uint256 r = amount - 3 * q; // remainder goes to the last deposit/call + IGatewayEVM(gateway).deposit{value: q}(dst, q, RevertOptions(msg.sender, false, address(0), "", 0)); @@ - IGatewayEVM(gateway).deposit{value: amount / 4 + additionalFee }(dst, amount / 4, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).deposit{value: q + additionalFee}(dst, q, RevertOptions(msg.sender, false, address(0), "", 0)); @@ - IGatewayEVM(gateway).depositAndCall{value: amount / 4 + additionalFee }(dst, amount / 4, payload, RevertOptions(msg.sender, false, address(0), "", 0)); - IGatewayEVM(gateway).depositAndCall{value: amount / 4 + additionalFee }(dst, amount / 4, payload, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).depositAndCall{value: q + additionalFee}(dst, q, payload, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).depositAndCall{value: r + additionalFee}(dst, r, payload, RevertOptions(msg.sender, false, address(0), "", 0));
296-304: Mirror dust handling and validateassetAmount.Same integer-division dust applies to ERC20. Optionally guard very small amounts to avoid zero-amount deposits on tokens with low values.
- IGatewayEVM(gateway).deposit(dst, assetAmount / 4, asset, RevertOptions(msg.sender, false, address(0), "", 0)); + uint256 q = assetAmount / 4; + uint256 r = assetAmount - 3 * q; + IGatewayEVM(gateway).deposit(dst, q, asset, RevertOptions(msg.sender, false, address(0), "", 0)); @@ - IGatewayEVM(gateway).deposit{ value: additionalFee }(dst, assetAmount / 4, asset, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).deposit{ value: additionalFee }(dst, q, asset, RevertOptions(msg.sender, false, address(0), "", 0)); @@ - IGatewayEVM(gateway).depositAndCall{ value: additionalFee }(dst, assetAmount / 4, asset, payload, RevertOptions(msg.sender, false, address(0), "", 0)); - IGatewayEVM(gateway).depositAndCall{ value: additionalFee }(dst, assetAmount / 4, asset, payload, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).depositAndCall{ value: additionalFee }(dst, q, asset, payload, RevertOptions(msg.sender, false, address(0), "", 0)); + IGatewayEVM(gateway).depositAndCall{ value: additionalFee }(dst, r, asset, payload, RevertOptions(msg.sender, false, address(0), "", 0));Optionally:
+ require(assetAmount >= 4, "assetAmount too small");zetaclient/chains/evm/observer/v2_inbound.go (5)
154-159: Flag-driven filtering: confirm fallback behavior.If
FromContextfails,shouldAllowMultipleCallsfalls back to filtering. This preserves legacy behavior but may contradict a default-enabled flag. Confirm this is intentional for safety.Consider documenting this explicitly in a code comment here, or defaulting to allow when the global default is enabled by config bootstrap.
162-185: Reduce noisy warnings and cache the decision per batch.
Warnon missing app context can spam logs during scans. PreferDebugand compute once per observe call.- ob.Logger().Inbound.Warn().Err(err).Msg("unable to get app context, using default behavior") + ob.Logger().Inbound.Debug().Err(err).Msg("app context not set; defaulting to single-event filtering")Also cache:
- func (ob *Observer) shouldAllowMultipleCalls(ctx context.Context) bool { + func (ob *Observer) shouldAllowMultipleCalls(ctx context.Context) bool { // Optionally: memoize in ctx or ob for this batch if needed. }
188-206: Consistent log levels and lean map types.Use a consistent level across filters (Info vs Warn differs between deposit/call). Prefer
Debugto avoid log noise in healthy paths. Also usemap[string]struct{}to minimize allocations.- guard := make(map[string]bool) + guard := make(map[string]struct{}) @@ - if guard[event.Raw.TxHash.Hex()] { - ob.Logger().Inbound.Info(). + if _, ok := guard[event.Raw.TxHash.Hex()]; ok { + ob.Logger().Inbound.Debug(). Stringer(logs.FieldTx, event.Raw.TxHash). Msg("multiple Deposited events in same tx") continue } - guard[event.Raw.TxHash.Hex()] = true + guard[event.Raw.TxHash.Hex()] = struct{}{}Apply same pattern to
filterCallEventsByTxandfilterDepositAndCallEventsByTxfor uniformity.
326-331: Symmetry is good; add a metric for filtered events.Good parity with deposit filtering. Add a counter for dropped events to aid rollout monitoring of the feature flag.
472-490: Uniform logging and preallocation.Apply the same logging level and guard-map changes here; also preallocate
filteredwithlen(validEvents)capacity to reduce reallocations.- filtered := make([]*gatewayevm.GatewayEVMDepositedAndCalled, 0) + filtered := make([]*gatewayevm.GatewayEVMDepositedAndCalled, 0, len(validEvents))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
e2e/contracts/testdappv2/TestDAppV2.binis excluded by!**/*.bin
📒 Files selected for processing (12)
changelog.md(1 hunks)e2e/contracts/testdappv2/TestDAppV2.go(1 hunks)e2e/contracts/testdappv2/TestDAppV2.json(1 hunks)e2e/contracts/testdappv2/TestDAppV2.sol(2 hunks)e2e/e2etests/test_erc20_multiple_deposits.go(3 hunks)e2e/e2etests/test_eth_multiple_deposits.go(2 hunks)e2e/e2etests/test_inbound_trackers.go(3 hunks)zetaclient/chains/evm/observer/v2_inbound.go(10 hunks)zetaclient/chains/evm/observer/v2_inbound_tracker.go(4 hunks)zetaclient/config/config_chain.go(2 hunks)zetaclient/config/types.go(3 hunks)zetaclient/orchestrator/orchestrator.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
e2e/e2etests/test_inbound_trackers.gozetaclient/config/types.gozetaclient/orchestrator/orchestrator.goe2e/e2etests/test_erc20_multiple_deposits.gozetaclient/chains/evm/observer/v2_inbound_tracker.gozetaclient/config/config_chain.goe2e/e2etests/test_eth_multiple_deposits.goe2e/contracts/testdappv2/TestDAppV2.gozetaclient/chains/evm/observer/v2_inbound.go
🧬 Code graph analysis (5)
zetaclient/orchestrator/orchestrator.go (1)
zetaclient/config/types.go (1)
Config(85-120)
e2e/e2etests/test_erc20_multiple_deposits.go (1)
e2e/utils/zetacore.go (1)
WaitCctxsMinedByInboundHash(89-167)
zetaclient/config/config_chain.go (1)
zetaclient/config/types.go (1)
FeatureFlags(77-80)
e2e/e2etests/test_eth_multiple_deposits.go (1)
e2e/utils/zetacore.go (1)
WaitCctxsMinedByInboundHash(89-167)
zetaclient/chains/evm/observer/v2_inbound.go (3)
zetaclient/context/context.go (1)
FromContext(19-26)zetaclient/config/types.go (1)
Config(85-120)zetaclient/logs/fields.go (1)
FieldTx(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (26)
e2e/contracts/testdappv2/TestDAppV2.json (1)
523-523: Contract bytecode update acknowledged.The
Binfield contains updated bytecode while the ABI remains unchanged, indicating a recompilation of the TestDAppV2 contract. This is consistent with the introduction of multiple-call functionality described in the PR objectives.Since this is an auto-generated file, ensure the bytecode was regenerated from the updated Solidity source and that the deployment/test artifacts remain consistent across the e2e suite.
changelog.md (1)
13-13: Changelog entry is appropriate.The addition clearly documents the feature flag introduction for PR 4288, providing users with visibility into the configuration-driven multiple EVM calls capability.
e2e/contracts/testdappv2/TestDAppV2.go (1)
55-55: Auto-generated Go binding updated correctly.The bytecode in
TestDAppV2MetaData.Binmatches the updated contract binary. All Go binding methods and structures remain unchanged, confirming ABI compatibility.zetaclient/config/config_chain.go (2)
19-19: Feature flag initialization added to config constructor.The
FeatureFlagsfield is now initialized with default values in theNewconstructor, ensuring consistent feature flag state across all zetaclient instances.
83-88: Ensure production override for EnableMultipleCalls
defaultFeatureFlags()currently enables multiple calls by default—either change the default tofalseor explicitly override tofalsein your production deployment configs.- Update your migration/documentation to call out this default behavior.
- Verify
Observer.shouldAllowMultipleCallsin v2_inbound.go handles both enabled and disabled states correctly.e2e/e2etests/test_eth_multiple_deposits.go (3)
28-29: Fee calculation updated for six inbounds.The payable amount now adds five fees to cover six inbound transactions (first is free), correctly reflecting the expanded multiple-call behavior enabled by the feature flag.
40-46: Test expectations expanded to six CCTXs.The test now waits for and logs six cross-chain transactions instead of three, consistent with the feature flag enabling multiple calls per transaction. The labeling clearly distinguishes between deposits, deposit-and-calls, and plain calls.
51-53: Resolved: gatewayMultipleDeposits emits six invocations matching test assertions
Verified thatgatewayMultipleDepositscalls IGatewayEVM six times (1×deposit, 1×deposit with fee, 2×depositAndCall, 2×call), producing six outbound events—no changes required.zetaclient/chains/evm/observer/v2_inbound_tracker.go (4)
70-72: Review early-return logic for correctness.The condition
if err != nil || !allowMultipleCallschanges previous behavior. Previously, any error would cause an immediate return. Now, the function also returns early whenallowMultipleCallsis false, even on successful vote posting.This means when the feature flag is disabled, only the first event in a transaction is processed. Verify this aligns with the intended behavior: processing should halt after the first successful vote when multiple calls are not allowed.
91-93: Consistent early-return pattern applied.The same early-return logic is applied here for deposit-and-call events, maintaining consistency with the deposit path.
112-114: Consistent early-return pattern applied.The same early-return logic is applied here for call events, maintaining consistency across all three event types.
44-45: No action needed for shouldAllowMultipleCalls implementation
TheshouldAllowMultipleCallsmethod is already defined onObserverinv2_inbound.goand correctly retrieves theEnableMultipleCallsfeature flag.Likely an incorrect or invalid review comment.
zetaclient/orchestrator/orchestrator.go (2)
21-21: Import addition approved.The config package import is required for the new
logFeatureFlagsmethod.
102-103: Feature flag logging at startup approved.Adding visibility into feature flag status during orchestrator startup is a good operational practice, aiding in debugging and configuration validation.
e2e/e2etests/test_erc20_multiple_deposits.go (3)
4-4: Import addition approved.The
math/bigimport is required for the fee multiplication calculation.
34-35: Fee calculation updated correctly.The calculation now accounts for 4 inbounds with the first being free, requiring 3×fee. The comment accurately describes the logic.
60-61: Status assertions for new cctxs approved.The assertions verify that the additional cctxs reach OutboundMined status, properly exercising the expanded test scenario.
e2e/e2etests/test_inbound_trackers.go (4)
126-127: ETH multiple deposits fee calculation approved.The calculation correctly accounts for 6 inbounds with the first being free, requiring 5×fee added to the base amount. The comment is accurate.
133-133: CCTX count updated correctly for ETH path.The expected count of 6 cctxs aligns with the multiple deposits scenario for ETH.
144-145: ERC20 multiple deposits fee calculation approved.The calculation correctly accounts for 4 inbounds with the first being free, requiring 3×fee. The comment is accurate.
157-157: CCTX count updated correctly for ERC20 path.The expected count of 4 cctxs aligns with the multiple deposits scenario for ERC20.
zetaclient/config/types.go (4)
76-80: FeatureFlags struct design approved.The struct is well-designed with clear naming and proper JSON tagging. The comment accurately describes its purpose.
116-117: FeatureFlags field integration approved.The field is properly integrated into the Config struct with appropriate JSON tagging and documentation comment.
238-243: Accessor method implementation approved.The
GetFeatureFlagsmethod follows the established pattern of using read locks for thread-safe access, consistent with other getter methods in this file.
245-250: Convenience method implementation approved.The
IsEnableMultipleCallsEnabledmethod provides a clean, expressive API for checking the feature flag status. The implementation correctly uses read locks for thread safety.zetaclient/chains/evm/observer/v2_inbound.go (1)
464-469: OK to gate here; ensure tests cover mixed-event txs.The gating mirrors deposit and call. Ensure tests include multiple events within a single tx across all three types.
should it be in docs or changelog? |
Let's add in the changelogs, even though we don't have ZetaClient specific changelogs, I will manually add this to the ZetaClient release notes when doing the release. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@lumtis @ws4charlie i had to bring back zetae2e ante for upgrade tests to work since i modified e2e tests here please re-check when you get a chance |
Description
add feature flags to zetaclient config, and toggle multiple evm calls inbound events filtering
also improved multiple calls e2e tests, if it was 1 type of each event like before it was not testing new functionality properly
How Has This Been Tested?
Note
Adds EnableMultipleCalls feature flag to control filtering of multiple EVM gateway events per transaction and updates contracts/e2e tests to exercise multiple deposits/calls.
FeatureFlags.EnableMultipleCalls(default: true) with getters; wire into context and orchestrator logging.shouldAllowMultipleCalls; when enabled, process all events from the same tx; otherwise keep original first-event-only behavior.context.Contextand split filtering helpers (filter*ByTx).ConfigwithFeatureFlags, defaults, accessors.TestDAppV2.solmultiple deposit flows (more actions, fee math changes) and regeneratebin/go/jsonartifacts.changelog.mdwith multiple EVM calls and feature flag note.Written by Cursor Bugbot for commit 5815117. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests
Documentation
Contracts