test: fix upgrade tests #3196
test: fix upgrade tests #3196
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new command-line flag, Changes
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
|
kingpinXD
added
UPGRADE_TESTS
kingpinXD
changed the title
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
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: 3
🧹 Outside diff range and nitpick comments (9)
e2e/e2etests/test_zrc20_swap.go (2)
Line range hint
16-21: Consider implementing a more robust initialization strategy.The current approach of ignoring initialization failures, while functional, could be improved for better test reliability and clarity. Consider implementing a helper function that checks for pair existence before attempting creation.
func ensurePairExists(r *runner.E2ERunner) (ethcommon.Address, error) { pair, err := r.UniswapV2Factory.GetPair(&bind.CallOpts{}, r.ERC20ZRC20Addr, r.ETHZRC20Addr) if err != nil { return ethcommon.Address{}, err } if pair != (ethcommon.Address{}) { return pair, nil } tx, err := r.UniswapV2Factory.CreatePair(r.ZEVMAuth, r.ERC20ZRC20Addr, r.ETHZRC20Addr) if err != nil { return ethcommon.Address{}, err } receipt := utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) return r.UniswapV2Factory.GetPair(&bind.CallOpts{}, r.ERC20ZRC20Addr, r.ETHZRC20Addr) }
Line range hint
45-52: Consider restructuring gas limit management.The current gas limit modification could be encapsulated in a helper function for better reusability and cleaner test structure.
func withCustomGasLimit(r *runner.E2ERunner, gasLimit uint64, fn func() error) error { previousGasLimit := r.ZEVMAuth.GasLimit r.ZEVMAuth.GasLimit = gasLimit defer func() { r.ZEVMAuth.GasLimit = previousGasLimit }() return fn() }Usage example:
err := withCustomGasLimit(r, 400000, func() error { tx, err := r.UniswapV2Router.AddLiquidity( r.ZEVMAuth, // ... existing parameters ... ) if err != nil { return err } receipt = utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) return nil }) require.NoError(r, err)e2e/e2etests/test_v2_deposit_and_call_swap.go (2)
Line range hint
13-16: Add timeline for test deprecationThe TODO comment provides good context and issue reference. Consider adding an expected timeline for removal to help track technical debt.
-// TODO: This test is similar to TestCrosschainSwap +// TODO(#2711): This test is similar to TestCrosschainSwap and should be removed by Q2 2025
Line range hint
29-102: Improve test structure and readabilityThe test implementation could benefit from the following improvements:
- Extract setup steps into helper functions
- Encapsulate gas limit manipulation
- Define constants for magic numbers (e.g.,
1e18,1e10,1e8)Example refactor for gas limit manipulation:
func withTemporaryGasLimit(r *runner.E2ERunner, gasLimit uint64, fn func() error) error { previousLimit := r.ZEVMAuth.GasLimit r.ZEVMAuth.GasLimit = gasLimit defer func() { r.ZEVMAuth.GasLimit = previousLimit }() return fn() }Usage:
err := withTemporaryGasLimit(r, 400000, func() error { tx, err := r.UniswapV2Router.AddLiquidity(...) if err != nil { return err } return utils.WaitForTxReceipt(...) }) require.NoError(r, err)e2e/e2etests/test_crosschain_swap.go (1)
Line range hint
24-29: Consider refactoring pair creation into test setupThe TODO comment indicates that pair creation should be moved to setup. This would improve test organization and prevent potential side effects between test runs.
Consider:
- Moving pair creation to a shared setup function
- Adding a check for existing pairs
- Using test fixtures or a setup/teardown pattern
Example approach:
func setupUniswapPair(r *runner.E2ERunner) error { // Check if pair exists first pair, err := r.UniswapV2Factory.GetPair(nil, r.ERC20ZRC20Addr, r.BTCZRC20Addr) if err != nil { return fmt.Errorf("failed to check existing pair: %w", err) } if pair != (common.Address{}) { r.Logger.Printf("ℹ️ pair already exists at %s", pair.Hex()) return nil } // Create new pair if needed _, err = r.UniswapV2Factory.CreatePair(r.ZEVMAuth, r.ERC20ZRC20Addr, r.BTCZRC20Addr) if err != nil { return fmt.Errorf("failed to create pair: %w", err) } return nil }contrib/localnet/orchestrator/start-zetae2e.sh (2)
225-226: LGTM! Consider tracking the temporary flag.The addition of the
--skip-bitcoin-dust-withdrawflag aligns with the PR objectives. However, since this is a temporary change (as indicated by the comment), it would be beneficial to track its removal.Would you like me to create a GitHub issue to track the removal of this flag after v23 is released?
Line range hint
1-1: Consider enhancing script robustness and security.Several improvements could make the script more robust and secure:
- Add strict shell options at the beginning:
#!/bin/bash +set -euo pipefail
- Add timeout to curl commands to prevent hanging:
-curl -s -f zetacore0:1317/cosmos/base/tendermint/v1beta1/node_info +curl --max-time 10 -s -f zetacore0:1317/cosmos/base/tendermint/v1beta1/node_info
- Consider moving hardcoded values to configuration:
-UPGRADE_HEIGHT=${UPGRADE_HEIGHT:=225} +UPGRADE_HEIGHT=${UPGRADE_HEIGHT:=${DEFAULT_UPGRADE_HEIGHT}}
- Add error handling for external commands:
-solana airdrop 100 "$relayer" > /dev/null +if ! solana airdrop 100 "$relayer" > /dev/null; then + echo "Failed to fund solana relayer" + exit 1 +ficmd/zetae2e/local/local.go (2)
90-91: Consider enhancing the flag description.While the current description is clear, it could be more informative by explaining when and why one might want to skip these tests.
- cmd.Flags().Bool(flagSkipBitcoinDustWithdraw, false, "set to true to skip tests that withdraw dust amount from Bitcoin") + cmd.Flags().Bool(flagSkipBitcoinDustWithdraw, false, "set to true to skip Bitcoin dust withdrawal tests (useful for environments where dust transactions are not supported or when testing other Bitcoin functionality)")
336-338: Consider a more idiomatic implementation.The current implementation is correct but could be more elegant using a conditional initialization.
- if !skipBitcoinDustWithdraw { - bitcoinDepositTests = append(bitcoinDepositTests, e2etests.TestBitcoinDepositAndCallRevertWithDustName) - } + bitcoinDepositTests := []string{ + e2etests.TestBitcoinDonationName, + e2etests.TestBitcoinDepositName, + e2etests.TestBitcoinDepositAndCallName, + e2etests.TestBitcoinDepositAndCallRevertName, + e2etests.TestBitcoinStdMemoDepositName, + e2etests.TestBitcoinStdMemoDepositAndCallName, + e2etests.TestBitcoinStdMemoDepositAndCallRevertName, + e2etests.TestBitcoinStdMemoDepositAndCallRevertOtherAddressName, + e2etests.TestBitcoinStdMemoInscribedDepositAndCallName, + e2etests.TestCrosschainSwapName, + } + if !skipBitcoinDustWithdraw { + bitcoinDepositTests = append(bitcoinDepositTests, + e2etests.TestBitcoinDepositAndCallRevertWithDustName) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
cmd/zetae2e/local/local.go(4 hunks)contrib/localnet/orchestrator/start-zetae2e.sh(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(1 hunks)e2e/e2etests/test_crosschain_swap.go(1 hunks)e2e/e2etests/test_v2_deposit_and_call_swap.go(1 hunks)e2e/e2etests/test_zrc20_swap.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go
🧰 Additional context used
📓 Path-based instructions (5)
cmd/zetae2e/local/local.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.
e2e/e2etests/test_crosschain_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_v2_deposit_and_call_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_zrc20_swap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
cmd/zetae2e/local/local.go (2)
28-50: LGTM! Flag constant follows established patterns.
The new flag constant flagSkipBitcoinDustWithdraw is well-placed and follows the existing naming conventions.
100-121: LGTM! Consistent flag value retrieval.
The flag value is retrieved using the must helper, maintaining consistency with other flag retrievals in the file.
|
We should not introduce a flag and make cli more complex for just an edge case |
|
Removed the flag, we already have the light flag pattern for test to not run in previous version and should reuse this |
Description
Fixes the upgrade and upgrade-light tests by making the following changes
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation