Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add E2E testing for V2 contract migration #2763

Merged
merged 24 commits into from
Aug 30, 2024
Merged

Conversation

lumtis
Copy link
Member

@lumtis lumtis commented Aug 21, 2024

Description

Add

make start-upgrade-v2-migration-test

that will:

Run same workflow as upgrade test light from v18 (to setup environment with v1 contracts)

After upgrade:

  • Run migration instructions as per playbook
  • Run v1 tests to check backward compatibility
  • Run v2 tests

Add a few fixes:

Summary by CodeRabbit

  • New Features

    • Introduced a new boolean input parameter for migration tests in the GitHub Actions workflow.
    • Added new target for testing the upgrade process from version 18 to version 2 in the Makefile.
    • Enhanced user account configurations with new entries for various transaction types.
    • Implemented a new function to orchestrate end-to-end tests related to version 2 of the system.
    • Added new commands to facilitate funding of Ethereum accounts for testing purposes.
    • Introduced a method for adding new authorizations within the authority management system.
  • Bug Fixes

    • Enhanced balance checking logic to prevent unnecessary calls in the custody balance retrieval.
  • Documentation

    • Improved comments and logging for clarity and consistency across various modules.
  • Chores

    • Updated the dependency version for protocol contracts to ensure compatibility with the latest features.

Copy link
Contributor

coderabbitai bot commented Aug 21, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Walkthrough

The changes encompass the introduction of new features and enhancements to existing functionalities across various components of the system. Key updates include the addition of migration testing parameters, user account configurations for version 2, enhancements to end-to-end testing routines, and improvements to contract management processes. The modifications facilitate better handling of version migrations, optimize balance checks, and refine error handling and logging mechanisms.

Changes

File Path Change Summary
.github/workflows/e2e.yml Introduced v2-migration-test input parameter and V2_MIGRATION_TESTS output variable for enhanced migration testing control.
Makefile Added start-upgrade-v2-migration-test target for testing the upgrade process from version 18 to version 2.
cmd/zetae2e/config/localnet.yml Added four new user account configurations for v2 interactions under additional_accounts.
cmd/zetae2e/local/local.go Enhanced localE2ETest function to include a flag for v2 migration tests and adjusted logic for handling migration scenarios.
cmd/zetae2e/local/post_tss_migration.go Renamed postMigrationTestRoutine to postTSSMigrationTestRoutine to clarify focus on TSS migration tests.
cmd/zetae2e/local/tss_migration.go Renamed migrationTestRoutine to tssMigrationTestRoutine for clarity and updated logging messages.
cmd/zetae2e/local/v2.go Introduced startV2Tests function for orchestrating v2 end-to-end tests with parallel execution.
contrib/localnet/orchestrator/start-zetae2e.sh Added commands to fund additional accounts for v2 tests.
contrib/localnet/scripts/start-zetacored.sh Enhanced account management for version 2 by adding funding commands for new user accounts.
e2e/config/config.go Updated AdditionalAccounts struct to include new fields for v2 user accounts and modified related methods for initialization.
e2e/e2etests/test_migrate_erc20_custody_funds.go Removed comments regarding migration test context, streamlining the code.
e2e/runner/accounting.go Improved balance check logic for new ERC20 custody contract, optimizing performance.
e2e/runner/evm.go Modified approval logic in ApproveETHZRC20 and ApproveERC20ZRC20 functions to broaden approval criteria.
e2e/runner/v2_migration.go Implemented comprehensive migration process for v2, including contract deployment and custody fund migration.
e2e/runner/v2_setup_evm.go Enhanced logging in SetupEVMV2 method and added legacy support handling for ERC20 custody.
e2e/runner/v2_setup_zeta.go Renamed UpdateChainParamsERC20CustodyContract to UpdateChainParamsV2Contracts to reflect broader functionality and added gateway address handling.
e2e/txserver/authority.go Added AddAuthorization method for dynamic addition of authorizations in the authority module.
e2e/txserver/zeta_tx_server.go Minor cosmetic change in UpdateGatewayAddress method for improved readability.
go.mod Updated github.com/zeta-chain/protocol-contracts dependency to a newer version.
x/fungible/keeper/msg_server_update_gateway_contract.go Enhanced UpdateGatewayContract function with validation for new gateway address and updates for associated ZRC20 contracts.
x/fungible/keeper/msg_server_update_gateway_contract_test.go Expanded TestKeeper_UpdateGatewayContract to verify updates across ZRC20 contracts and added test for invalid gateway address handling.
x/fungible/keeper/v2_evm.go Introduced CallUpdateGatewayAddress method for updating gateway addresses in ZRC20 contracts.
zetaclient/chains/bitcoin/observer/inbound.go Refined error logging in WatchInbound method to suppress specific error messages.
zetaclient/chains/bitcoin/observer/observer.go Updated error handling in WatchUTXOs method to suppress logging for specific wallet conditions.
zetaclient/chains/evm/signer/signer.go Enhanced SetGatewayAddress method to actively manage the gateway address state.
zetaclient/orchestrator/orchestrator.go Updated resolveSigner and runScheduler methods for improved address management and transaction handling.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Workflow
    participant Contract
    participant Keeper

    User->>Workflow: Trigger v2 Migration Test
    Workflow->>Keeper: Set up migration parameters
    Keeper->>Contract: Deploy new v2 contracts
    Contract-->>Keeper: Confirm deployment
    Keeper->>Keeper: Update gateway address
    Keeper->>User: Migration test completed
