test: e2e add withdraw emissions
#3151
Conversation
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
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📝 WalkthroughWalkthroughThe pull request introduces updates across several configuration and script files to enhance account management related to emissions withdrawal. A new account named 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
changed the title
kingpinXD
changed the title
e2e add withdraw emissions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3151 +/- ##
========================================
Coverage 62.64% 62.64%
========================================
Files 424 424
Lines 30115 30116 +1
========================================
+ Hits 18866 18867 +1
Misses 10408 10408
Partials 841 841
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
pkg/rpc/clients.go (1)
55-56: Maintain consistency in field documentationThe comment format should match the established pattern used for other fields.
Apply this change to maintain consistency:
- // EmissionsClient is a github.com/zeta-chain/zetacore/x/emissions/types QueryClient + // Emissions is a github.com/zeta-chain/zetacore/x/emissions/types QueryCliente2e/utils/zetacore.go (1)
19-22: Consider grouping related constants together.The constants look good and follow consistent naming conventions. However, consider grouping policy-related constants separately from other constants for better organization.
Consider reorganizing the constants like this:
const ( + // Policy names EmergencyPolicyName = "emergency" AdminPolicyName = "admin" OperationalPolicyName = "operational" UserEmissionsWithdrawName = "emissions_withdraw" + + // Timeouts + DefaultCctxTimeout = 8 * time.Minute )contrib/localnet/orchestrator/start-zetae2e.sh (2)
Line range hint
1-139: Consider enhancing script security and error handlingThe script could benefit from some security and maintainability improvements:
- Add error handling for the
fund_eth_from_configcalls- Set safe shell options at the start
Apply this diff to enhance script security:
#!/bin/bash +# Exit on error, undefined vars, and pipe failures +set -euo pipefail +# For debugging +set -x + # shellcheck disable=SC2317Also, consider wrapping the funding calls with error handling:
-fund_eth_from_config '.additional_accounts.user_emissions_withdraw.evm_address' 10000 "emissions withdraw tester" +if ! fund_eth_from_config '.additional_accounts.user_emissions_withdraw.evm_address' 10000 "emissions withdraw tester"; then + echo "Failed to fund emissions withdraw tester account" + exit 1 +fi
Line range hint
28-55: Enhance the fund_eth function with input validationThe
fund_ethfunction should validate its inputs to prevent potential issues with malformed addresses or negative amounts.Consider this enhanced implementation:
fund_eth() { local address=$1 local ether=$2 local comment=${3:-} + # Validate ethereum address format + if [[ ! "$address" =~ ^0x[a-fA-F0-9]{40}$ ]]; then + echo "Error: Invalid Ethereum address format: $address" + return 1 + fi + + # Validate ether amount is positive + if ! [[ "$ether" =~ ^[0-9]+$ ]] || [ "$ether" -le 0 ]; then + echo "Error: Invalid ether amount: $ether" + return 1 + fi + if [ -z "$comment" ]; then echo "funding eth address $address with $ether eth" else echo "funding eth address $address ($comment) with $ether eth" fie2e/runner/runner.go (1)
Missing e2e test implementation for emissions withdrawal functionality
The codebase has the necessary infrastructure for emissions withdrawal (helper functions, client setup, etc.) but lacks actual e2e test implementation. This is evidenced by:
- Presence of emissions-related helper functions in
e2e/runner/emissions.go- Recent commit "1c77e10 add withdraw emissions to e2e"
- No matching test file in
e2e/e2etests/directory for emissions withdrawal functionalityRequired actions:
- Add a new test file
e2e/e2etests/test_emissions_withdrawal.goimplementing comprehensive test scenarios- Include test cases for both successful and failed withdrawal scenarios
- Verify withdrawal amounts and balance changes
- Test authorization and permission scenarios
🔗 Analysis chain
Line range hint
102-214: Verify test coverage for emissions functionalityWhile the EmissionsClient is properly integrated, ensure that comprehensive e2e tests are implemented to verify the emissions withdrawal functionality mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for emissions-related test implementations # Search for emissions-related test files and functions echo "Searching for emissions test implementations..." rg -t go -l "emissions.*test" e2e/ rg -t go "func.*(Test|test).*[Ee]missions" e2e/ # Check for emissions-related test helper functions echo "Searching for emissions test helper functions..." ast-grep --pattern 'func $_(runner *E2ERunner) $_($$$) { $$$ $_.EmissionsClient.$$$ $$$ }'Length of output: 397
Script:
#!/bin/bash # Let's search more broadly for any emissions-related code and tests echo "Searching for emissions-related code..." rg -g '*.go' -B 2 -A 2 'emissions' e2e/ # Look for any test files that might contain emissions testing echo "Searching for test files in e2e..." fd -e go -e test test e2e/ # Check for any pending or recently added emissions test files echo "Checking git status for emissions-related files..." git status | grep -i emissions # Look for emissions-related changes in recent commits echo "Checking recent commits for emissions-related changes..." git log --grep="emissions" --since="1 month ago" --onelineLength of output: 15599
contrib/localnet/scripts/start-zetacored.sh (1)
317-319: Validate genesis account balanceConsider adding validation for the balance amount to prevent potential issues with extremely large or small values. Also, consider extracting the balance amount to a configuration variable for better maintainability.
+# Define standard genesis account balance +GENESIS_ACCOUNT_BALANCE="100000000000000000000000000azeta" + +# Validate balance format +validate_balance() { + local balance=$1 + if [[ ! "$balance" =~ ^[0-9]+azeta$ ]]; then + echo "Error: Invalid balance format: $balance" + return 1 + fi +} + # emissions withdraw tester address=$(yq -r '.additional_accounts.user_emissions_withdraw.bech32_address' /root/config.yml) -zetacored add-genesis-account "$address" 100000000000000000000000000azeta +validate_balance "$GENESIS_ACCOUNT_BALANCE" && \ +zetacored add-genesis-account "$address" "$GENESIS_ACCOUNT_BALANCE"e2e/runner/emissions.go (4)
35-35: Use Structured Logging for ClarityThe logging statement uses
r.Logger.Print, which may not provide structured logging capabilities. To enhance log clarity and facilitate log parsing, consider using structured logging methods.Refactor the logging statement to include structured fields:
-r.Logger.Print("🏃 withdrawing emissions from the emissions pool on ZetaChain for observer %s", observer) +r.Logger.With( + "observer", observer, +).Info("Withdrawing emissions from the emissions pool on ZetaChain.")This provides better log management and easier debugging.
69-72: Handle Zero Emissions Without LoggingLogging a message for observers with zero emissions may clutter the logs if many observers have no emissions. Consider omitting the log statement for zero emissions to keep logs concise.
You may simply continue without logging:
-if amountInt.IsZero() { - r.Logger.Print("no emissions to withdraw for observer %s", observer) +if amountInt.IsZero() { continue }
74-77: Propagate Error Context inWithdrawAllEmissionsWhen
WithdrawAllEmissionsreturns an error, additional context can help diagnose issues. Wrap the error with contextual information.Modify the error handling as follows:
-if err = r.ZetaTxServer.WithdrawAllEmissions(amountInt, e2eutils.UserEmissionsWithdrawName, observer); err != nil { - return err +if err := r.ZetaTxServer.WithdrawAllEmissions(amountInt, e2eutils.UserEmissionsWithdrawName, observer); err != nil { + return fmt.Errorf("failed to withdraw emissions for observer %s: %w", observer, err) }This provides clarity on which observer's emission withdrawal failed.
79-82: Consistent Error Wrapping with%wVerbIn the error message, use the
%wverb for wrapping errors to maintain consistency with error handling best practices.Update the error formatting:
-if err != nil { - return errors.Wrapf(err, "failed to get balance for observer after withdrawing emissions %s", observer) +if err != nil { + return fmt.Errorf("failed to get balance for observer %s after withdrawing emissions: %w", observer, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
cmd/zetae2e/config/local.yml(1 hunks)cmd/zetae2e/config/localnet.yml(1 hunks)cmd/zetae2e/local/local.go(2 hunks)contrib/localnet/orchestrator/start-zetae2e.sh(1 hunks)contrib/localnet/scripts/start-zetacored.sh(4 hunks)e2e/config/config.go(3 hunks)e2e/runner/emissions.go(1 hunks)e2e/runner/runner.go(3 hunks)e2e/runner/setup_zeta.go(0 hunks)e2e/txserver/zeta_tx_server.go(2 hunks)e2e/utils/zetacore.go(1 hunks)pkg/rpc/clients.go(3 hunks)
💤 Files with no reviewable changes (1)
- e2e/runner/setup_zeta.go
🧰 Additional context used
📓 Path-based instructions (9)
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.
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/emissions.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/runner.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/txserver/zeta_tx_server.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/utils/zetacore.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/rpc/clients.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
71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
cmd/zetae2e/config/localnet.yml
71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (16)
pkg/rpc/clients.go (3)
20-20: LGTM: Import follows Go conventions
The new import is correctly placed and follows the established pattern for internal package imports.
85-85: LGTM: Client initialization is consistent
The emissions client is properly initialized and correctly grouped under ZetaCore specific clients.
55-56: Verify emissions client capabilities
Let's ensure the emissions client provides all necessary query methods for the withdrawal functionality mentioned in the PR objectives.
Also applies to: 85-85
✅ Verification successful
Let me gather more specific information about the emissions query methods.
Let me search for the emissions proto definitions more specifically.
Emissions client provides all necessary query methods
Based on the proto definitions, the emissions client provides all required query methods for withdrawal functionality:
Params: For querying module parametersListPoolAddresses: For retrieving emission pool addressesShowAvailableEmissions: For checking withdrawable emission amounts
The client implementation aligns with the withdrawal functionality mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the emissions client has all required query methods
# Search for query method definitions in the emissions types
ast-grep --pattern 'rpc $_ ($_) returns ($_) {
option (google.api.http).$$$
}' | grep -A 5 'x/emissions/types'
# Search for any existing usage patterns
rg -A 3 'Emissions\.'
Length of output: 2633
Script:
#!/bin/bash
# Search for query service definition in proto files
fd '\.proto$' | xargs grep -l "service Query" | xargs cat
# Search for query client usage in the codebase
rg "Emissions\." -A 5 -g '!*.pb.go' -g '!*.pb.gw.go'
Length of output: 35153
Script:
#!/bin/bash
# Search for emissions proto file
fd '\.proto$' | xargs grep -l "package.*emissions" | xargs cat
# Search for emissions client usage in go files
rg "Emissions\." -g '*.go' -A 3
Length of output: 8658
cmd/zetae2e/config/localnet.yml (2)
68-71: LGTM - Configuration follows established patterns
The new account configuration is well-structured and consistent with other test accounts in the file. The credentials are appropriately scoped for the e2e testing environment.
🧰 Tools
🪛 Gitleaks
71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
68-71: Verify account configuration consistency
The new user_emissions_withdraw account configuration follows the established pattern. Let's verify its integration with other components:
✅ Verification successful
Account configuration verified and properly integrated
The user_emissions_withdraw account configuration is correctly implemented and consistent across the codebase:
- Present in both
localnet.ymlandlocal.ymlwith identical configuration - Properly defined in the configuration struct (
e2e/config/config.go) - Follows the established pattern of other test accounts
- Valid bech32 address format confirmed
- Consistent with the e2e testing infrastructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of the new account configuration across related files
# Check if the account is properly referenced in the e2e test setup
rg -l "user_emissions_withdraw" --type yaml
rg -l "0x9945724EBc9B3B9cc724abedE379349EE5E1657a" --type go
# Verify the bech32 address format
echo "zeta1n9zhyn4unvaee3ey40k7x7f5nmj7zet6qr5kl7" | gawk '/^zeta1[a-zA-Z0-9]{38}$/ {print "Valid bech32 address"}'
Length of output: 302
Script:
#!/bin/bash
# Let's examine the content of local.yml to ensure configuration consistency
cat cmd/zetae2e/config/local.yml
# Check for any e2e tests that might be using this account
rg "user_emissions_withdraw" --type go -A 3
Length of output: 6859
🧰 Tools
🪛 Gitleaks
71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
cmd/zetae2e/config/local.yml (1)
68-71: LGTM! Verify address formats for consistency.
The new user_emissions_withdraw account follows the established pattern and aligns with the PR's objective for e2e testing of emissions withdrawal functionality.
✅ Verification successful
Address formats verified successfully
All address formats in the new user_emissions_withdraw account configuration follow the expected patterns:
- Bech32 address matches
zeta1prefix with correct length - EVM address is a valid 40-character hex string with
0xprefix - Private key is a valid 64-character hex string
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify address format consistency
# Verify bech32 address format
echo "zeta1n9zhyn4unvaee3ey40k7x7f5nmj7zet6qr5kl7" | grep -E "^zeta1[a-zA-Z0-9]{38}$"
# Verify EVM address format (should be 40 hex chars)
echo "0x9945724EBc9B3B9cc724abedE379349EE5E1657a" | grep -E "^0x[a-fA-F0-9]{40}$"
# Verify private key format (should be 64 hex chars)
echo "9d524fe318c0eb5f80d8b246993a9f15f924db24d4b8b873839b13bc30040d03" | grep -E "^[a-fA-F0-9]{64}$"
Length of output: 421
🧰 Tools
🪛 Gitleaks
71-71: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
contrib/localnet/orchestrator/start-zetae2e.sh (1)
137-139: LGTM: Emissions withdraw account funding follows established patterns
The addition follows the consistent pattern used throughout the script for funding test accounts, maintaining the same funding amount and config key structure.
e2e/config/config.go (3)
64-79: LGTM! Clean addition of UserEmissionsWithdraw account.
The new account field is well-structured and follows the established pattern, with proper YAML tagging for configuration management.
252-252: LGTM! Proper integration in AsSlice method.
The new account is correctly added to the slice, maintaining consistency with the existing implementation pattern.
369-372: LGTM! Consistent implementation in GenerateKeys.
The account generation and error handling follow the established pattern, ensuring reliable key generation for the emissions withdrawal functionality.
e2e/runner/runner.go (3)
47-47: LGTM: Import statement follows conventions
The emissions types import is properly grouped with other internal package imports and follows consistent aliasing patterns.
102-102: LGTM: Field addition follows struct organization
The EmissionsClient field is properly typed and consistently placed with other client fields in the E2ERunner struct.
214-214: LGTM: Client initialization follows constructor pattern
The EmissionsClient is properly initialized from the clients parameter, maintaining consistency with other client initializations in the constructor.
cmd/zetae2e/local/local.go (2)
163-168: LGTM: Policy configuration looks good!
The addition of UserEmissionsWithdrawName and its corresponding private key follows the established pattern and maintains consistency with the existing policy configuration.
Also applies to: 173-173
501-502: Verify emissions withdrawal timing and dependencies.
The placement of emissions withdrawal needs verification to ensure it doesn't affect:
- Subsequent TSS migration tests (if enabled)
- Network report validation
- Zero balance checks on restricted addresses
✅ Verification successful
Let me gather more context about the withdrawal implementation and its dependencies.
Emissions withdrawal placement is correctly positioned
The current placement of emissions withdrawal is optimal as it:
- Executes after all core tests are completed, ensuring no interference with test scenarios
- Runs before the optional TSS migration test, which has no emissions balance dependencies
- Precedes the final balance validations, allowing proper verification of:
- Zero balance checks on restricted addresses
- Network report validation that explicitly verifies emissions pool balance reduction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any dependencies on emissions balance in tests and subsequent operations
# Check for emissions balance usage in test files
rg -A 5 "emissions" "e2e/e2etests/"
# Check for emissions balance usage in TSS migration
rg -A 5 "emissions" -g "**/*tss*.go"
# Check for emissions balance in network report validation
rg -A 5 "emissions" -g "**/report*.go"
Length of output: 2065
Script:
#!/bin/bash
# Check the implementation of WithdrawEmissions
ast-grep --pattern 'func $_ WithdrawEmissions() $_ {
$$$
}'
# Check for any balance checks or validations after emissions withdrawal
rg -B 5 -A 10 "WithdrawEmissions" cmd/zetae2e/local/local.go
# Check for any TSS migration dependencies
rg -B 5 -A 10 "TSSMigration" cmd/zetae2e/local/local.go
Length of output: 3556
e2e/txserver/zeta_tx_server.go (1)
13-13: LGTM: Import addition is well-structured
The new import for sdkmath is correctly aliased and appropriately placed within the import block.
| changeInBalance, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
It is difficult to check the emissions pool balance or available emissions here, as there can be some emissions in the blocks in between the tx's.
There was a problem hiding this comment.
Why not withdraw all emissions by default? No need to specify a certain amount.
That's how the commission withdrawal works for validators and I can't imagine a situation in which someone needs to withdraw some but not all emission rewards.
There was a problem hiding this comment.
We are withdrawing all the available emissions.
But there might be emissions added after we withdraw, so available emissions would not be zero, similar reason for emissions pool balance as well
There was a problem hiding this comment.
We can update the emissions params to set percentage to 0 before withdrawing.
So no emission happens after
There was a problem hiding this comment.
I considered that, but we would have to unset it after the check so that tss migration and upgrade tests are successful,
It seemed to me a bit unnecessary to do that just for this check, considering we also have unit tests .
Happy to add it though , if needed . Its trivial effort
| @@ -65,6 +65,10 @@ additional_accounts: | |||
| bech32_address: "zeta1nry9yeg6njhjrp2ctppa8558vqxal9fxk69zxg" | |||
| evm_address: "0x98c852651A9CAF2185585843d3D287600Ddf9526" | |||
| private_key: "bf9456c679bb5a952a9a137fcfc920e0413efdb97c36de1e57455763084230cb" | |||
| user_emissions_withdraw: | |||
There was a problem hiding this comment.
Isn't one of the observer? I would rather call it user_observer (or observer_hot_key to be more precise), might be used for other purpose in the future
There was a problem hiding this comment.
No, it's not an observer; it's a new account used just for withdrawal.
This tries to simulate the mainnet system in which all our validators grant authorization to an address, which then withdraws on their behalf
| @@ -67,6 +67,55 @@ add_v17_message_authorizations() { | |||
| ' $json_file > temp.json && mv temp.json $json_file | |||
| } | |||
|
|
|||
|
|
|||
| add_emissions_withdraw_authorizations() { | |||
There was a problem hiding this comment.
Small note here: any alternative that could work in Go?
Ideally we want to reduce the shell codebase and have everything in Go
There was a problem hiding this comment.
Okay, we can just use the same logic in Go a function ,
or a command similar to add-observer-list
But I guess it's okay for now since we are adding just one authorization for now. If we need to modify more items in the genesis file, we can consider baking that into Zetacored in the future
| err = r.ZetaTxServer.WithdrawAllEmissions(availableCoin.Amount, e2eutils.UserEmissionsWithdrawName, observer) | ||
| if err != nil { |
There was a problem hiding this comment.
| err = r.ZetaTxServer.WithdrawAllEmissions(availableCoin.Amount, e2eutils.UserEmissionsWithdrawName, observer) | |
| if err != nil { | |
| if err := r.ZetaTxServer.WithdrawAllEmissions(availableCoin.Amount, e2eutils.UserEmissionsWithdrawName, observer); err != nil { |
| changeInBalance, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
We can update the emissions params to set percentage to 0 before withdrawing.
So no emission happens after
Description
This pr adds withdrawal of emissions to the e2e .
The withdrawal uses the following mechanism.
UserEmissionsWithdraw, this is done to try to simulate the mainnet/testnet govops process as much as possibleUserEmissionsWithdrawwithdraws all available emissions by signing the withdraw tx.Closes : #3146
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores