chore: set lower tally params for localnet #3067
Conversation
gartnera
added
no-changelog
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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
|
gartnera
force-pushed
the
localnet-lower-tally-params
branch
from
f08f3e2
to
1353801
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
contrib/localnet/scripts/start-zetacored.sh (4)
Line range hint 39-52: Enhance SSH key generation security and error handling.
The current SSH key setup has several security and reliability concerns:
- Keys are generated with empty passphrase
- No validation of key copying
- Missing error handling for SSH operations
Consider implementing these security improvements:
if [ $HOSTNAME == "zetacore0" ]; then
if [[ ! -f ~/.ssh/id_rsa ]]; then
- ssh-keygen -t rsa -q -N "" -f ~/.ssh/id_rsa
- cp ~/.ssh/id_rsa.pub ~/.ssh/authorized_keys
- # keep localtest.pem for compatibility
- cp ~/.ssh/id_rsa ~/.ssh/localtest.pem
- chmod 600 ~/.ssh/*
+ if ! ssh-keygen -t ed25519 -q -N "" -f ~/.ssh/id_rsa; then
+ echo "Failed to generate SSH key" >&2
+ exit 1
+ fi
+ if ! cp ~/.ssh/id_rsa.pub ~/.ssh/authorized_keys; then
+ echo "Failed to copy public key to authorized_keys" >&2
+ exit 1
+ fi
+ # keep localtest.pem for compatibility
+ if ! cp ~/.ssh/id_rsa ~/.ssh/localtest.pem; then
+ echo "Failed to copy private key to localtest.pem" >&2
+ exit 1
+ fi
+ chmod 600 ~/.ssh/id_rsa ~/.ssh/localtest.pem
+ chmod 644 ~/.ssh/id_rsa.pub ~/.ssh/authorized_keys
fi
fiLine range hint 65-294: Improve maintainability by modularizing genesis creation steps.
The genesis creation logic is complex and would benefit from being broken down into functions for better maintainability and testing.
Consider restructuring the code into functions:
+# Initialize node configuration
+init_node_config() {
+ mkdir -p ~/.backup/config
+ zetacored init Zetanode-Localnet --chain-id=$CHAINID
+ rm -rf ~/.zetacored/config/{app,client,config}.toml
+ cp -r ~/zetacored/common/{app,client,config}.toml ~/.zetacored/config/
+ sed -i -e "/moniker =/s/=.*/= \"$HOSTNAME\"/" "$HOME"/.zetacored/config/config.toml
+}
+
+# Collect observer information from nodes
+collect_observer_info() {
+ for NODE in "${NODELIST[@]}"; do
+ INDEX=${NODE:0-1}
+ ssh zetaclient"$INDEX" mkdir -p ~/.zetacored/
+ while ! scp "$NODE":~/.zetacored/os_info/os.json ~/.zetacored/os_info/os_z"$INDEX".json; do
+ echo "Waiting for os_info.json from node $NODE"
+ sleep 1
+ done
+ scp ~/.zetacored/os_info/os_z"$INDEX".json zetaclient"$INDEX":~/.zetacored/os.json
+ done
+}Then use these functions in the main flow:
if [[ $HOSTNAME == "zetacore0" && ! -f ~/.zetacored/init_complete ]]; then
init_node_config
collect_observer_info
# ... rest of the logic
fi296-301: LGTM! Consider optimizing the jq commands.
The governance parameter updates align well with the PR objectives, enabling upgrade tests with more than two nodes by setting appropriate quorum and threshold values.
Consider combining all governance-related jq operations into a single command for better performance:
- jq '.app_state["gov"]["params"]["voting_period"]="100s" |
- .app_state["gov"]["params"]["quorum"]="0.1" |
- .app_state["gov"]["params"]["threshold"]="0.1"' \
- $HOME/.zetacored/config/genesis.json > tmp.json && mv tmp.json $HOME/.zetacored/config/genesis.json
+ jq '.app_state.gov.params |= . + {
+ "voting_period": "100s",
+ "quorum": "0.1",
+ "threshold": "0.1"
+ }' $HOME/.zetacored/config/genesis.json > tmp.json && mv tmp.json $HOME/.zetacored/config/genesis.jsonLine range hint 303-359: Enhance account setup with better balance management and reduced repetition.
The current implementation has several areas for improvement:
- Extremely large initial balances might pose risks
- Repetitive account funding code reduces maintainability
Consider implementing these improvements:
+# Define a reasonable initial balance
+INITIAL_BALANCE="1000000000000000000000azeta" # 1000 ZETA
+
+# Function to add account with balance
+add_account_with_balance() {
+ local address=$1
+ local balance=${2:-$INITIAL_BALANCE}
+ if ! zetacored add-genesis-account "$address" "$balance"; then
+ echo "Failed to add genesis account: $address" >&2
+ return 1
+ fi
+}
+
+# Read and fund accounts from config
+fund_accounts() {
+ local accounts=(
+ "default_account"
+ "additional_accounts.user_erc20"
+ "additional_accounts.user_zeta_test"
+ # ... add other accounts
+ )
+
+ for account in "${accounts[@]}"; do
+ address=$(yq -r ".$account.bech32_address" /root/config.yml)
+ add_account_with_balance "$address"
+ done
+}Then use this function instead of repetitive code:
fund_accounts📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/localnet/scripts/start-zetacored.sh (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
contrib/localnet/scripts/start-zetacored.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
Description
This will allow for running upgrade tests with more than two nodes. I want to do this in developnet (performance tests + automated upgrades).
References:
How Has This Been Tested?
Summary by CodeRabbit
New Features
quorumandthreshold.Documentation