Loading

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@lumtis lumtis linked an issue Aug 21, 2024 that may be closed by this pull request
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 37.68116% with 43 lines in your changes missing coverage. Please review.

Project coverage is 66.61%. Comparing base (7b34d1d) to head (07f7a66).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
zetaclient/chains/bitcoin/observer/inbound.go 0.00% 13 Missing ⚠️
zetaclient/orchestrator/orchestrator.go 8.33% 10 Missing and 1 partial ⚠️
...gible/keeper/msg_server_update_gateway_contract.go 55.00% 7 Missing and 2 partials ⚠️
zetaclient/chains/bitcoin/observer/observer.go 0.00% 4 Missing ⚠️
zetaclient/chains/evm/signer/signer.go 0.00% 4 Missing ⚠️
x/fungible/keeper/v2_evm.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2763      +/-   ##
===========================================
- Coverage    66.65%   66.61%   -0.04%     
===========================================
  Files          370      370              
  Lines        20680    20732      +52     
===========================================
+ Hits         13784    13811      +27     
- Misses        6261     6283      +22     
- Partials       635      638       +3     
Files with missing lines Coverage Δ
x/fungible/keeper/v2_evm.go 27.41% <87.50%> (+8.90%) ⬆️
zetaclient/chains/bitcoin/observer/observer.go 32.89% <0.00%> (-0.34%) ⬇️
zetaclient/chains/evm/signer/signer.go 47.15% <0.00%> (-0.46%) ⬇️
...gible/keeper/msg_server_update_gateway_contract.go 72.72% <55.00%> (-6.44%) ⬇️
zetaclient/orchestrator/orchestrator.go 22.04% <8.33%> (-0.30%) ⬇️
zetaclient/chains/bitcoin/observer/inbound.go 14.13% <0.00%> (-0.27%) ⬇️

@lumtis lumtis added V2_TESTS Run make start-v2-test V2_MIGRATION_TESTS Run make start-upgrade-v2-migration-test labels Aug 27, 2024
@github-actions github-actions bot added the ci Changes to CI pipeline or github actions label Aug 27, 2024
@lumtis lumtis marked this pull request as ready for review August 27, 2024 17:17
@lumtis lumtis linked an issue Aug 27, 2024 that may be closed by this pull request
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Outside diff range, codebase verification and nitpick comments (3)
cmd/zetae2e/local/post_tss_migration.go (1)

Line range hint 14-54: Enhance error handling and add logging.

  1. Enhance error handling to provide more context.
  2. Add logging for better traceability of the steps.

