test(e2e): fix add liquidity occasional error (tentative) #3199
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📝 WalkthroughWalkthroughThe pull request implements several modifications across multiple files, primarily affecting ERC20 token handling and liquidity management in end-to-end tests. Key changes include increasing the amount of ERC20 tokens sent in tests, streamlining liquidity pool creation directly within test functions, adjusting deposit amounts, and enhancing error handling for liquidity operations. Additionally, error message clarity is improved in the context of transaction status checks, ensuring better debugging capabilities. Changes
Possibly related 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
|
lumtis
requested review from
brewmaster012,
kingpinXD,
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 (5)
cmd/zetae2e/local/erc20.go (1)
40-40: Consider defining the token amount as a documented constantWhile increasing the token amount may address liquidity-related test failures, using magic numbers reduces code maintainability. Consider defining this as a named constant with documentation explaining the minimum requirements.
+// MinimumTestERC20Amount represents the minimum amount of ERC20 tokens required +// for reliable liquidity pool operations in tests. This value ensures sufficient +// liquidity to prevent occasional test failures. +const MinimumTestERC20Amount = 10000 - txERC20Send := deployerRunner.SendERC20OnEvm(account.EVMAddress(), 10000) + txERC20Send := deployerRunner.SendERC20OnEvm(account.EVMAddress(), MinimumTestERC20Amount)e2e/utils/require.go (1)
19-25: Consider enhancing error message readability with structured formattingWhile the current error message is informative, its readability could be improved by using a structured format with line breaks.
Consider this alternative implementation:
- msg := fmt.Sprintf( - "cctx status is not %q cctx index %s, status: %s, error: %s", - expected.String(), - cctx.Index, - cctx.CctxStatus.StatusMessage, - cctx.CctxStatus.ErrorMessage, - ) + msg := fmt.Sprintf(`CCTX Status Mismatch: + Expected: %q + Index : %s + Status : %s + Error : %s`, + expected.String(), + cctx.Index, + cctx.CctxStatus.StatusMessage, + cctx.CctxStatus.ErrorMessage, + )e2e/runner/liquidity.go (1)
Line range hint
1-89: Consider enhancing error diagnosticsWhile reviewing the entire file, I notice that we could improve error diagnostics by capturing more context when liquidity operations fail.
Consider adding these diagnostic improvements:
- Log pool balances before and after the operation
- Include gas used and gas price in error messages
- Add transaction trace logging for failed transactions
Example helper:
func (r *E2ERunner) logPoolState(pairAddr common.Address, operation string) { reserves, err := r.UniswapV2Pair.GetReserves(&bind.CallOpts{}, pairAddr) if err != nil { r.Logger.Error("Failed to get reserves: %v", err) return } r.Logger.Info("%s - Pool reserves: %s / %s", operation, reserves.Reserve0.String(), reserves.Reserve1.String()) }e2e/e2etests/test_erc20_deposit_refund.go (1)
Line range hint
16-16: Enhance test documentation with clear scenario descriptionsWhile the test is well-structured, adding comprehensive documentation would improve maintainability and clarity.
Consider adding a detailed test description:
+// TestERC20DepositAndCallRefund tests the ERC20 deposit and refund functionality in two scenarios: +// 1. Without liquidity pool: Verifies that the transaction is aborted and can be refunded via admin +// 2. With liquidity pool: Verifies that the transaction is reverted and automatically refunded +// +// Test flow: +// - First scenario: Attempts deposit without liquidity pool +// - Verifies abort status and admin refund +// - Second scenario: Creates liquidity pool +// - Attempts deposit with invalid message +// - Verifies revert status and automatic refund func TestERC20DepositAndCallRefund(r *runner.E2ERunner, _ []string) {e2e/runner/evm.go (1)
63-64: Consider using constants for magic numbersWhile increasing the deposit amount to 100e18 is a valid fix for the occasional liquidity errors, the magic number
100should be defined as a constant to improve maintainability and documentation.Consider applying this change:
+ const defaultDepositMultiplier = 100 // multiplier for default deposit amount to ensure sufficient liquidity func (r *E2ERunner) DepositERC20() ethcommon.Hash { r.Logger.Print("⏳ depositing ERC20 into ZEVM") - oneHundred := big.NewInt(0).Mul(big.NewInt(1e18), big.NewInt(100)) + amount := big.NewInt(0).Mul(big.NewInt(1e18), big.NewInt(defaultDepositMultiplier)) - return r.DepositERC20WithAmountAndMessage(r.EVMAddress(), oneHundred, []byte{}) + return r.DepositERC20WithAmountAndMessage(r.EVMAddress(), amount, []byte{}) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
cmd/zetae2e/local/erc20.go(1 hunks)e2e/e2etests/test_erc20_deposit_refund.go(1 hunks)e2e/runner/evm.go(1 hunks)e2e/runner/liquidity.go(2 hunks)e2e/utils/require.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
cmd/zetae2e/local/erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_erc20_deposit_refund.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.
e2e/runner/liquidity.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/utils/require.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
cmd/zetae2e/local/erc20.go (1)
40-41: Verify if the increased amount resolves the occasional errors
Let's verify if this change consistently resolves the occasional add liquidity errors mentioned in the PR objectives.
e2e/utils/require.go (1)
Line range hint 11-28: LGTM! Well-structured test utility function
The function is well-implemented with proper error handling, clear documentation, and consistent use of the testify/require package. The nil check before accessing CctxStatus fields prevents potential panics during test execution.
e2e/e2etests/test_erc20_deposit_refund.go (1)
65-67: Verify the impact of liquidity pool creation changes
The removal of createZetaERC20LiquidityPool function might affect other tests that previously used it.
Let's verify if there are any remaining references:
✅ Verification successful
No impact from liquidity pool creation changes
The search results show that the liquidity pool creation is now handled directly through AddLiquidityETH and AddLiquidityERC20 methods consistently across the codebase. There are no remaining references to the removed createZetaERC20LiquidityPool function, and all test files are using the same pattern as implemented in the modified code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed function
rg "createZetaERC20LiquidityPool" --type go
# Search for similar liquidity pool creation patterns that might need the same update
rg "AddLiquidityETH.*AddLiquidityERC20" --type go
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for liquidity-related patterns
rg "AddLiquidity" --type go -A 2
# Also search for any test files that might be affected
fd "test.*\.go$" --exec grep -l "Liquidity"
# Check for any ERC20-related pool operations
rg "ERC20.*Pool|Pool.*ERC20" --type go
Length of output: 7185
e2e/runner/evm.go (1)
63-64: LGTM: Consistent with other deposit methods
The increased deposit amount of 100e18 aligns with the amount used in DepositEther(), maintaining consistency across different asset types. This change should help resolve the occasional liquidity errors mentioned in the PR objectives.
Let's verify the consistency of deposit amounts across the codebase:
✅ Verification successful
The previous pattern was too specific. Let's broaden our search to find deposit-related functions and their amount handling, which will help us verify the consistency of deposit amounts across the codebase.
The previous search results were too broad. Let's focus specifically on deposit amount definitions and related functions to verify the consistency of the 100e18 amount.
Deposit amount of 100e18 is consistently used across deposit methods
The search results confirm that both DepositEther() and DepositERC20() methods in e2e/runner/evm.go use the same deposit amount of 100e18:
// DepositEther
amount := big.NewInt(0).Mul(big.NewInt(1e18), big.NewInt(100)) // 100 eth
// DepositERC20
oneHundred := big.NewInt(0).Mul(big.NewInt(1e18), big.NewInt(100))This consistent amount helps maintain parity between different asset deposits and aligns with the PR's goal of resolving liquidity errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other deposit amount definitions
# Look for patterns like big.NewInt(100) or similar deposit amount definitions
rg -U 'big\.NewInt\(.*\).*\*.*big\.NewInt\(.*1e18.*\)' --type go
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to find deposit amounts
# Search for deposit-related functions and their amount parameters
rg -U "Deposit.*Amount|Deposit.*\(" --type go -A 5
# Also search for big.NewInt operations near deposit-related code
rg -U "(?s)Deposit.*(\n.*){0,10}big\.NewInt" --type go
Length of output: 106308
Script:
#!/bin/bash
# Let's search for deposit amount definitions in the codebase
rg -U "DepositEther|DepositERC20|oneHundred.*1e18" --type go -A 5
# Also search for other deposit amount constants
rg -U "const.*deposit.*amount|var.*deposit.*amount" --type go -A 5
Length of output: 80022
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3199 +/- ##
========================================
Coverage 62.74% 62.74%
========================================
Files 425 425
Lines 30227 30231 +4
========================================
+ Hits 18965 18969 +4
Misses 10425 10425
Partials 837 837
|
* increase erc20 values * consolidate function call * lint * add eth liquidity * increase deposit amount
Description
Summary by CodeRabbit
New Features
Bug Fixes
Documentation