feat: support for fast outbound confirmation for EVM/Bitcoin chain #3619
Conversation
…t-fast-confirmation-inbound-evm-btc
…st confirmation E2E
…lace argument type string with common.Address
…set string with common.Address
…ove divisor related file to chains pkg; renaming and comments
…t-fast-confirmation-inbound-evm-btc
Co-authored-by: Lucas Bertrand <[email protected]>
…t-fast-confirmation-inbound-evm-btc
…t-fast-confirmation-outbound-evm-btc
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
ws4charlie
added
zetaclient
|
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 introduces outbound fast confirmation support for both EVM and Bitcoin chains. It adds new test cases and modifies existing end-to-end tests to adjust parameters and remove legacy local mining procedures. Additionally, the changes refactor several functions—including updates to method signatures, introduction of new helper methods for BTC withdrawals, and enhancements to outbound vote processing in observer components—to streamline transaction confirmation handling across the system. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant R as E2ERunner
participant G as GatewayZEVM
participant O as Observer
participant N as Node
T->>R: Initiate BTCWithdraw(receiver, amount, options)
R->>G: Call Withdraw API
G-->>R: Return Transaction (tx)
R->>O: Notify Observer with tx details
O->>N: Poll for confirmations
N-->>O: Confirmations reached
O->>O: Evaluate fast confirmation eligibility
O->>G: If eligible, call PostVoteOutbound to cast vote
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
|
ws4charlie
changed the title
…n local mining logic; fix evm outbound tracker reportet; misc
…t-fast-confirmation-outbound-evm-btc
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3619 +/- ##
===========================================
+ Coverage 64.59% 64.85% +0.26%
===========================================
Files 469 469
Lines 32911 32940 +29
===========================================
+ Hits 21259 21364 +105
+ Misses 10687 10607 -80
- Partials 965 969 +4
🚀 New features to boost your workflow:
|
| @@ -68,6 +69,15 @@ func (signer *Signer) reportToOutboundTracker( | |||
| continue | |||
| } | |||
|
|
|||
| // stop if the cctx is already finalized | |||
There was a problem hiding this comment.
I saw a lots unnecessary outbound tracker submissions in E2E tests. I tracked down the root cause:
the outbound ballot finalizes so fast in local network and the tracker reporter is still submitting tracker without checking the CCTX's status. This will not happen on mainnet due to the slow finality.
ws4charlie
changed the title
![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
🔭 Outside diff range comments (1)
e2e/runner/bitcoin.go (1)
392-394: 🛠️ Refactor suggestionError handling in goroutine should be improved.
The error handling in the goroutine calls
require.NoError(r, err)which could potentially panic and terminate the goroutine unexpectedly. In long-running goroutines, it's better to handle errors gracefully.Implement proper error logging instead of using require.NoError in the goroutine:
- _, err := r.GenerateToAddressIfLocalBitcoin(1, r.BTCDeployerAddress) - require.NoError(r, err) + if _, err := r.GenerateToAddressIfLocalBitcoin(1, r.BTCDeployerAddress); err != nil { + r.Logger.Error("Failed to generate blocks in MineBlocksIfLocalBitcoin: %v", err) + time.Sleep(BTCRegnetBlockTime) // Still sleep to avoid tight loop on persistent errors + continue + }This approach prevents the goroutine from terminating on error and provides helpful error logs for debugging.
🧹 Nitpick comments (21)
e2e/runner/bitcoin.go (1)
379-403: MineBlocksIfLocalBitcoin implementation is sound but could be enhanced with context support.The current implementation of continuous block mining is well-structured with a clean stop mechanism. However, it would be more robust to use a context-based cancellation approach rather than a custom stop channel.
Consider implementing context-based cancellation for more idiomatic Go code:
-func (r *E2ERunner) MineBlocksIfLocalBitcoin() func() { +func (r *E2ERunner) MineBlocksIfLocalBitcoin() context.CancelFunc { require.NotNil(r, r.BTCDeployerAddress, "E2ERunner.BTCDeployerAddress is nil") - stopChan := make(chan struct{}) + ctx, cancel := context.WithCancel(r.Ctx) go func() { for { select { - case <-stopChan: + case <-ctx.Done(): return default: _, err := r.GenerateToAddressIfLocalBitcoin(1, r.BTCDeployerAddress) require.NoError(r, err) time.Sleep(BTCRegnetBlockTime) } } }() - return func() { - close(stopChan) - } + return cancel }This approach leverages Go's standard context package for cancellation, which is the idiomatic way to handle goroutine cancellation in Go. It would also require adding
contextto the imports if not already present.zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
74-79: Improve error handling for CCTX retrieval failure.The current implementation logs the error when unable to retrieve the CCTX but continues execution, which could potentially lead to unnecessary outbound tracker submissions if the error is transient. Consider implementing a retry mechanism or adding more contextual information to the error message.
if err != nil { - logger.Err(err).Msg("unable to get cctx for outbound") + logger.Err(err). + Str("chain_id", fmt.Sprintf("%d", chainID)). + Str("nonce", fmt.Sprintf("%d", nonce)). + Msg("unable to get cctx for outbound, will continue with tracker submission") } else if !crosschainkeeper.IsPending(cctx) { logger.Info().Msg("cctx already finalized") return nil }This improved error logging would provide more diagnostic information for troubleshooting while maintaining the existing behavior.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-79: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73-L79
Added lines #L73 - L79 were not covered by teststestutil/sample/crosschain.go (1)
371-390: Well-implemented test utility for outbound vote generation.This function properly creates sample outbound vote messages with randomized data for testing purposes. It mirrors the structure of the existing
InboundVoteFromRandfunction and correctly implements randomization for all necessary fields.Consider adding brief comments to explain the significance of specific fields, particularly for the
Statusfield where specific values are chosen from an enumeration.// OutboundVoteFromRand creates a sample outbound vote message from a random source of randomness func OutboundVoteFromRand(to int64, r *rand.Rand) types.MsgVoteOutbound { + // Randomly select between success and failed status for the outbound transaction status := []chains.ReceiveStatus{chains.ReceiveStatus_success, chains.ReceiveStatus_failed} return types.MsgVoteOutbound{ Creator: Bech32AccAddress().String(), CctxHash: GetCctxIndexFromString("sampleCctxIndex"), ObservedOutboundHash: Hash().String(), ObservedOutboundBlockHeight: uint64(r.Uint32()), ObservedOutboundGasUsed: uint64(r.Uint32()), ObservedOutboundEffectiveGasPrice: sdkmath.NewInt(r.Int63()), ObservedOutboundEffectiveGasLimit: uint64(r.Uint32()), ValueReceived: sdkmath.NewUint(r.Uint64()), + // Randomly select transaction status to simulate different outcome scenarios Status: status[r.Intn(len(status))], OutboundChain: to, OutboundTssNonce: uint64(r.Uint32()), CoinType: CoinTypeFromRand(r), + // Randomize confirmation mode (FAST or SAFE) ConfirmationMode: ConfirmationModeFromRand(r), } }e2e/runner/zevm.go (1)
27-43: Well-structured BTCWithdraw implementation.The new BTCWithdraw method follows the established pattern of other withdrawal methods in the codebase and provides a clean interface for Bitcoin withdrawals through the Gateway. The implementation correctly:
- Accepts a Bitcoin address, amount, and revert options
- Encodes the Bitcoin address appropriately
- Calls the Gateway contract with proper parameters
- Returns the transaction object for further processing
Consider adding a more descriptive comment similar to other withdrawal methods for consistency.
-// BTCWithdraw calls Withdraw of Gateway with gas token on ZEVM +// BTCWithdraw calls Withdraw of Gateway with Bitcoin token on ZEVMe2e/e2etests/test_eth_withdraw_fast_confirmation.go (4)
29-43: Consider dynamic block intervals for fast vs. safe
The hardcodedSafeOutboundCount = 10andFastOutboundCount = 1seem tuned for a local environment. If block times vary or multiple tests run in parallel, consider making them configurable to ensure reliability.
49-67: Include a sanity check on tracker readiness
You're introducing a 1-block wait before retrieving the CCTX. Consider adding an explicit readiness check or shorter block wait for faster tests. Otherwise, a single block might not always be sufficient if block production is delayed.
70-99: Reusing chain parameter toggles
The pattern for disabling fast confirmation mirrors enabling it. Consider extracting the enabling/disabling logic into a helper to reduce duplication and improve clarity.
100-105: Ensure meaningful performance thresholds
Using a fixed threshold of 3 seconds might be risky if the environment is inconsistent. Consider using a margin tied to block production metrics or mocking clock times for more robust comparisons.zetaclient/chains/bitcoin/observer/outbound.go (6)
102-108: Variable initialization
Declaring local variables fromcctx.GetCurrentOutboundParam()clarifies usage. However, if you plan to add more logic around them, consider grouping them into a struct for clarity.
153-155: Add unit test coverage
Lines 153–155 are not covered by tests according to static analysis. Consider including a test scenario whereres.Confirmationsmeets the safe threshold, ensuring this branch is tested.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 153-155: zetaclient/chains/bitcoin/observer/outbound.go#L153-L155
Added lines #L153 - L155 were not covered by tests
170-184: Posting outbound vote
The logic to skip fast confirmation eligibility ifIsOutboundEligibleForFastConfirmation(msg) == falseis reasonable. However, consider logging the reason for ineligibility to aid troubleshooting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 175-176: zetaclient/chains/bitcoin/observer/outbound.go#L175-L176
Added lines #L175 - L176 were not covered by tests
[warning] 180-181: zetaclient/chains/bitcoin/observer/outbound.go#L180-L181
Added lines #L180 - L181 were not covered by tests
175-176: Add unit test coverage
Lines 175–176 related to checking outbound eligibility for fast confirmation aren’t covered by tests. Introducing a targeted test case would ensure that the skip logic is validated.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 175-176: zetaclient/chains/bitcoin/observer/outbound.go#L175-L176
Added lines #L175 - L176 were not covered by tests
180-181: Add unit test coverage
Lines 180–181, which handle errors fromPostVoteOutbound, are untested per static analysis. Include a scenario wherePostVoteOutboundfails to confirm robust error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 180-181: zetaclient/chains/bitcoin/observer/outbound.go#L180-L181
Added lines #L180 - L181 were not covered by tests
186-226: Refactored outbound vote construction
This new function consolidates the data needed for a Bitcoin outbound vote. The usage ofNewMsgVoteOutboundis clear. If you foresee adding additional fields (e.g., metadata for debugging), consider constructing a local struct and converting it intoMsgVoteOutboundto maintain clarity.zetaclient/chains/evm/observer/outbound.go (5)
146-157: Posting the outbound vote
Consider adding structured logs here for better traceability, especially before posting. IfPostVoteOutboundis a frequent failure point, more logs or metrics around this call might help.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-156: zetaclient/chains/evm/observer/outbound.go#L156
Added line #L156 was not covered by tests
156-156: Add unit test coverage
Line 156 is uncovered by tests per static analysis. A test scenario wherePostVoteOutboundfails (or returns an error) would ensure robust coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-156: zetaclient/chains/evm/observer/outbound.go#L156
Added line #L156 was not covered by tests
162-195:newOutboundVotefor EVM
Similar to the Bitcoin observer changes, constructingMsgVoteOutboundin a separate method centralizes logic. This fosters consistency and reduces duplication.
227-229: Test coverage for restricted cctx
These lines handle restricted cross-chain transactions by returning early. Ensure that you have coverage for a scenario where compliance restrictions apply, to validate the logic.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 227-229: zetaclient/chains/evm/observer/outbound.go#L227-L229
Added lines #L227 - L229 were not covered by tests
470-471: Coverage for fast confirmation
These lines enable fast confirmation checks. Please add a test scenario that targets partial confirmation or boundary conditions, ensuring coverage of the transition from unconfirmed to fast-confirmed.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 470-471: zetaclient/chains/evm/observer/outbound.go#L470-L471
Added lines #L470 - L471 were not covered by testszetaclient/chains/bitcoin/observer/outbound_test.go (1)
293-340: Consider standardizing default configuration valuesThe test suite setup provides flexibility through a functional options pattern, which is excellent. However, you might consider extracting the default values into constants for better maintainability.
+// Default values for test configuration +const ( + defaultLastBlock uint64 = 100 +) func newTestSuite(t *testing.T, opts ...func(*testSuiteConfig)) *testSuite { var cfg testSuiteConfig for _, opt := range opts { opt(&cfg) } chain := chains.BitcoinMainnet if cfg.chain != nil { chain = *cfg.chain } params := mocks.MockChainParams(chain.ChainId, 10) if cfg.ConfirmationParams != nil { params.ConfirmationParams = cfg.ConfirmationParams } btcClient := mocks.NewBitcoinClient(t) - btcClient.On("GetBlockCount", mock.Anything).Return(int64(100), nil) + btcClient.On("GetBlockCount", mock.Anything).Return(int64(defaultLastBlock), nil)e2e/utils/zetacore.go (1)
46-62: Function implementation follows best practices but could benefit from enhanced documentationThe
GetCCTXByInboundHashfunction provides a clear and concise way to retrieve a cross-chain transaction by its inbound hash. The implementation correctly handles error checking and assertion on the expected result length.Consider enhancing the function's documentation to provide more details about its behavior, particularly in error cases:
-// GetCCTXByInboundHash gets cctx by inbound hash +// GetCCTXByInboundHash retrieves a cross-chain transaction (CCTX) by its inbound hash. +// It queries the client for the CCTX data associated with the provided inbound hash, +// asserts that exactly one CCTX is returned, and returns a pointer to that CCTX. +// The function will cause a test failure if an error occurs or if no/multiple CCTXs are found.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
changelog.md(1 hunks)cmd/zetae2e/local/bitcoin.go(7 hunks)cmd/zetae2e/local/evm.go(1 hunks)cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(4 hunks)e2e/e2etests/helpers.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_fast_confirmation.go(2 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_and_abort.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_fast_confirmation.go(1 hunks)e2e/e2etests/test_bitcoin_withdraw_invalid_address.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_legacy.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_p2sh.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_p2wsh.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_restricted_address.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_segwit.go(2 hunks)e2e/e2etests/test_bitcoin_withdraw_taproot.go(2 hunks)e2e/e2etests/test_crosschain_swap.go(0 hunks)e2e/e2etests/test_eth_deposit_fast_confirmation.go(2 hunks)e2e/e2etests/test_eth_withdraw_fast_confirmation.go(1 hunks)e2e/e2etests/test_migrate_tss.go(0 hunks)e2e/runner/bitcoin.go(1 hunks)e2e/runner/evm.go(1 hunks)e2e/runner/setup_bitcoin.go(0 hunks)e2e/runner/zevm.go(2 hunks)e2e/utils/zetacore.go(2 hunks)testutil/sample/crosschain.go(1 hunks)x/observer/types/chain_params.go(1 hunks)x/observer/types/chain_params_test.go(1 hunks)zetaclient/chains/base/confirmation.go(1 hunks)zetaclient/chains/base/confirmation_test.go(2 hunks)zetaclient/chains/base/observer.go(2 hunks)zetaclient/chains/base/observer_test.go(2 hunks)zetaclient/chains/bitcoin/observer/outbound.go(4 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(4 hunks)zetaclient/chains/evm/observer/observer_test.go(4 hunks)zetaclient/chains/evm/observer/outbound.go(4 hunks)zetaclient/chains/evm/observer/outbound_test.go(3 hunks)zetaclient/chains/evm/signer/outbound_tracker_reporter.go(2 hunks)
💤 Files with no reviewable changes (9)
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go
- e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go
- e2e/e2etests/test_migrate_tss.go
- e2e/e2etests/test_bitcoin_deposit_and_call_revert.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_and_abort.go
- e2e/e2etests/test_crosschain_swap.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go
- e2e/runner/setup_bitcoin.go
- e2e/e2etests/test_bitcoin_withdraw_invalid_address.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.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.
cmd/zetae2e/local/evm.gox/observer/types/chain_params.goe2e/runner/bitcoin.gozetaclient/chains/base/confirmation.goe2e/e2etests/test_bitcoin_withdraw_p2wsh.goe2e/e2etests/test_bitcoin_withdraw_segwit.goe2e/e2etests/test_bitcoin_withdraw_legacy.goe2e/e2etests/test_bitcoin_withdraw_taproot.goe2e/runner/evm.goe2e/e2etests/test_bitcoin_deposit_fast_confirmation.goe2e/e2etests/test_bitcoin_withdraw_fast_confirmation.gox/observer/types/chain_params_test.goe2e/runner/zevm.gocmd/zetae2e/local/local.goe2e/utils/zetacore.gozetaclient/chains/base/observer_test.goe2e/e2etests/helpers.gozetaclient/chains/evm/signer/outbound_tracker_reporter.goe2e/e2etests/test_bitcoin_withdraw_p2sh.goe2e/e2etests/test_eth_withdraw_fast_confirmation.gozetaclient/chains/evm/observer/observer_test.goe2e/e2etests/test_eth_deposit_fast_confirmation.gozetaclient/chains/base/confirmation_test.goe2e/e2etests/test_bitcoin_withdraw_restricted_address.gocmd/zetae2e/local/bitcoin.gozetaclient/chains/evm/observer/outbound.gozetaclient/chains/evm/observer/outbound_test.gozetaclient/chains/bitcoin/observer/outbound.gotestutil/sample/crosschain.gozetaclient/chains/base/observer.gozetaclient/chains/bitcoin/observer/outbound_test.goe2e/e2etests/e2etests.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/evm/signer/outbound_tracker_reporter.go
[warning] 73-79: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73-L79
Added lines #L73 - L79 were not covered by tests
zetaclient/chains/evm/observer/outbound.go
[warning] 156-156: zetaclient/chains/evm/observer/outbound.go#L156
Added line #L156 was not covered by tests
[warning] 227-229: zetaclient/chains/evm/observer/outbound.go#L227-L229
Added lines #L227 - L229 were not covered by tests
[warning] 470-471: zetaclient/chains/evm/observer/outbound.go#L470-L471
Added lines #L470 - L471 were not covered by tests
zetaclient/chains/bitcoin/observer/outbound.go
[warning] 153-155: zetaclient/chains/bitcoin/observer/outbound.go#L153-L155
Added lines #L153 - L155 were not covered by tests
[warning] 175-176: zetaclient/chains/bitcoin/observer/outbound.go#L175-L176
Added lines #L175 - L176 were not covered by tests
[warning] 180-181: zetaclient/chains/bitcoin/observer/outbound.go#L180-L181
Added lines #L180 - L181 were not covered by tests
🔇 Additional comments (83)
e2e/runner/bitcoin.go (2)
31-31: BTCRegnetBlockTime reduction appears correct for fast confirmation support.The reduction of the block time from 6 seconds to 5 seconds aligns with the PR objective of supporting fast outbound confirmations for Bitcoin chains. This change will allow blocks to be generated more frequently in the local regnet environment during testing.
380-380: Update function documentation to reflect new block time.The function documentation states "mines blocks on the local BTC chain at a rate of 1 blocks every 5 seconds" which now accurately matches the updated BTCRegnetBlockTime constant. This shows good attention to documentation consistency.
zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
11-11: Import added correctly for crosschain keeper.The added import provides access to the necessary functions for checking CCTX status, properly supporting the new finalization check.
e2e/e2etests/helpers.go (1)
25-32: Function signature and implementation improved for Bitcoin withdrawals.The
withdrawBTCZRC20function has been refactored to return an Ethereum transaction type and streamline the implementation. This is a well-executed change that aligns with the PR's goal of supporting fast outbound confirmations.cmd/zetae2e/local/evm.go (1)
23-23: Test case addition for fast confirmation support.Adding the
TestETHWithdrawFastConfirmationNametest case to the happy path testing workflow is appropriate and aligns with the PR objectives to validate fast confirmation for Ethereum withdrawals.x/observer/types/chain_params.go (1)
174-178: Well-implemented outbound fast confirmation check.The
IsOutboundFastConfirmationEnabledmethod follows a consistent pattern with the existing inbound check function. It correctly verifies that fast confirmation is properly configured by ensuring the fast count is both enabled (> 0) and provides an actual benefit (< safe count).This method is an essential component for the fast outbound confirmation feature, allowing the system to determine when transactions are eligible for expedited processing.
e2e/runner/evm.go (1)
196-199: Bitcoin ZRC20 approval helper follows established patterns.The addition of the
ApproveBTCZRC20method is well-designed, maintaining consistency with the existing token approval methods and adhering to the DRY principle by reusing the underlyingapproveZRC20implementation.changelog.md (1)
31-31: Well-documented feature addition.This addition to the changelog clearly documents the new feature for outbound fast confirmation support for both EVM chain and Bitcoin chain. The entry follows the consistent format of the changelog, providing a link to the PR and a concise description.
cmd/zetae2e/local/local.go (1)
305-307: Improved resource management with proper cleanup.The change now correctly captures the return value from
startBitcoinTestsas a cleanup function and ensures it's called when the test completes through the use ofdefer. This follows the resource acquisition is initialization (RAII) pattern, ensuring Bitcoin mining resources are properly released even in error scenarios.x/observer/types/chain_params_test.go (1)
346-361: Comprehensive test coverage for the new fast outbound confirmation functionality.This test thoroughly validates the
IsOutboundFastConfirmationEnabledmethod with proper test cases covering:
- When fast confirmation should be enabled (SafeOutboundCount > FastOutboundCount > 0)
- When fast confirmation should be disabled due to zero FastOutboundCount
- When fast confirmation should be disabled due to equal SafeOutboundCount and FastOutboundCount
The test structure aligns with existing patterns in the file and provides good coverage of the method's behavior.
e2e/e2etests/test_bitcoin_withdraw_p2wsh.go (2)
9-9: Import addition is appropriate for the enhanced transaction status validation.The addition of the
crosschaintypesimport is necessary for accessing theCctxStatus_OutboundMinedconstant used in the transaction verification process.
21-26: Well-structured transaction execution and verification flow.The enhanced implementation properly separates transaction execution from verification, improving test clarity and robustness. The code now:
- Executes the withdrawal and captures the transaction
- Waits for the cross-chain transaction to be mined
- Verifies the transaction status matches the expected outcome
This approach ensures complete end-to-end validation of the Bitcoin withdrawal process.
e2e/e2etests/test_bitcoin_withdraw_legacy.go (2)
9-9: Import addition is appropriate for the enhanced transaction status validation.The addition of the
crosschaintypesimport is necessary for accessing theCctxStatus_OutboundMinedconstant used in the transaction verification process.
22-29: Well-structured test with clear phases and enhanced verification.The implementation follows a clear Arrange-Act-Assert pattern with explicit comments demarcating test phases. The transaction flow is properly structured with:
- Execution of the withdrawal operation
- Waiting for the cross-chain transaction to be mined
- Verification of the expected transaction status
This approach enhances test readability and ensures thorough validation of the withdrawal process.
e2e/e2etests/test_bitcoin_withdraw_taproot.go (2)
9-9: Import addition is appropriate for the enhanced transaction status validation.The addition of the
crosschaintypesimport is necessary for accessing theCctxStatus_OutboundMinedconstant used in the transaction verification process.
21-26: Well-structured transaction execution and verification flow.The enhanced implementation properly separates transaction execution from verification, improving test clarity and robustness. The code now:
- Executes the withdrawal and captures the transaction
- Waits for the cross-chain transaction to be mined
- Verifies the transaction status matches the expected outcome
This approach ensures complete end-to-end validation of the Bitcoin withdrawal process, maintaining consistency with other test implementations.
zetaclient/chains/base/confirmation.go (1)
94-104:✅ Verification successful
Well-implemented fast confirmation eligibility check with clear business logic.
The
IsOutboundEligibleForFastConfirmationmethod is well-designed with:
- Appropriate chain parameter validation first
- Clear business logic for determining eligibility based on transaction status
- Comprehensive comments explaining the reasoning behind excluding non-successful statuses
The method follows the established pattern of other confirmation-related functions in the codebase and properly supports the new fast outbound confirmation feature for EVM/Bitcoin chains.
Consider adding unit tests for this method to validate its behavior under various conditions.
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new IsOutboundEligibleForFastConfirmation method echo "Searching for test files that might contain tests for IsOutboundEligibleForFastConfirmation..." fd "confirmation_test.go" --exec grep -l "IsOutboundEligibleForFastConfirmation" {} \;Length of output: 332
Approval: Fast Confirmation Eligibility Logic Verified
The
IsOutboundEligibleForFastConfirmationmethod is implemented with clear chain parameter validation and concise business logic. The presence of tests in the filezetaclient/chains/base/confirmation_test.goconfirms that the feature is being vetted against various conditions. Please ensure that the test cases comprehensively cover all edge scenarios.zetaclient/chains/evm/observer/observer_test.go (4)
395-399: Clear and structured enhancements to testSuiteConfig.The addition of
LastBlockandConfirmationParamsfields to the testSuiteConfig struct provides necessary configuration options for testing different confirmation scenarios in line with the PR objectives.
413-415: Good null-safety check for ConfirmationParams.The code correctly checks if
cfg.ConfirmationParamsis not nil before assigning it tochainParams.ConfirmationParams, which is a proper defensive programming practice.
424-426: Proper initialization with OperatorAddress.The addition of the OperatorAddress to the keys setup ensures that the test environment correctly simulates the operational context for outbound confirmations.
446-448: Appropriate conditional block for LastBlock configuration.The code correctly checks if
cfg.LastBlock > 0before applying it, maintaining backward compatibility with existing tests while enabling new configuration capabilities.e2e/e2etests/test_bitcoin_withdraw_p2sh.go (2)
9-9: Appropriate crosschain type import.The addition of the crosschaintypes import is necessary for accessing the CctxStatus constants used in transaction verification.
22-27: Enhanced transaction verification flow.This change improves the test by properly capturing the transaction result and verifying its completion status. The implementation now ensures that the Bitcoin P2SH withdrawal completes successfully by:
- Storing the transaction result
- Waiting for the transaction to be mined
- Verifying the final status matches the expected OutboundMined state
This aligns with the PR objective of implementing fast outbound confirmation support.
e2e/e2etests/test_bitcoin_withdraw_segwit.go (2)
9-9: Appropriate crosschain type import.The addition of the crosschaintypes import is necessary for accessing the CctxStatus constants used in transaction verification.
21-26: Enhanced transaction verification flow.This change improves the test by properly capturing the transaction result and verifying its completion status. The implementation now ensures that the Bitcoin SegWit withdrawal completes successfully by following the same pattern as the P2SH implementation:
- Storing the transaction result
- Waiting for the transaction to be mined
- Verifying the final status matches the expected OutboundMined state
The consistent implementation across different Bitcoin withdrawal types is a good practice.
e2e/runner/zevm.go (1)
8-8: Appropriate btcutil import.The addition of the btcutil import is necessary for the new BTCWithdraw method's parameter type.
e2e/e2etests/test_bitcoin_deposit_fast_confirmation.go (4)
34-34: Parameter change improves test efficiencyReducing
SafeInboundCountfrom 6 to 4 decreases the test execution time from approximately 36 seconds to 20 seconds while still maintaining a sufficient interval compared to the fast confirmation time.
36-37: Good practice: Preserving existing parametersUsing values from
resOldChainParamsrather than hardcoded values ensures test isolation and prevents interference with other test cases that may rely on these parameters.
44-44: API consistency improvementReplacing
utils.WaitForZetaBlockswithr.WaitForBlocks(2)standardizes the block waiting mechanism across the codebase.
102-105: Well-structured test teardown with clear documentationThe selective restoration of only inbound parameters with an explanatory comment prevents interference with other tests that may depend on outbound parameters while ensuring proper test cleanup.
zetaclient/chains/base/observer_test.go (6)
5-11: Appropriate imports added for extended functionalityNew imports for error handling, random number generation, and time management support the expanded test functionality.
548-551: Well-structured test setupThe test setup properly initializes the context and creates a random generator with a time-based seed for enhanced test reliability and determinism.
553-561: Comprehensive successful case testThis test case thoroughly validates the happy path for outbound vote posting, properly mocking the ZetacoreClient's expected behavior.
563-574: Proper validation error handling testThe test correctly verifies that invalid message parameters are detected and reported with appropriate error messages before making any external calls.
576-585: RPC failure handling verificationThis test ensures the method propagates errors from the underlying ZetacoreClient correctly, which is crucial for maintaining reliable error reporting throughout the system.
587-596: Already voted case handlingThe test validates the proper handling of the case where a vote has already been cast, ensuring no duplicate votes are submitted and no errors are returned.
zetaclient/chains/base/observer.go (4)
394-396: Code simplification improves readabilityDirect field references from the message object streamline the code by eliminating unnecessary local variables, making it more concise and easier to maintain.
423-436: Well-structured function initialization with appropriate validationThe new
PostVoteOutboundfunction properly initializes gas limits based on message status and performs basic validation to prevent unnecessary RPC calls with invalid data.
437-446: Comprehensive logging fieldsThe logger field setup captures all relevant information for tracing and debugging, including method name, transaction hash, nonce, coin type, confirmation mode, and ballot digest.
447-462: Robust error handling with appropriate log levelsThe function implements proper error propagation and uses different log levels based on the context (error for failures, debug for already voted cases to prevent log pollution, info for successful operations).
e2e/e2etests/test_eth_deposit_fast_confirmation.go (3)
39-40: Preserving system state integrityUsing values from
resOldChainParamsrather than hardcoded values ensures that the test respects the current system configuration and doesn't interfere with other test cases.
47-47: Simplified block waiting implementationThe direct use of
r.WaitForBlocks(2)improves code clarity by eliminating an unnecessary abstraction layer.
106-110: Selective parameter restoration prevents test interferenceRestoring only the inbound confirmation parameters during teardown rather than the entire chain parameters prevents potential conflicts with other tests that may rely on outbound parameters.
e2e/e2etests/test_bitcoin_withdraw_restricted_address.go (4)
6-6: New import added appropriatelyThe import for
chainhashfrom thebtcsuitepackage is correctly added to support converting hash strings to hash objects.
33-44: Improved transaction tracking flowThe implementation now correctly stores the transaction reference and properly retrieves the cross-chain transaction status. This approach is more robust as it follows the actual transaction through the system rather than invoking the withdrawal function multiple times.
40-43: Well-structured transaction hash extractionThe extraction of the Bitcoin transaction hash from the cross-chain transaction and conversion to a proper chainhash object follows best practices.
46-47: Appropriate use of BTC RPC clientThe function properly utilizes the BTC RPC client to retrieve the raw transaction details using the extracted hash.
e2e/e2etests/test_bitcoin_withdraw_fast_confirmation.go (5)
14-15: Well-documented test functionThe test function includes a clear comment describing its purpose, which helps maintainers understand the test's intent.
23-41: Robust chain parameter modificationThe implementation correctly retrieves the existing chain parameters, preserves the necessary values, and updates only the confirmation parameters needed for the test. This approach prevents unintended side effects on other tests.
43-63: Clear test execution and verificationThe test has a clear structure with appropriate comments marking the stages of the test (ACT-1, ASSERT-1). The code properly waits for the outbound tracker submission before measuring confirmation time, ensuring accurate timing measurements.
94-99: Effective time comparison verificationThe time comparison logic correctly asserts that fast confirmation is indeed faster than safe confirmation by a meaningful amount (at least one Bitcoin block time).
100-106: Proper test cleanupThe test properly restores the original chain parameters in its teardown phase, ensuring that subsequent tests aren't affected by this test's changes. The comment about not restoring the complete
resOldChainParamsprovides valuable context for future maintainers.e2e/e2etests/e2etests.go (4)
23-23: Consistent naming conventionThe new constant
TestETHWithdrawFastConfirmationNamefollows the established naming pattern in the codebase.
120-120: Appropriate positioning of new testThe new
TestBitcoinWithdrawFastConfirmationNameconstant is correctly placed within the Bitcoin tests section, maintaining the code organization.
305-313: Complete test registrationThe Ethereum withdrawal fast confirmation test is properly registered with appropriate descriptions and defaults. The minimum version requirement ensures it only runs on compatible versions.
906-914: Consistent test registration patternThe Bitcoin withdrawal fast confirmation test registration follows the same pattern as other tests, providing consistent user experience.
zetaclient/chains/base/confirmation_test.go (5)
6-8: Appropriate import additionsThe additions of
math/randandtimeimports are necessary for the new test function that uses randomization with appropriate seeding.
16-16: Import for sample packageThe
samplepackage import is correctly added to support generating test data for the new test cases.
419-474: Well-structured and comprehensive test casesThe test function
Test_IsOutboundEligibleForFastConfirmationis well-designed with:
- Appropriate test cases covering both positive and negative scenarios
- Clear test case descriptions
- Good separation of ARRANGE, ACT, ASSERT phases
- Use of random data generation for increased test coverage
The test appropriately verifies the conditions under which outbound transactions are eligible for fast confirmation.
420-421: Proper random number generationThe random number generator is correctly seeded with the current time to ensure different test data on each run, while still being deterministic within a single test run.
464-465: Effective use of test utilitiesThe test properly uses the
sample.OutboundVoteFromRandutility to generate realistic test data, and correctly modifies the status field to test the specific scenario.e2e/e2etests/test_eth_withdraw_fast_confirmation.go (2)
16-19: Validate argument length and usage
This check ensures the test is invoked with the correct number of arguments. The approach is clean.
106-113: Restore chain parameters
The final code properly restores chain params, helping maintain test isolation. This is crucial if subsequent tests rely on the original settings.zetaclient/chains/bitcoin/observer/outbound.go (2)
7-7: Math/big import
The addition of"math/big"is appropriate for handling large integer calculations in confirmations and amounts.
142-155: Dual confirmation mode check
The logic here neatly handles both safe and fast confirmations. Maintain caution about partial confirmations (e.g., ifres.Confirmationschanges mid-flow). Ensure the difference betweenOutboundConfirmationSafeandOutboundConfirmationFastis validated to avoid ambiguous conditions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 153-155: zetaclient/chains/bitcoin/observer/outbound.go#L153-L155
Added lines #L153 - L155 were not covered by testszetaclient/chains/evm/observer/outbound.go (2)
119-126: Local variable definitions
Good practice extracting necessary fields (chainID,txHash, etc.) from the context. Improves readability and ensures you only pass around relevant data.
143-144: Graceful error handling
Returning the parse error ensures the function handles outbound event failures cleanly. This helps debugging if unexpected transaction logs occur.zetaclient/chains/bitcoin/observer/outbound_test.go (5)
26-40: Well-structured test setup with clear scenariosThe test function is organized with a comprehensive setup for validating outbound confirmation voting behavior. The confirmParams structure initialization is clearly defined with appropriate threshold values.
36-75: Comprehensive test cases with clear expectationsThe table-driven test approach provides excellent coverage of different scenarios, including both SAFE and FAST confirmation paths. Each test case has descriptive naming and clear expected outcomes.
97-139: Well-implemented test execution flowThe execution of each test case follows a clear arrangement-action-assertion pattern, providing good readability and maintainability. Mock setup is appropriately scoped to each test case.
150-151: Updated to use new test suite pattern consistentlyThe existing test functions have been updated to use the new test suite setup, which promotes consistency across the test file.
Also applies to: 232-233
342-348: Well-designed test suite structureThe test suite structure offers a clean composition of dependencies, making it easy to access and manipulate test components as needed.
cmd/zetae2e/local/bitcoin.go (7)
24-25: Function signature improved to support cleanup functionalityThe function now returns a cleanup function, which is a good practice for proper resource management.
42-43: Appropriate test organization for new functionalityThe test organization has been updated to include fast confirmation tests in the appropriate test groups, keeping the test structure clean.
Also applies to: 52-54
67-79: Proper implementation of mining controlThe function now returns a
stopMiningfunction and passes it to the caller, enabling proper control of the mining process during tests.
90-91: Enhanced function signature to support cleanupThe function signature has been updated to return an additional function for stopping mining, improving resource management.
119-122: Centralized mining control for consistent test environmentThis change ensures all E2E tests run with a consistent block time, which is crucial for deterministic testing. This is a significant improvement that aligns with the PR objectives.
126-133: Simplified initial fund allocationThe updated code more clearly handles sending BTC block rewards to the deployer address as initial funds, improving code readability.
145-146: Updated return statement for consistent interfaceThe function return statement now includes the new
stopMiningfunction, ensuring consistency with the updated function signature.zetaclient/chains/evm/observer/outbound_test.go (4)
21-21: Test function renamed to better reflect its purposeThe function name has been updated from
Test_IsOutboundProcessedtoTest_VoteOutboundIfConfirmed, which better reflects its focus on voting behavior based on confirmation status.
46-61: Updated test case to match new function behaviorThe test description and expectations have been adjusted to accurately reflect that the function should post a vote and return false if outbound is processed, which aligns with the PR objectives.
77-92: Added test case for fast confirmation eligibilityThis new test case specifically verifies the behavior when an outbound transaction is not eligible for fast confirmation, providing good coverage for the new functionality.
95-95: Test function renamed for consistencyThe function name update maintains consistency with the other renamed test functions, ensuring a cohesive test suite.
|
Let's keep it out of the release for now |
There was a problem hiding this comment.
Let's chat about the importance of this feature.
I'm not sure how it would benefit users in general, but I can see it being useful in a few rare situations where we need to perform something in ZEVM after confirming an outbound.
Let me know what you think. cc @lumtis
| client crosschaintypes.QueryClient, | ||
| chainID int64, | ||
| nonce uint64, | ||
| hashCount int, |
There was a problem hiding this comment.
why do we need this hashCount param?
There was a problem hiding this comment.
hashCount > 1 can happen on outbound replacement (gas price increase), so I wanted to make it general.
Exactly, this has benefits once you have logic to handle response from the cctx. There will be benefits once we integrate #2966, user will have funds refunded faster. Although this feature is mostly benefitial for crosschain call that doesn't concern fast observation for now. In general the benefits will be minimal in all cases. If this PR is not too complex let's still move forward and merge it, you can disable the fast move by setting the same value as safe. |
…t-fast-confirmation-outbound-evm-btc
Description
The main contents of this PR:
IsOutboundEligibleForFastConfirmationto determine if an outbound can be fast confirmed.FAST | SAFEto messageMsgVoteOutbound.MineBlocksIfLocalBitcoinout from individual E2E tests, so that all tests will be running on a constantRegnetblock time.PostVoteOutboundto base observer to simply and unify outbound vote logic for both EVM and Bitcoin chain.FASTETH withdraw andFASTBTC withdraw.How Has This Been Tested?
Summary by CodeRabbit
New Features
Performance Improvements