Apply this diff to improve the function:

 func postTSSMigrationTestRoutine(
 	conf config.Config,
 	deployerRunner *runner.E2ERunner,
 	verbose bool,
 	testNames ...string,
 ) func() error {
 	return func() (err error) {
 		account := conf.AdditionalAccounts.UserBitcoin
 		// initialize runner for post migration test
+		postMigrationRunner.Logger.Print("Initializing post migration runner")
 		postMigrationRunner, err := initTestRunner(
 			"postTSSMigration",
 			conf,
 			deployerRunner,
 			account,
 			runner.NewLogger(verbose, color.FgMagenta, "postTSSMigration"),
 		)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to initialize post migration runner: %w", err)
 		}

 		postMigrationRunner.Logger.Print("🏃 starting post TSS migration tests")
 		startTime := time.Now()

 		testsToRun, err := postMigrationRunner.GetE2ETestsToRunByName(
 			e2etests.AllE2ETests,
 			testNames...,
 		)
 		if err != nil {
-			return errors.Wrap(err, "post TSS migration tests failed")
+			return errors.Wrap(err, "failed to get E2E tests to run: %w", err)
 		}

 		if err := postMigrationRunner.RunE2ETests(testsToRun); err != nil {
-			return errors.Wrap(err, "post TSS migration tests failed")
+			return errors.Wrap(err, "failed to run E2E tests: %w", err)
 		}

 		if err := postMigrationRunner.CheckBtcTSSBalance(); err != nil {
-			return err
+			return fmt.Errorf("failed to check BTC TSS balance: %w", err)
 		}

 		postMigrationRunner.Logger.Print("🍾 post TSS migration tests completed in %s", time.Since(startTime).String())

 		return err
 	}
 }
cmd/zetae2e/local/tss_migration.go (1)

Line range hint 14-59: Enhance error handling and add logging.

  1. Enhance error handling to provide more context.
  2. Add logging for better traceability of the steps.

Apply this diff to improve the function:

 func tssMigrationTestRoutine(
 	conf config.Config,
 	deployerRunner *runner.E2ERunner,
 	verbose bool,
 	testNames ...string,
 ) func() error {
 	return func() (err error) {
 		account := conf.AdditionalAccounts.UserMigration
 		// initialize runner for migration test
+		tssMigrationTestRunner.Logger.Print("Initializing TSS migration runner")
 		tssMigrationTestRunner, err := initTestRunner(
 			"TSSmigration",
 			conf,
 			deployerRunner,
 			account,
 			runner.NewLogger(verbose, color.FgHiGreen, "migration"),
 			runner.WithZetaTxServer(deployerRunner.ZetaTxServer),
 		)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to initialize TSS migration runner: %w", err)
 		}

 		tssMigrationTestRunner.Logger.Print("🏃 starting TSS migration tests")
 		startTime := time.Now()

 		if len(testNames) == 0 {
 			tssMigrationTestRunner.Logger.Print("🍾 TSS migration tests completed in %s", time.Since(startTime).String())
 			return nil
 		}
 		// run TSS migration test
+		tssMigrationTestRunner.Logger.Print("Running TSS migration tests")
 		testsToRun, err := tssMigrationTestRunner.GetE2ETestsToRunByName(
 			e2etests.AllE2ETests,
 			testNames...,
 		)
 		if err != nil {
-			return fmt.Errorf("TSS migration tests failed: %v", err)
+			return fmt.Errorf("failed to get E2E tests to run: %w", err)
 		}

 		if err := tssMigrationTestRunner.RunE2ETests(testsToRun); err != nil {
-			return fmt.Errorf("TSS migration tests failed: %v", err)
+			return fmt.Errorf("failed to run E2E tests: %w", err)
 		}
 		if err := tssMigrationTestRunner.CheckBtcTSSBalance(); err != nil {
-			return err
+			return fmt.Errorf("failed to check BTC TSS balance: %w", err)
 		}

 		tssMigrationTestRunner.Logger.Print("🍾 TSS migration tests completed in %s", time.Since(startTime).String())

 		return err
 	}
 }
e2e/runner/v2_migration.go (1)

52-68: Enhance Error Handling in upgradeZRC20s Function

The upgradeZRC20s function currently lacks explicit error handling for the upgradeZRC20 calls. While errors from ChainID are checked, failures in upgrading ZRC20s should also be managed to ensure the process is robust and reliable. Consider adding error checks for each upgradeZRC20 call to handle potential issues effectively.

  • File: e2e/runner/v2_migration.go
  • Lines: 59, 63, 67
Analysis chain

Verify the correctness of each upgrade step in upgradeZRC20s.

