test: split bitcoin e2e into deposit group and withdraw group #3105
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements and modifications to the codebase, particularly focusing on Bitcoin-related functionalities and testing. Key changes include the addition of new features such as the whitelisting of SPL tokens on Solana and improvements in build reproducibility. The Bitcoin end-to-end tests have been split into two runners for better performance. Additionally, various configuration files have been updated to reflect changes in account definitions. The overall structure and error handling in several methods have also been refined for clarity and maintainability. Changes
Assessment against linked issues
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3105 +/- ##
========================================
Coverage 63.30% 63.30%
========================================
Files 422 422
Lines 29954 29954
========================================
Hits 18963 18963
Misses 10150 10150
Partials 841 841
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (26)
cmd/zetae2e/setup_bitcoin.go (1)
61-61: Document the behavior of SetupBitcoinAccounts.Since this method has been changed from handling a single account to multiple accounts, it would be beneficial to add documentation explaining the new behavior and its relationship to the split test groups (deposit and withdraw).
Add a comment above the method call explaining:
- The purpose of setting up multiple Bitcoin accounts
- How these accounts map to deposit and withdraw test groups
- The significance of the boolean parameter
e2e/runner/run.go (1)
Line range hint
6-13: Consider adding support for concurrent test execution.Given the PR's objective to split Bitcoin E2E tests for better performance, consider enhancing
RunE2ETeststo support concurrent execution of independent test groups.Here's a suggested implementation:
// RunE2ETests runs a list of e2e tests func (r *E2ERunner) RunE2ETests(e2eTests []E2ETest) (err error) { + errChan := make(chan error, len(e2eTests)) + var running int + maxConcurrent := runtime.GOMAXPROCS(0) // Or configurable value + for _, e2eTest := range e2eTests { - if err := r.RunE2ETest(e2eTest, true); err != nil { - return err + if running >= maxConcurrent { + if err := <-errChan; err != nil { + return err + } + running-- } + running++ + go func(test E2ETest) { + errChan <- r.RunE2ETest(test, true) + }(e2eTest) } + + // Wait for remaining tests + for i := 0; i < running; i++ { + if err := <-errChan; err != nil { + return err + } + } return nil }e2e/runner/require.go (1)
Line range hint
28-45: Consider refactoring for improved maintainability and error handling.The current implementation has repetitive token balance checks that could be simplified. Additionally, the external calls could benefit from context timeout handling.
Consider this more maintainable approach:
func (r *E2ERunner) EnsureZeroBalanceOnRestrictedAddressZEVM() { + ctx, cancel := context.WithTimeout(r.Ctx, 30*time.Second) + defer cancel() + restrictedAddress := ethcommon.HexToAddress(sample.RestrictedEVMAddressTest) // ensure ZETA balance is zero - balance, err := r.WZeta.BalanceOf(&bind.CallOpts{}, restrictedAddress) + balance, err := r.WZeta.BalanceOf(&bind.CallOpts{Context: ctx}, restrictedAddress) require.NoError(r, err) require.Zero(r, balance.Cmp(big.NewInt(0)), "the wZETA balance of the address should be zero") - // ensure ZRC20 ETH balance is zero - ensureZRC20ZeroBalance(r, r.ETHZRC20, restrictedAddress) - - // ensure ZRC20 ERC20 balance is zero - ensureZRC20ZeroBalance(r, r.ERC20ZRC20, restrictedAddress) - - // ensure ZRC20 BTC balance is zero - ensureZRC20ZeroBalance(r, r.BTCZRC20, restrictedAddress) - - // ensure ZRC20 SOL balance is zero - ensureZRC20ZeroBalance(r, r.SOLZRC20, restrictedAddress) + // Check all ZRC20 token balances + zrc20Tokens := map[string]*zrc20.ZRC20{ + "ETH": r.ETHZRC20, + "ERC20": r.ERC20ZRC20, + "BTC": r.BTCZRC20, + "SOL": r.SOLZRC20, + } + + for tokenName, token := range zrc20Tokens { + ensureZRC20ZeroBalance(r, token, restrictedAddress, ctx) + } }And update the helper function:
-func ensureZRC20ZeroBalance(r *E2ERunner, zrc20 *zrc20.ZRC20, address ethcommon.Address) { - balance, err := zrc20.BalanceOf(&bind.CallOpts{}, address) +func ensureZRC20ZeroBalance(r *E2ERunner, zrc20 *zrc20.ZRC20, address ethcommon.Address, ctx context.Context) { + balance, err := zrc20.BalanceOf(&bind.CallOpts{Context: ctx}, address) require.NoError(r, err)e2e/runner/setup_bitcoin.go (2)
29-30: Enhance method documentation with parameter details.The method documentation should explain the purpose of the
createWalletparameter and any side effects of the method.-// SetupBitcoinAccounts sets up the TSS account and deployer account +// SetupBitcoinAccounts sets up the TSS account and deployer account in the Bitcoin node. +// If createWallet is true, it creates a new wallet and imports the deployer's private key. func (r *E2ERunner) SetupBitcoinAccounts(createWallet bool) {
72-89: Consider extracting error message as a constant and improving error handling.The current implementation has two potential improvements:
- Extract the error message as a constant to avoid magic strings
- Consider using a more robust way to handle existing wallet errors
+const ( + errDatabaseExists = "Database already exists" +) func (r *E2ERunner) SetupBtcAddress(name string, setupWallet bool) { // ... if err != nil { - require.ErrorContains(r, err, "Database already exists") + require.ErrorContains(r, err, errDatabaseExists) } // ... }cmd/zetae2e/config/localnet.yml (1)
23-26: LGTM: New account properly configured for test separation.The
user_bitcoin2account is correctly structured with all required fields. Consider adding a comment to indicate which account is for deposits and which is for withdrawals to improve maintainability.Add documentation above each Bitcoin account:
+ # Account for Bitcoin deposit tests user_bitcoin1: ... + # Account for Bitcoin withdrawal tests user_bitcoin2:🧰 Tools
🪛 Gitleaks
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
e2e/runner/accounting.go (2)
36-49: Add method documentation and consider structural improvements.The method would benefit from:
- Documentation explaining the purpose and importance of each balance check
- Structured separation between different chain validations
Consider adding documentation and restructuring like this:
+// CheckZRC20BalanceAndSupply verifies that the TSS balances across different chains +// (ETH, ERC20, ZETA, BTC) are greater than or equal to their respective ZRC20 total supplies. +// This ensures there is sufficient collateral locked in the TSS accounts to back the ZRC20 tokens. func (r *E2ERunner) CheckZRC20BalanceAndSupply() { r.Logger.Info("Checking ZRC20 Balance vs. Supply") + + // Ethereum chain validations err := r.checkEthTSSBalance() require.NoError(r, err, "ETH balance check failed") + // ERC20 token validations err = r.checkERC20TSSBalance() require.NoError(r, err, "ERC20 balance check failed") + // ZETA chain validations err = r.checkZetaTSSBalance() require.NoError(r, err, "ZETA balance check failed") + // Bitcoin chain validations err = r.CheckBtcTSSBalance() require.NoError(r, err, "BTC balance check failed") }
Line range hint
109-149: Improve Bitcoin balance handling and consistency.Several improvements are needed in the Bitcoin balance check implementation:
- Using
float64for financial calculations can lead to precision issues- The magic number
10000000should be a named constant- Error message format differs from other check methods
Consider these improvements:
+const ( + // BTCInitialPoolAmount represents the amount minted to initialize the pool + BTCInitialPoolAmount = 10000000 +) func (r *E2ERunner) CheckBtcTSSBalance() error { allTssAddress, err := r.ObserverClient.TssHistory(r.Ctx, &observertypes.QueryTssHistoryRequest{}) if err != nil { return err } - tssTotalBalance := float64(0) + tssTotalBalance := big.NewInt(0) for _, tssAddress := range allTssAddress.TssList { btcTssAddress, err := zetacrypto.GetTssAddrBTC(tssAddress.TssPubkey, r.BitcoinParams) if err != nil { continue } utxos, err := r.BtcRPCClient.ListUnspent() if err != nil { continue } for _, utxo := range utxos { if utxo.Address == btcTssAddress { - tssTotalBalance += utxo.Amount + satoshis := big.NewInt(int64(utxo.Amount * 1e8)) + tssTotalBalance.Add(tssTotalBalance, satoshis) } } } zrc20Supply, err := r.BTCZRC20.TotalSupply(&bind.CallOpts{}) if err != nil { return err } + // Subtract initial pool amount from total supply + adjustedSupply := new(big.Int).Sub(zrc20Supply, big.NewInt(BTCInitialPoolAmount)) + - if int64(tssTotalBalance*1e8) < (zrc20Supply.Int64() - 10000000) { + if tssTotalBalance.Cmp(adjustedSupply) < 0 { return fmt.Errorf( - "BTC: TSS Balance (%d) < ZRC20 TotalSupply (%d)", - int64(tssTotalBalance*1e8), - zrc20Supply.Int64()-10000000, + "BTC: TSS balance (%s) < ZRC20 TotalSupply (%s)", + tssTotalBalance.String(), + adjustedSupply.String(), ) } r.Logger.Info( - "BTC: Balance (%d) >= ZRC20 TotalSupply (%d)", - int64(tssTotalBalance*1e8), - zrc20Supply.Int64()-10000000, + "BTC: Balance (%s) >= ZRC20 TotalSupply (%s)", + tssTotalBalance.String(), + adjustedSupply.String(), ) return nil }e2e/runner/logger.go (3)
18-18: Consider making the padding configurable.Currently, the padding is hardcoded as a constant. Given that this has been modified to support longer prefixes, consider making it configurable through the
NewLoggerconstructor for better flexibility.type Logger struct { verbose bool logger *color.Color prefix string + padding int mu sync.Mutex } -const ( - loggerSeparator = " | " - padding = 12 -) -func NewLogger(verbose bool, printColor color.Attribute, prefix string) *Logger { +func NewLogger(verbose bool, printColor color.Attribute, prefix string, padding int) *Logger { // trim prefix to padding if len(prefix) > padding { prefix = prefix[:padding] } return &Logger{ verbose: verbose, logger: color.New(printColor), prefix: prefix, + padding: padding, } } func (l *Logger) getPrefixWithPadding() string { prefix := l.prefix - for i := len(l.prefix); i < padding; i++ { + for i := len(l.prefix); i < l.padding; i++ { prefix += " " } return prefix }Also applies to: 213-220
Line range hint
213-220: Consider using strings.Repeat for padding.The current implementation uses a loop to add padding spaces. Using
strings.Repeatwould be more idiomatic and potentially more efficient.func (l *Logger) getPrefixWithPadding() string { - prefix := l.prefix - for i := len(l.prefix); i < padding; i++ { - prefix += " " - } - return prefix + return l.prefix + strings.Repeat(" ", padding-len(l.prefix)) }
Line range hint
1-220: Consider adding structured logging support.The current logger implementation uses plain text formatting. For better log analysis and filtering in production environments, consider adding support for structured logging using a library like
zaporlogrus.contrib/localnet/orchestrator/start-zetae2e.sh (1)
101-105: LGTM! The changes align with the PR objectives.The split of Bitcoin tester accounts into two separate accounts (
user_bitcoin1anduser_bitcoin2) properly supports the infrastructure needed for running deposit and withdrawal tests independently.Consider defining the funding amount as a constant at the top of the script for better maintainability:
#!/bin/bash +# Default funding amount for test accounts +DEFAULT_TEST_ACCOUNT_FUNDING=10000 # shellcheck disable=SC2317Then update the funding calls:
-fund_eth_from_config '.additional_accounts.user_bitcoin1.evm_address' 10000 "bitcoin tester1" +fund_eth_from_config '.additional_accounts.user_bitcoin1.evm_address' $DEFAULT_TEST_ACCOUNT_FUNDING "bitcoin tester1"x/observer/types/chain_params.go (1)
Line range hint
1-400: Consider architectural improvements for chain parametersWhile reviewing the changes, I noticed some opportunities for improvement in the codebase:
- The TODO comment about removing GetDefaultChainParams should be addressed
- Consider documenting the significance of parameter values and their impact on chain behavior
- Magic numbers in validation ranges could be defined as constants
Example improvement for validation ranges:
const ( MaxOutboundScheduleInterval = 100 MaxOutboundScheduleLookahead = 500 MaxTickerInterval = 300 )e2e/runner/bitcoin.go (5)
119-121: Documentation enhancement needed for receiver parameterWhile the function documentation has been improved, it should also document the
receiverparameter and its purpose.// DepositBTC deposits BTC from the Bitcoin node wallet into ZetaChain. // Note: This function only works for node wallet based deployer account. +// Parameters: +// - receiver: The Ethereum address that will receive the deposited BTC on ZetaChain
Line range hint
135-139: Consider extracting UTXO validation logicThe UTXO validation logic appears in multiple places. Consider extracting it into a helper method for better maintainability.
+func (r *E2ERunner) validateUTXOs(utxos []btcjson.ListUnspentResult) (float64, int, error) { + spendableAmount := 0.0 + spendableUTXOs := 0 + for _, utxo := range utxos { + if utxo.Spendable { + spendableAmount += utxo.Amount + spendableUTXOs++ + } + } + return spendableAmount, spendableUTXOs, nil +}
150-152: Consider adding validation for minimum deposit amountThe hardcoded amount of 1.15 BTC plus fee should be defined as a constant and validated against network minimums.
+const ( + MinimumDepositAmount = 1.15 +) + +func (r *E2ERunner) validateDepositAmount(amount float64) error { + if amount < MinimumDepositAmount { + return fmt.Errorf("deposit amount %f is below minimum required amount %f", amount, MinimumDepositAmount) + } + return nil +}
157-158: Add documentation for TSS donation purposeThe comment explains the purpose of the donation but should be formatted as a proper documentation comment for better visibility in generated documentation.
+// sendTSSDonation sends a donation to the TSS address to: +// 1. Compensate for funds minted automatically during pool creation +// 2. Prevent accounting errors +// 3. Provide gas fees for TSS to send BTC to other addresses
435-435: Consider making mining interval configurableThe sleep duration is hardcoded. Consider making it configurable to allow for different test environments and requirements.
+// DefaultMiningInterval is the default interval between mining blocks +const DefaultMiningInterval = 6 * time.Second + +// MiningConfig holds configuration for mining behavior +type MiningConfig struct { + Interval time.Duration +} + func (r *E2ERunner) MineBlocksIfLocalBitcoin() func() { + interval := DefaultMiningInterval + if r.Config.Mining.Interval > 0 { + interval = r.Config.Mining.Interval + } ... - time.Sleep(6 * time.Second) + time.Sleep(interval)contrib/localnet/scripts/start-zetacored.sh (3)
Line range hint
16-58: Enhance error handling and add safety measures inadd_v17_message_authorizations.The function should include error handling and backup mechanisms for safer genesis file manipulation:
add_v17_message_authorizations() { # Path to the JSON file json_file="/root/.zetacored/config/genesis.json" + + # Validate file existence + if [[ ! -f "$json_file" ]]; then + echo "Error: Genesis file not found at $json_file" + return 1 + fi + + # Create backup + cp "$json_file" "${json_file}.bak" # Using jq to parse JSON, create new entries, and append them to the authorization array - jq ' + if ! jq ' # Store the nodeAccountList array .app_state.observer.nodeAccountList as $list | # ... (existing jq operations) - ' $json_file > temp.json && mv temp.json $json_file + ' "$json_file" > temp.json; then + echo "Error: Failed to modify genesis file" + mv "${json_file}.bak" "$json_file" + return 1 + fi + + mv temp.json "$json_file" + rm "${json_file}.bak" }
245-250: Refactor bitcoin tester accounts setup for maintainability.Consider using a variable for the common balance amount and adding address validation:
+# Define common balance amount +GENESIS_ACCOUNT_BALANCE="100000000000000000000000000azeta" + # bitcoin tester1 address=$(yq -r '.additional_accounts.user_bitcoin1.bech32_address' /root/config.yml) +if [[ ! "$address" =~ ^zeta[0-9a-z]{39}$ ]]; then + echo "Error: Invalid bitcoin tester1 address format" + exit 1 +fi -zetacored add-genesis-account "$address" 100000000000000000000000000azeta +zetacored add-genesis-account "$address" "$GENESIS_ACCOUNT_BALANCE" # bitcoin tester2 address=$(yq -r '.additional_accounts.user_bitcoin2.bech32_address' /root/config.yml) +if [[ ! "$address" =~ ^zeta[0-9a-z]{39}$ ]]; then + echo "Error: Invalid bitcoin tester2 address format" + exit 1 +fi -zetacored add-genesis-account "$address" 100000000000000000000000000azeta +zetacored add-genesis-account "$address" "$GENESIS_ACCOUNT_BALANCE"
Line range hint
1-11: Enhance script security and reliability.The script requires several security improvements:
- SSH daemon startup:
-/usr/sbin/sshd +if ! /usr/sbin/sshd -t; then + echo "Error: Invalid sshd configuration" + exit 1 +fi +/usr/sbin/sshd || { echo "Error: Failed to start sshd"; exit 1; }
- Add script safety flags:
#!/bin/bash +set -euo pipefail +trap 'echo "Error: Script failed on line $LINENO"' ERR
- Add proper documentation:
+# Security note: This script is intended for local development only. +# It should not be used in production environments as it contains +# development-specific configurations and security trade-offs.cmd/zetae2e/local/local.go (2)
Line range hint
304-316: Consider grouping related Bitcoin deposit test cases.The Bitcoin deposit tests could be organized into subgroups based on their functionality:
- Standard deposit tests
- Deposit-and-call tests
- Standard memo tests
- Cross-chain tests
Consider reorganizing the tests array as follows:
bitcoinDepositTests := []string{ + // Standard deposits e2etests.TestBitcoinDonationName, e2etests.TestBitcoinDepositName, + // Deposit and call e2etests.TestBitcoinDepositAndCallName, e2etests.TestBitcoinDepositAndCallRevertName, + // Standard memo deposits e2etests.TestBitcoinStdMemoDepositName, e2etests.TestBitcoinStdMemoDepositAndCallName, e2etests.TestBitcoinStdMemoDepositAndCallRevertName, e2etests.TestBitcoinStdMemoDepositAndCallRevertOtherAddressName, e2etests.TestBitcoinStdMemoInscribedDepositAndCallName, + // Cross-chain e2etests.TestCrosschainSwapName, }
317-329: Consider grouping Bitcoin withdraw test cases by address type.The Bitcoin withdraw tests could be organized based on address types and test scenarios:
- Basic withdraw tests
- Advanced address type tests
- Multiple/Restricted tests
Consider reorganizing the tests as follows:
bitcoinWithdrawTests := []string{ + // Basic withdraw tests e2etests.TestBitcoinWithdrawSegWitName, e2etests.TestBitcoinWithdrawInvalidAddressName, e2etests.TestZetaWithdrawBTCRevertName, } bitcoinWithdrawTestsAdvanced := []string{ + // Advanced address types e2etests.TestBitcoinWithdrawTaprootName, e2etests.TestBitcoinWithdrawLegacyName, e2etests.TestBitcoinWithdrawP2SHName, e2etests.TestBitcoinWithdrawP2WSHName, + // Multiple and restricted e2etests.TestBitcoinWithdrawMultipleName, e2etests.TestBitcoinWithdrawRestrictedName, }changelog.md (2)
10-10: Remove empty line.The empty line disrupts the changelog structure and should be removed to maintain consistency.
-
12-12: Fix PR link formatting.The PR link format is inconsistent with other entries in the changelog. Each entry should follow the format:
[PR-NUMBER](URL) - description.-* [3105](https://github.com/zeta-chain/node/pull/3105) - split Bitcoin E2E tests into two runners +* [#3105](https://github.com/zeta-chain/node/pull/3105) - Split Bitcoin E2E tests into two runnerscmd/zetae2e/local/bitcoin.go (1)
132-148: Consider error context increateTestRoutine.When wrapping errors, providing context enhances debuggability. Ensure the error messages are descriptive and informative.
Adjust the error handling:
- return fmt.Errorf("bitcoin tests failed: %v", err) + return fmt.Errorf("bitcoin tests failed for runner %s: %w", r.Name, err)This change provides clearer information about which runner encountered an error.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (38)
changelog.md(1 hunks)cmd/zetae2e/bitcoin_address.go(1 hunks)cmd/zetae2e/config/local.yml(1 hunks)cmd/zetae2e/config/localnet.yml(1 hunks)cmd/zetae2e/local/bitcoin.go(1 hunks)cmd/zetae2e/local/local.go(5 hunks)cmd/zetae2e/setup_bitcoin.go(1 hunks)contrib/localnet/orchestrator/start-zetae2e.sh(1 hunks)contrib/localnet/scripts/start-zetacored.sh(1 hunks)e2e/config/config.go(3 hunks)e2e/e2etests/test_bitcoin_deposit.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_call.go(0 hunks)e2e/e2etests/test_bitcoin_donation.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go(0 hunks)e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_invalid_address.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_legacy.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_multiple.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_p2sh.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_p2wsh.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_restricted_address.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_segwit.go(0 hunks)e2e/e2etests/test_bitcoin_withdraw_taproot.go(0 hunks)e2e/e2etests/test_migrate_tss.go(0 hunks)e2e/e2etests/test_stress_btc_deposit.go(0 hunks)e2e/e2etests/test_stress_btc_withdraw.go(0 hunks)e2e/runner/accounting.go(1 hunks)e2e/runner/balances.go(1 hunks)e2e/runner/bitcoin.go(7 hunks)e2e/runner/logger.go(1 hunks)e2e/runner/require.go(1 hunks)e2e/runner/run.go(1 hunks)e2e/runner/setup_bitcoin.go(1 hunks)x/observer/types/chain_params.go(1 hunks)
💤 Files with no reviewable changes (20)
- e2e/e2etests/test_bitcoin_deposit.go
- e2e/e2etests/test_bitcoin_deposit_and_call_revert.go
- e2e/e2etests/test_bitcoin_deposit_call.go
- e2e/e2etests/test_bitcoin_donation.go
- e2e/e2etests/test_bitcoin_std_deposit.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go
- e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go
- e2e/e2etests/test_bitcoin_withdraw_invalid_address.go
- e2e/e2etests/test_bitcoin_withdraw_legacy.go
- e2e/e2etests/test_bitcoin_withdraw_multiple.go
- e2e/e2etests/test_bitcoin_withdraw_p2sh.go
- e2e/e2etests/test_bitcoin_withdraw_p2wsh.go
- e2e/e2etests/test_bitcoin_withdraw_restricted_address.go
- e2e/e2etests/test_bitcoin_withdraw_segwit.go
- e2e/e2etests/test_bitcoin_withdraw_taproot.go
- e2e/e2etests/test_migrate_tss.go
- e2e/e2etests/test_stress_btc_deposit.go
- e2e/e2etests/test_stress_btc_withdraw.go
🧰 Additional context used
📓 Path-based instructions (15)
cmd/zetae2e/bitcoin_address.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/setup_bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/scripts/start-zetacored.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
e2e/config/config.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/accounting.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/balances.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/logger.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/require.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/run.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/setup_bitcoin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/chain_params.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 Gitleaks
cmd/zetae2e/config/local.yml
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
cmd/zetae2e/config/localnet.yml
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (20)
cmd/zetae2e/bitcoin_address.go (1)
71-71: LGTM! Improved logging format.
The use of proper encoding methods (EncodeAddress() and String()) for logging Bitcoin addresses and private keys is a good practice. This ensures consistent and correct string representation of these values.
Also applies to: 73-73
e2e/runner/run.go (2)
29-29: LGTM: Improved comment clarity.
The updated comment provides better context about the specific check being performed.
31-31: Verify error handling in CheckZRC20BalanceAndSupply.
The removal of error handling at this level requires verification that errors are properly caught and reported in the new implementation.
✅ Verification successful
The initial results show that CheckZRC20BalanceAndSupply uses require.NoError for assertions, but we need to verify the implementation of the individual check functions and their error handling patterns to ensure comprehensive verification.
Error handling has been properly implemented through assertions
The verification shows that error handling has been effectively migrated to use assertions:
- Each individual check function (
checkEthTSSBalance,checkERC20TSSBalance,checkZetaTSSBalance) properly returns errors - The main
CheckZRC20BalanceAndSupplyfunction usesrequire.NoErrorto assert on these errors with descriptive messages - This pattern ensures errors are caught and reported with clear context for debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of CheckZRC20BalanceAndSupply
# to ensure proper error handling through assertions
# Search for the implementation and its test coverage
echo "Searching for CheckZRC20BalanceAndSupply implementation:"
rg -A 10 "func.*CheckZRC20BalanceAndSupply"
echo "\nSearching for test coverage:"
rg -A 5 "Test.*CheckZRC20BalanceAndSupply"
Length of output: 921
Script:
#!/bin/bash
# Let's check the implementation of the individual balance check functions
# and their error handling mechanisms
echo "Checking individual balance check implementations:"
rg -A 15 "func.*(checkEthTSSBalance|checkERC20TSSBalance|checkZetaTSSBalance)"
echo "\nChecking require package usage and assertions:"
rg "require\." e2e/runner/accounting.go
Length of output: 3369
e2e/runner/require.go (1)
27-28: LGTM! Clear and descriptive method rename.
The rename from EnsureZeroBalanceAddressZEVM to EnsureZeroBalanceOnRestrictedAddressZEVM improves clarity by explicitly indicating the purpose of checking a restricted address.
e2e/runner/setup_bitcoin.go (1)
37-48: LGTM! Clean implementation with good error handling and logging.
The code properly sets up both the deployer and TSS addresses with appropriate error handling and informative logging.
cmd/zetae2e/config/localnet.yml (2)
19-26: Consider RPC configuration for concurrent test execution.
While the account split enables parallel test execution, verify that the Bitcoin RPC configuration (particularly at bitcoin:18443) can handle concurrent connections from separate test groups.
Let's check for any RPC connection handling configurations:
#!/bin/bash
# Search for Bitcoin RPC configuration patterns across the codebase
rg -A 5 "bitcoin.*18443|bitcoin.*rpc" --type go🧰 Tools
🪛 Gitleaks
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
19-22: LGTM: Account renaming aligns with test split objectives.
The renaming of user_bitcoin to user_bitcoin1 maintains consistent structure with other accounts while supporting the separation of Bitcoin E2E tests.
Let's verify the account renaming is consistent across related files:
✅ Verification successful
Let me gather more specific information about the account renaming.
Account renaming verification successful
The renaming from user_bitcoin to user_bitcoin1 has been properly implemented across the codebase:
- Consistent naming in configuration files (
local.ymlandlocalnet.yml) - Proper struct field definitions in
e2e/config/config.go - No lingering references to the old
user_bitcoinname
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old account name
rg "user_bitcoin[^12]" --type yaml
Length of output: 6257
Script:
#!/bin/bash
# Search for user_bitcoin references in test files
rg "user_bitcoin" --type go --type yaml
Length of output: 371
🧰 Tools
🪛 Gitleaks
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
cmd/zetae2e/config/local.yml (3)
19-22: LGTM: Account renaming aligns with test splitting strategy.
The renaming of user_bitcoin to user_bitcoin1 maintains configuration consistency while supporting the separation of Bitcoin E2E tests.
🧰 Tools
🪛 Gitleaks
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
23-26: LGTM: New Bitcoin account enables parallel test execution.
The addition of user_bitcoin2 with complete configuration (bech32, EVM address, and private key) facilitates concurrent execution of deposit and withdrawal test groups, addressing the CI timeout issues mentioned in #3080.
🧰 Tools
🪛 Gitleaks
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
19-26: Verify test configuration updates.
Please ensure that the E2E test configurations have been updated to reference these accounts appropriately.
✅ Verification successful
Configuration appears to be properly structured and referenced
The verification shows that:
- The Bitcoin accounts (
user_bitcoin1anduser_bitcoin2) are properly defined in the configuration structure (AdditionalAccounts) - The accounts are correctly included in the configuration generation process
- No legacy or incorrect Bitcoin account references were found
- Multiple Bitcoin-related test files exist that can utilize these accounts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if E2E tests properly reference the new Bitcoin account names
# Expected: Find references to both user_bitcoin1 and user_bitcoin2 in test files
echo "Checking for Bitcoin account references in E2E tests..."
rg -l "user_bitcoin[12]" "e2e/"
echo "Checking for any remaining references to the old account name..."
rg -l "user_bitcoin[^12]" "e2e/"
Length of output: 345
Script:
#!/bin/bash
# Let's examine the actual content of the config file to understand the usage
echo "Checking the content of e2e/config/config.go for Bitcoin account references:"
rg -A 5 -B 5 "user_bitcoin[12]" "e2e/config/config.go"
# Let's also check for any test files that might be using these accounts
echo -e "\nChecking for any test files using these accounts:"
fd -e go -e yaml -e yml . e2e/ --exec rg -l "user_bitcoin"
# Let's examine the content of local.yml to understand the configuration structure
echo -e "\nChecking the content of local.yml files:"
fd "local.yml" . --exec cat {}
Length of output: 7575
Script:
#!/bin/bash
# Let's check for any test files that might be using these Bitcoin accounts
echo "Checking for test files using Bitcoin accounts or their addresses:"
rg -l "zeta19q7czqysah6qg0n4y3l2a08gfzqxydla492v80|zeta17e77anpmzhuuam67hg6x3mtqrulqh80z9chv70|0x283d810090EdF4043E75247eAeBcE848806237fD|0xf67deecc3B15F9CEeF5eba3468ed601f3e0B9de2" e2e/
# Let's check the implementation details where these accounts are used
echo -e "\nChecking implementation details:"
rg -A 5 -B 5 "UserBitcoin[12]" e2e/
# Let's also check for any Bitcoin-related test configurations
echo -e "\nChecking for Bitcoin test configurations:"
fd -e go -e yaml -e yml . e2e/ --exec rg -l "bitcoin"
Length of output: 3113
Script:
#!/bin/bash
# Let's examine the Bitcoin test files to see how these accounts are used
echo "Checking Bitcoin test implementations:"
rg -A 5 -B 5 "user_bitcoin[12]" e2e/e2etests/test_bitcoin_*.go
# Let's also check the helpers and runner files
echo -e "\nChecking helper implementations:"
rg -A 5 -B 5 "user_bitcoin[12]" e2e/e2etests/helpers.go e2e/runner/bitcoin.go e2e/runner/balances.go
# Let's verify if there are any old Bitcoin account references
echo -e "\nChecking for any old Bitcoin account references:"
rg "user_bitcoin[^12]" e2e/
Length of output: 1282
🧰 Tools
🪛 Gitleaks
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
e2e/runner/logger.go (1)
18-18: LGTM: Padding increase aligns with test split requirements.
The increase in padding from 10 to 12 characters accommodates longer prefixes needed for the split Bitcoin test runners.
contrib/localnet/orchestrator/start-zetae2e.sh (1)
101-105: Add explicit validation for funding operations.
While the funding operations are functionally correct, consider adding explicit validation:
Consider adding these improvements to the fund_eth_from_config function:
- Explicit check for geth RPC availability before funding
- Validation of successful fund transfers
- Retry mechanism for failed transfers
x/observer/types/chain_params.go (1)
293-293: LGTM: OutboundScheduleInterval reduction optimizes test execution
The reduction of OutboundScheduleInterval from 2 to 1 for Bitcoin regtest chain parameters is appropriate for the test environment and aligns with the PR objective to optimize Bitcoin E2E test execution time. The value remains within the valid range (0 < value ≤ 100) enforced by ValidateChainParams.
Let's verify the impact of this change on other test configurations:
✅ Verification successful
OutboundScheduleInterval change is safe for Bitcoin regtest environment
The reduction of OutboundScheduleInterval from 2 to 1 in the Bitcoin regtest chain parameters is verified to be safe because:
- The change only affects the test environment (regtest)
- All production configurations maintain the standard 30-second interval
- Test timing patterns are based on block times or receipt confirmations, not the schedule interval
- The value remains within valid bounds as other test configurations also use 1 as a valid interval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any test configurations that might be affected by the OutboundScheduleInterval change
# Search for test configurations referencing OutboundScheduleInterval
rg -l 'OutboundScheduleInterval.*[12]' 'test/' 'e2e/'
# Search for any hardcoded timing assumptions in Bitcoin tests
rg -i 'sleep|wait.*[12].*second' 'test/' 'e2e/' -g '*bitcoin*'
Length of output: 387
Script:
#!/bin/bash
# Let's verify the impact more thoroughly by:
# 1. Checking all chain parameter configurations
# 2. Looking for any timing-related tests in Bitcoin
# 3. Examining the validation logic
# Check all chain parameter configurations
ast-grep --pattern 'OutboundScheduleInterval:\s+\d+'
# Look for Bitcoin-specific test files and timing patterns
rg -g '*bitcoin*.go' -l 'time.Sleep|WaitFor'
# Check the validation logic for OutboundScheduleInterval
ast-grep --pattern 'func ValidateChainParams'
Length of output: 342
Script:
#!/bin/bash
# Let's examine the specific timing patterns in Bitcoin test files
# and check other OutboundScheduleInterval configurations
# Check timing patterns in Bitcoin test files
rg -g '*bitcoin*.go' 'time.Sleep|WaitFor' -C 2
# Look for all OutboundScheduleInterval configurations
rg 'OutboundScheduleInterval:' -C 2
# Check the validation function implementation
rg 'func.*ValidateChainParams' -A 10
Length of output: 12084
e2e/config/config.go (3)
67-68: LGTM: Clean addition of split Bitcoin accounts.
The addition of UserBitcoin1 and UserBitcoin2 fields follows the existing naming convention and maintains consistent field tags. This change aligns well with the PR objective of splitting Bitcoin E2E tests into separate runners.
237-238: LGTM: Consistent slice representation.
The AsSlice method correctly includes both new Bitcoin accounts while maintaining the order consistency with the struct definition.
317-321: LGTM: Proper key generation for new accounts.
The GenerateKeys method correctly handles the generation of keys for both new Bitcoin accounts with appropriate error handling, following the established pattern.
However, let's verify that these are the only instances of the old UserBitcoin reference in the codebase to ensure a complete migration.
✅ Verification successful
Migration to UserBitcoin1 and UserBitcoin2 is complete and consistent
The verification confirms:
- No legacy references to
UserBitcoinexist in the codebase - New fields
UserBitcoin1andUserBitcoin2are consistently used across configuration and implementation files - All usages follow the same pattern in both YAML configuration and code references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old UserBitcoin field
# to ensure complete migration to UserBitcoin1 and UserBitcoin2
echo "Searching for any remaining references to 'UserBitcoin'..."
rg --type go "UserBitcoin[^12]" || echo "No remaining references found."
echo "Verifying consistent usage of new fields..."
rg --type go "UserBitcoin[12]"
Length of output: 915
cmd/zetae2e/local/local.go (3)
369-377: LGTM: Test routine initialization aligns with PR objectives.
The initialization of separate Bitcoin test routines for deposits and withdrawals aligns with the PR objective of splitting Bitcoin E2E tests to address CI timeout issues.
470-471: LGTM: Improved logging clarity.
The addition of a space in the logging message improves readability.
489-491: LGTM: Added balance verification for restricted addresses.
The addition of balance verification for restricted addresses enhances the test suite's robustness by ensuring proper cleanup after test execution.
cmd/zetae2e/local/bitcoin.go (1)
110-110: Replace require.NoError with standard error handling.
As previously mentioned, require.NoError should not be used in production code. Replace it with standard error handling.
| @@ -58,7 +58,7 @@ func runSetupBitcoin(_ *cobra.Command, args []string) error { | |||
| return err | |||
| } | |||
|
|
|||
| r.SetupBitcoinAccount(true) | |||
| r.SetupBitcoinAccounts(true) | |||
There was a problem hiding this comment.
Add error handling for SetupBitcoinAccounts.
The return value from SetupBitcoinAccounts should be checked for errors to maintain consistent error handling throughout the function.
Apply this diff:
- r.SetupBitcoinAccounts(true)
+ if err := r.SetupBitcoinAccounts(true); err != nil {
+ return err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r.SetupBitcoinAccounts(true) | |
| if err := r.SetupBitcoinAccounts(true); err != nil { | |
| return err | |
| } |
| if err != nil { | ||
| return err | ||
| } | ||
| addr, privKey := r.GetBtcAddress() |
There was a problem hiding this comment.
Restore error handling for GetBtcAddress.
The removal of error handling from GetBtcAddress could mask potential issues during address generation or retrieval. Bitcoin address operations are critical and should include proper error handling.
Apply this diff to restore error handling:
-addr, privKey := r.GetBtcAddress()
+addr, privKey, err := r.GetBtcAddress()
+if err != nil {
+ cancel()
+ return err
+}Committable suggestion skipped: line range outside the PR's diff.
| return "", fmt.Errorf("failed to decode BTC address: %w", err) | ||
| } | ||
|
|
||
| address, _ := r.GetBtcAddress() |
There was a problem hiding this comment.
Restore proper error handling for GetBtcAddress.
Discarding errors from GetBtcAddress could lead to runtime panics if the address is invalid. Even in test code, proper error handling is crucial for debugging and maintainability.
Apply this diff to restore error handling:
- address, _ := r.GetBtcAddress()
+ address, err := r.GetBtcAddress()
+ if err != nil {
+ return "", fmt.Errorf("failed to get BTC address: %w", err)
+ }This change:
- Preserves error context for easier debugging
- Prevents potential runtime panics
- Follows Go's error handling best practices
Committable suggestion skipped: line range outside the PR's diff.
| // initRunnerDeposit initializes the runner for deposit tests | ||
| func initRunnerDeposit( | ||
| conf config.Config, | ||
| deployerRunner *runner.E2ERunner, | ||
| verbose, initNetwork bool, | ||
| ) *runner.E2ERunner { | ||
| var ( | ||
| name = "btc_deposit" | ||
| account = conf.AdditionalAccounts.UserBitcoin1 | ||
| createWallet = true // deposit tests need Bitcoin node wallet to handle UTXOs | ||
| ) | ||
|
|
||
| return initRunner(name, account, conf, deployerRunner, verbose, initNetwork, createWallet) | ||
| } | ||
|
|
||
| // initRunnerWithdraw initializes the runner for withdraw tests | ||
| func initRunnerWithdraw( | ||
| conf config.Config, | ||
| deployerRunner *runner.E2ERunner, | ||
| verbose, initNetwork bool, | ||
| ) *runner.E2ERunner { | ||
| var ( | ||
| name = "btc_withdraw" | ||
| account = conf.AdditionalAccounts.UserBitcoin2 | ||
| createWallet = false // withdraw tests DON'T use Bitcoin node wallet | ||
| ) | ||
|
|
||
| return initRunner(name, account, conf, deployerRunner, verbose, initNetwork, createWallet) | ||
| } |
There was a problem hiding this comment.
These wrappers seem to be too thin to necessitate their usage. Can we just call initRunner() directly in startBitcoinTestRoutines()
| // initialize funds | ||
| // send BTC to TSS for gas fees and to tester ZEVM address | ||
| if initNetwork { | ||
| // mine 101 blocks to ensure the BTC rewards are spendable | ||
| // Note: the block rewards can be sent to any address in here | ||
| _, err := runnerDeposit.GenerateToAddressIfLocalBitcoin(101, runnerDeposit.BTCDeployerAddress) | ||
| require.NoError(runnerDeposit, err) | ||
|
|
||
| // send BTC to ZEVM addresses | ||
| runnerDeposit.DepositBTC(runnerDeposit.EVMAddress()) | ||
| runnerDeposit.DepositBTC(runnerWithdraw.EVMAddress()) | ||
| } |
There was a problem hiding this comment.
I don't understand why we can't do this in the individual runners?
Description
Done:
deposit,withdrawbefore split:
after split:
Closes: #3080
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation
Refactor