The function upgradeZRC20s includes multiple steps for upgrading ZRC20s. Ensure that each step handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `upgradeZRC20s` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'upgradeZRC20s'

Length of output: 743


Script:

#!/bin/bash
# Extract the full implementation of the `upgradeZRC20s` function to verify error handling and edge case management.
ast-grep --lang go --pattern $'func (r *E2ERunner) upgradeZRC20s() {
  $$$
}' 

Length of output: 1236

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1d5cee1 and 6397079.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (26)
  • .github/workflows/e2e.yml (7 hunks)
  • Makefile (1 hunks)
  • cmd/zetae2e/config/localnet.yml (1 hunks)
  • cmd/zetae2e/local/local.go (9 hunks)
  • cmd/zetae2e/local/post_tss_migration.go (2 hunks)
  • cmd/zetae2e/local/tss_migration.go (3 hunks)
  • cmd/zetae2e/local/v2.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_migrate_erc20_custody_funds.go (1 hunks)
  • e2e/runner/accounting.go (1 hunks)
  • e2e/runner/evm.go (2 hunks)
  • e2e/runner/v2_migration.go (1 hunks)
  • e2e/runner/v2_setup_evm.go (2 hunks)
  • e2e/runner/v2_setup_zeta.go (2 hunks)
  • e2e/txserver/authority.go (1 hunks)
  • e2e/txserver/zeta_tx_server.go (1 hunks)
  • go.mod (1 hunks)
  • x/fungible/keeper/msg_server_update_gateway_contract.go (3 hunks)
  • x/fungible/keeper/msg_server_update_gateway_contract_test.go (3 hunks)
  • x/fungible/keeper/v2_evm.go (2 hunks)
  • zetaclient/chains/bitcoin/observer/inbound.go (2 hunks)
  • zetaclient/chains/bitcoin/observer/observer.go (2 hunks)
  • zetaclient/chains/evm/signer/signer.go (1 hunks)
  • zetaclient/orchestrator/orchestrator.go (3 hunks)
Files skipped from review due to trivial changes (2)
  • e2e/e2etests/test_migrate_erc20_custody_funds.go
  • e2e/txserver/zeta_tx_server.go
Additional context used
Path-based instructions (20)
e2e/txserver/authority.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

cmd/zetae2e/local/post_tss_migration.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

cmd/zetae2e/local/tss_migration.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/msg_server_update_gateway_contract.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/runner/v2_setup_zeta.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/runner/v2_setup_evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

cmd/zetae2e/local/v2.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/v2_evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/msg_server_update_gateway_contract_test.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/v2_migration.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/runner/evm.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/config/config.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/bitcoin/observer/inbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

contrib/localnet/scripts/start-zetacored.sh (1)

Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.

cmd/zetae2e/local/local.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/evm/signer/signer.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/bitcoin/observer/observer.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/orchestrator/orchestrator.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

Gitleaks
cmd/zetae2e/config/localnet.yml

47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


59-59: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

Additional comments not posted (57)
x/fungible/keeper/msg_server_update_gateway_contract.go (4)

8-9: LGTM!

The new imports are necessary for the added functionality.

The code changes are approved.


26-34: LGTM!

The parsing and validation of the new gateway address are correctly implemented.

The code changes are approved.


47-64: LGTM!

The update of ZRC20 contracts with the new gateway address is correctly implemented, and the error handling enhances robustness.

The code changes are approved.


Line range hint 66-74: LGTM!

The event emission and final steps are correctly implemented, enhancing the function's interactivity with other components of the system.

The code changes are approved.

e2e/runner/v2_setup_zeta.go (2)

78-80: LGTM!

The updated comment and method name accurately describe the expanded functionality.

The code changes are approved.


104-106: LGTM!

The expanded functionality enhances the method's capability to manage multiple contract addresses during transitions.

The code changes are approved.

e2e/runner/v2_setup_evm.go (2)

25-25: LGTM!

The improved logging provides a more structured logging level, enhancing clarity.

The code changes are approved.


107-112: LGTM!

The new functionality for setting legacy support enhances the method's capability to manage legacy support for deposits.

The code changes are approved.

cmd/zetae2e/local/v2.go (2)

15-71: Add error handling for eg.Go calls.

The function is well-structured and leverages concurrency for efficiency. However, it lacks error handling for the eg.Go calls, which could lead to unhandled errors. Consider adding error handling to ensure that any errors encountered during the execution of the test routines are properly managed.

 func startV2Tests(eg *errgroup.Group, conf config.Config, deployerRunner *runner.E2ERunner, verbose bool) {
 	// Test happy paths for gas token workflow
-	eg.Go(v2TestRoutine(conf, "eth", conf.AdditionalAccounts.UserV2Ether, color.FgHiGreen, deployerRunner, verbose,
+	eg.Go(func() error {
+		return v2TestRoutine(conf, "eth", conf.AdditionalAccounts.UserV2Ether, color.FgHiGreen, deployerRunner, verbose,
 		e2etests.TestV2ETHDepositName,
 		e2etests.TestV2ETHDepositAndCallName,
 		e2etests.TestV2ETHWithdrawName,
 		e2etests.TestV2ETHWithdrawAndCallName,
 		e2etests.TestV2ZEVMToEVMCallName,
 		e2etests.TestV2EVMToZEVMCallName,
-	))
+		)
+	})

 	// Test happy paths for erc20 token workflow
-	eg.Go(v2TestRoutine(conf, "erc20", conf.AdditionalAccounts.UserV2ERC20, color.FgHiBlue, deployerRunner, verbose,
+	eg.Go(func() error {
+		return v2TestRoutine(conf, "erc20", conf.AdditionalAccounts.UserV2ERC20, color.FgHiBlue, deployerRunner, verbose,
 		e2etests.TestV2ETHDepositName, // necessary to pay fees on ZEVM
 		e2etests.TestV2ERC20DepositName,
 		e2etests.TestV2ERC20DepositAndCallName,
 		e2etests.TestV2ERC20WithdrawName,
 		e2etests.TestV2ERC20WithdrawAndCallName,
-	))
+		)
+	})

 	// Test revert cases for gas token workflow
 	eg.Go(func() error {
 		return v2TestRoutine(
 			conf,
 			"eth-revert",
 			conf.AdditionalAccounts.UserV2EtherRevert,
 			color.FgHiYellow,
 			deployerRunner,
 			verbose,
 			e2etests.TestV2ETHDepositName, // necessary to pay fees on ZEVM and withdraw
 			e2etests.TestV2ETHDepositAndCallRevertName,
 			e2etests.TestV2ETHDepositAndCallRevertWithCallName,
 			e2etests.TestV2ETHWithdrawAndCallRevertName,
 			e2etests.TestV2ETHWithdrawAndCallRevertWithCallName,
 		)
 	})

 	// Test revert cases for erc20 token workflow
 	eg.Go(func() error {
 		return v2TestRoutine(
 			conf,
 			"erc20-revert",
 			conf.AdditionalAccounts.UserV2ERC20Revert,
 			color.FgHiRed,
 			deployerRunner,
 			verbose,
 			e2etests.TestV2ETHDepositName,   // necessary to pay fees on ZEVM
 			e2etests.TestV2ERC20DepositName, // necessary to have assets to withdraw
 			e2etests.TestOperationAddLiquidityETHName, // liquidity with gas and ERC20 are necessary for reverts
 			e2etests.TestOperationAddLiquidityERC20Name,
 			e2etests.TestV2ERC20DepositAndCallRevertName,
 			e2etests.TestV2ERC20DepositAndCallRevertWithCallName,
 			e2etests.TestV2ERC20WithdrawAndCallRevertName,
 			e2etests.TestV2ERC20WithdrawAndCallRevertWithCallName,
 		)
 	})
 }

Line range hint 74-122: LGTM!

The function is well-implemented and follows best practices. The TODO comment is a good indication of future improvements.

The code changes are approved.

x/fungible/keeper/v2_evm.go (2)

18-42: Add validation for the new gateway address.

The function is well-implemented and follows best practices. However, there is no validation for the new gateway address. Consider adding a validation check to ensure that the new gateway address is not empty or invalid.

 func (k Keeper) CallUpdateGatewayAddress(
 	ctx sdk.Context,
 	zrc20Address common.Address,
 	newGatewayAddress common.Address,
 ) (*evmtypes.MsgEthereumTxResponse, error) {
 	zrc20ABI, err := zrc20.ZRC20MetaData.GetAbi()
 	if err != nil {
 		return nil, err
 	}

+	if crypto.IsEmptyAddress(newGatewayAddress) {
+		return nil, fmt.Errorf("new gateway address is empty or invalid")
+	}

 	return k.CallEVM(
 		ctx,
 		*zrc20ABI,
 		types.ModuleAddressEVM,
 		zrc20Address,
 		BigIntZero,
 		nil,
 		true,
 		false,
 		"updateGatewayAddress",
 		newGatewayAddress,
 	)
 }

196-203: LGTM!

The changes are minor and do not affect the logic or functionality of the code.

The code changes are approved.

x/fungible/keeper/msg_server_update_gateway_contract_test.go (5)

18-91: LGTM!

The changes enhance the test by adding more comprehensive checks for the gateway contract address update in ZRC20 contracts.

The code changes are approved.


Line range hint 92-123: LGTM!

The changes add a new test case to ensure that the gateway contract can be updated and overwritten if the system contract state variable is not found.

The code changes are approved.


Line range hint 124-153: LGTM!

The changes add a new test case to ensure that the gateway contract cannot be updated if the user is not an admin.

The code changes are approved.


156-177: LGTM!

The changes add a new test case to ensure that the gateway contract cannot be updated if the gateway address is invalid.

The code changes are approved.


43-63: Verify the correctness of the queryZRC20Gateway function.

The function queryZRC20Gateway is correctly implemented but ensure that it handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Verification successful

Function queryZRC20Gateway is correctly implemented and used.

The function queryZRC20Gateway is used within test assertions to verify gateway addresses, and its implementation handles errors appropriately. The usage is consistent with its intended purpose in the test cases.

  • The function is defined and used in x/fungible/keeper/msg_server_update_gateway_contract_test.go.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `queryZRC20Gateway` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'queryZRC20Gateway'

Length of output: 1998

e2e/runner/accounting.go (2)

173-183: LGTM!

The changes optimize the function by preventing unnecessary balance checks for the new ERC20 custody contract.

The code changes are approved.


176-183: Verify the correctness of the new logic.

The new logic correctly checks if the ERC20CustodyAddr and ERC20CustodyV2Addr are different before querying the balance of the new contract. Ensure that this logic handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Verification successful

The logic in checkERC20TSSBalance is correctly implemented. The function handles balance retrieval errors and ensures that the total balance is checked against the total supply, covering necessary edge cases. The logic for comparing addresses before querying balances is sound. No issues were found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `checkERC20TSSBalance` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'checkERC20TSSBalance'

Length of output: 609


Script:

#!/bin/bash
# Extract the full implementation of the `checkERC20TSSBalance` function to analyze error handling and edge case management.
ast-grep --lang go --pattern $'func (r *E2ERunner) checkERC20TSSBalance() error {
  $$$
}'

Length of output: 1954

e2e/runner/v2_migration.go (8)

20-48: LGTM!

The function RunV2Migration is comprehensive and covers all necessary steps for the v2 migration.

The code changes are approved.


50-68: LGTM!

The function upgradeZRC20s correctly upgrades all ZRC20s to the new version.

The code changes are approved.


77-123: LGTM!

The function upgradeZRC20 correctly upgrades a ZRC20 to the new version.

The code changes are approved.


125-203: LGTM!

The function migrateERC20CustodyFunds correctly migrates all funds of the ERC20 custody contract to the new version.

The code changes are approved.


70-75: LGTM!

The interface zrc20Caller correctly defines the necessary functions to call ZRC20 functions.

The code changes are approved.


126-203: Verify the correctness of each step in migrateERC20CustodyFunds.

The function migrateERC20CustodyFunds includes multiple steps for migrating ERC20 custody funds. Ensure that each step handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Verification successful

Verify error handling and edge cases in migrateERC20CustodyFunds.

The migrateERC20CustodyFunds function is defined and used only within e2e/runner/v2_migration.go. Since it is not invoked elsewhere, focus on ensuring robust error handling and coverage of edge cases within the function itself. Verify that each step checks for errors and handles them appropriately.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `migrateERC20CustodyFunds` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'migrateERC20CustodyFunds'

Length of output: 728


22-48: Verify the correctness of each step in RunV2Migration.

The function RunV2Migration includes multiple steps for the v2 migration. Ensure that each step handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Verification successful

Enhance Error Handling and Edge Case Coverage in RunV2Migration

The RunV2Migration function and its related functions effectively use require.NoError to handle errors, ensuring that execution halts on failure. However, to robustly handle potential edge cases, consider the following improvements:

  • Review Error Handling Strategy: Ensure that the current error handling covers potential edge cases such as network failures, transaction timeouts, or unexpected states.
  • Implement Retries: For known transient issues like network failures, consider implementing retry logic to enhance resilience.
  • Alternative Flows: Develop alternative flows or fallbacks for critical steps that might encounter unexpected conditions.

By addressing these areas, the migration process will be more robust and reliable.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `RunV2Migration` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'RunV2Migration'

Length of output: 800


Script:

#!/bin/bash
# Extract the implementation of RunV2Migration to analyze error handling.
ast-grep --lang go --pattern 'func (r *E2ERunner) RunV2Migration() { $$$ }'

# Extract implementations of related functions to check for error handling.
ast-grep --lang go --pattern 'func (r *E2ERunner) AddAuthorization($_) { $$$ }'
ast-grep --lang go --pattern 'func (r *E2ERunner) SetupEVMV2() { $$$ }'
ast-grep --lang go --pattern 'func (r *E2ERunner) upgradeZRC20s() { $$$ }'
ast-grep --lang go --pattern 'func (r *E2ERunner) SetZEVMContractsV2() { $$$ }'
ast-grep --lang go --pattern 'func (r *E2ERunner) migrateERC20CustodyFunds() { $$$ }'

Length of output: 18769


79-123: Verify the correctness of each step in upgradeZRC20.

The function upgradeZRC20 includes multiple steps for upgrading a ZRC20. Ensure that each step handles all possible error scenarios and edge cases.

Run the following script to verify the function usage:

Verification successful

Review the Error Handling in upgradeZRC20 Function

The upgradeZRC20 function effectively uses require.NoError to handle errors, ensuring that any failure stops execution immediately. While this is suitable for testing, consider implementing more robust error handling and retry logic if the function is intended for production use. This will help manage potential edge cases such as network failures or unexpected conditions.

  • Ensure that the function's error handling aligns with its intended use, especially if used outside a test environment.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `upgradeZRC20` match the expected usage.

# Test: Search for the function usage. Expect: Only occurrences of the expected usage.
rg --type go -A 5 $'upgradeZRC20'

Length of output: 2014


Script:

#!/bin/bash
# Extract the implementation of the `upgradeZRC20` function to analyze error handling and edge cases.
ast-grep --lang go --pattern $'func (r *E2ERunner) upgradeZRC20($_, $_, $_, $_) {
  $$$
}' --json

Length of output: 3456

e2e/runner/evm.go (2)

191-193: LGTM!

The changes enhance the approval criteria and improve code readability.

The code changes are approved.


208-210: LGTM!

The changes enhance the approval criteria and improve code readability.

The code changes are approved.

contrib/localnet/orchestrator/start-zetae2e.sh (4)

94-98: LGTM!

The changes follow a consistent pattern and appropriately use yq and geth commands.

The code changes are approved.


99-103: LGTM!

The changes follow a consistent pattern and appropriately use yq and geth commands.

The code changes are approved.


104-108: LGTM!

The changes follow a consistent pattern and appropriately use yq and geth commands.

The code changes are approved.


109-113: LGTM!

The changes follow a consistent pattern and appropriately use yq and geth commands.

The code changes are approved.

.github/workflows/e2e.yml (5)

56-59: LGTM!

The addition of the new input parameter is consistent with the existing parameters and enhances the workflow's capability.

The code changes are approved.


81-81: LGTM!

The addition of the new output variable is consistent with the existing output variables and enhances the workflow's capability.

The code changes are approved.


106-106: LGTM!

The changes ensure that the new migration test can be referenced and utilized in the job steps, enhancing the control flow of the workflow.

The code changes are approved.


121-121: LGTM!

The changes ensure that the new migration test can be referenced and utilized in the job steps, enhancing the control flow of the workflow.

The code changes are approved.


186-188: LGTM!

The addition of the new make-target is consistent with the existing make-targets and enhances the workflow's capability.

The code changes are approved.

e2e/config/config.go (3)

64-76: LGTM!

The new fields are correctly added to the AdditionalAccounts struct.

The code changes are approved.


232-235: LGTM!

The new fields are correctly included in the returned slice.

The code changes are approved.


328-343: LGTM!

The new fields are correctly initialized in the GenerateKeys method.

The code changes are approved.

zetaclient/chains/bitcoin/observer/inbound.go (1)

60-63: LGTM!

The change improves the clarity of the logs by preventing unnecessary error messages.

The code changes are approved.

Makefile (1)

317-325: LGTM!

The new target is correctly defined and enhances the functionality of the upgrade testing suite.

The code changes are approved.

go.mod (1)

46-46: LGTM!

The dependency update is appropriate and necessary for maintaining compatibility with the latest protocol contracts.

The code changes are approved.

contrib/localnet/scripts/start-zetacored.sh (5)

257-259: Verify the use of yq command.

Ensure that the yq command is available in the environment where this script will be executed. If not, consider adding a check or installing it within the script.


257-259: LGTM!

The addition of the v2 ether tester account is correctly implemented.

The code changes are approved.


260-262: LGTM!

The addition of the v2 erc20 tester account is correctly implemented.

The code changes are approved.


263-265: LGTM!

The addition of the v2 ether revert tester account is correctly implemented.

The code changes are approved.


266-268: LGTM!

The addition of the v2 erc20 revert tester account is correctly implemented.

The code changes are approved.

cmd/zetae2e/local/local.go (7)

46-46: LGTM!

The addition of the flagTestV2Migration constant is appropriate and necessary for enabling v2 migration tests.

The code changes are approved.


81-81: LGTM!

The registration of the flagTestV2Migration flag within the NewLocalCmd function is correctly implemented.

The code changes are approved.


109-109: Verify error handling in RunV2Migration method.

Ensure that the RunV2Migration method handles errors appropriately to prevent potential issues during the migration process.


242-254: LGTM!

The logic for handling the testV2Migration flag is correctly implemented and necessary for facilitating the v2 migration process.

The code changes are approved.


379-381: Verify error handling in UpdateChainParamsV2Contracts function.

Ensure that the UpdateChainParamsV2Contracts function handles errors appropriately to prevent potential issues during the v2 tests.


383-384: Verify error handling in startV2Tests function.

Ensure that the startV2Tests function handles errors appropriately to prevent potential issues during the v2 tests.


379-384: LGTM!

The logic for running v2 tests is correctly implemented and necessary for ensuring that v2 functionalities are validated.

The code changes are approved.

zetaclient/orchestrator/orchestrator.go (1)

392-401: Verify the impact on Prometheus gauge update logic.

Ensure that the change in the order of operations does not affect the accuracy of the Prometheus gauge update logic for pending transactions.

Run the following script to verify the impact:

Verification successful

Prometheus Gauge Update Logic Verified

The current order of operations ensures that the Prometheus gauge accurately reflects the state of cctxList, even when the list is empty. The logic is sound, and no issues were found with the update process.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact on Prometheus gauge update logic.

# Test: Search for the Prometheus gauge update logic. Expect: No issues in the order of operations.
rg --type go -A 5 $'metrics.PendingTxsPerChain'

Length of output: 440

@lumtis lumtis mentioned this pull request Aug 28, 2024
11 tasks
lumtis and others added 2 commits August 28, 2024 18:39
Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com>
Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com>
Copy link
Member

@gartnera gartnera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI stamp

@lumtis lumtis requested a review from swift1337 August 29, 2024 09:13
@lumtis lumtis enabled auto-merge August 30, 2024 11:15
@lumtis lumtis added this pull request to the merge queue Aug 30, 2024
Merged via the queue into develop with commit 47a0609 Aug 30, 2024
29 of 30 checks passed
@lumtis lumtis deleted the v2-migration-test branch August 30, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli ci Changes to CI pipeline or github actions V2_MIGRATION_TESTS Run make start-upgrade-v2-migration-test V2_TESTS Run make start-v2-test
Projects
None yet
4 participants