modify genesis scripts to take rewards vester account balance as an argument and count it towards total supply #525
Conversation
…rgument and count it towards total supply
WalkthroughThe changes primarily involve the addition of a new variable Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (5)
- protocol/testing/genesis.sh (3 hunks)
- protocol/testing/testnet-dev/dev.sh (1 hunks)
- protocol/testing/testnet-external/pregenesis.sh (1 hunks)
- protocol/testing/testnet-local/local.sh (1 hunks)
- protocol/testing/testnet-staging/staging.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testing/testnet-staging/staging.sh
Additional comments (Suppressed): 6
protocol/testing/testnet-external/pregenesis.sh (1)
- 188-192: The
edit_genesisfunction call has been updated with an additional empty string argument. Ensure that the function definition ofedit_genesishas been updated to handle this new argument and that it doesn't break existing functionality.protocol/testing/testnet-dev/dev.sh (1)
- 134-137: The
edit_genesisfunction call has been updated with two additional empty string arguments. Ensure that the function definition ofedit_genesishas been updated to handle these new arguments and that all calls to this function throughout the codebase have been updated to match the new signature.protocol/testing/genesis.sh (3)
58-65: The new
REWARDS_VESTER_ACCOUNT_BALANCEvariable is introduced and defaults to 1e12 if not provided. Ensure that this default value doesn't cause any unexpected behavior in the system.976-976: The calculation of
bridge_module_account_balancenow subtractsREWARDS_VESTER_ACCOUNT_BALANCEfromTOTAL_NATIVE_TOKEN_SUPPLY. This change could affect other parts of the system that rely on the bridge module account balance. Verify that these changes are propagated correctly throughout the system.1008-1011: The rewards vester account balance is now set to the value of
REWARDS_VESTER_ACCOUNT_BALANCEinstead of a hardcoded value. This change should provide more flexibility, but ensure that it doesn't introduce any inconsistencies in the system.protocol/testing/testnet-local/local.sh (1)
- 99-103: The
edit_genesisfunction call has been updated with three additional empty string arguments. Ensure that the function definition ofedit_genesishas been updated to handle these new parameters and that they are used correctly within the function. Also, verify that all calls toedit_genesisthroughout the codebase have been updated to match the new signature.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- protocol/testing/genesis.sh (3 hunks)
Additional comments (Suppressed): 3
protocol/testing/genesis.sh (3)
64-68: The new hunk introduces a variable
REWARDS_VESTER_ACCOUNT_BALANCEwhich is set to the 7th argument passed to the script. If this argument is not provided, it defaults to 10 million full coins. Ensure that all calls to this script throughout the codebase have been updated to match the new argument structure.979-979: The bridge module account balance is now initialized as the total native token supply minus the rewards vester account balance. This is a change from the previous behavior where the bridge module account balance was set to the total native token supply. This change could have implications on the total supply of tokens in the system. Make sure this change is intended and that it doesn't negatively impact the system's token economics.
1014-1014: The rewards vester account balance is now set to the value of
REWARDS_VESTER_ACCOUNT_BALANCEinstead of a hardcoded value of 10 million full coins. This change allows for more flexibility in setting the rewards vester account balance. Ensure that this change is reflected in all places where the rewards vester account balance is used.
protocol/testing/genesis.sh
Outdated
| @@ -1004,7 +1011,7 @@ function edit_genesis() { | |||
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].address" -v "${REWARDS_VESTER_ACCOUNT_ADDR}" | |||
| dasel put -t json -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[]" -v "{}" | |||
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[0].denom" -v "${REWARD_TOKEN}" | |||
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[0].amount" -v "10000000$EIGHTEEN_ZEROS" # 10 million full coins | |||
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[0].amount" -v "$REWARDS_VESTER_ACCOUNT_BALANCE" | |||
There was a problem hiding this comment.
When REWARDS_VESTER_ACCOUNT_BALANCE is zero, we should skip line 1011 to 1015
protocol/testing/genesis.sh
Outdated
| @@ -969,7 +975,8 @@ function edit_genesis() { | |||
| dydx_exchange_config_json=$(cat "$EXCHANGE_CONFIG_JSON_DIR/dydx_exchange_config.json" | jq -c '.') | |||
| dasel put -t string -f "$GENESIS" '.app_state.prices.market_params.[34].exchange_config_json' -v "$dydx_exchange_config_json" | |||
|
|
|||
| bridge_module_account_balance=$TOTAL_NATIVE_TOKEN_SUPPLY | |||
| # Initialize bridge module account balance as total native token supply minus rewards vester account balance. | |||
| bridge_module_account_balance=$(echo "$TOTAL_NATIVE_TOKEN_SUPPLY - $REWARDS_VESTER_ACCOUNT_BALANCE" | bc) | |||
There was a problem hiding this comment.
Instead of initializing module account balance = total_supply - vester_balance, can we do decrement bridge_module_account_balance balance around line 1014, similar to how to decrement other type of balances?
| @@ -61,6 +61,12 @@ function edit_genesis() { | |||
| INITIAL_CLOB_PAIR_STATUS='STATUS_ACTIVE' | |||
| fi | |||
|
|
|||
| REWARDS_VESTER_ACCOUNT_BALANCE="$7" | |||
There was a problem hiding this comment.
Besides dev local and staging can you make sure https://github.com/dydxprotocol/v4-chain/blob/f50431e3a43a804fc9d4337d779bc054deffd7d9/protocol/scripts/genesis/prod_pregenesis.sh is also updated, and run this script to make sure the sample pregensis file is up-to-date?
There was a problem hiding this comment.
Updated prod_pregenesis,sh as well. When I run that script to check that sample file is up-to-date, the content is the same except that parameters have a different ordering, for example:
< "ticker": "ARB-USD",
< "id": 24,
< "market_id": 24,
< "liquidity_tier": 1
---
> "id": 24,
> "liquidity_tier": 1,
> "market_id": 24,
> "ticker": "ARB-USD"
There was a problem hiding this comment.
Can you rebase from main and try again? We recently made a change that fixes non-deterministic ordering in sample pregenesis
There was a problem hiding this comment.
merged main and ran again
Diffing output against current sample_pregenesis.json...
./scripts/genesis/sample_pregenesis.json is up-to-date
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (4)
- protocol/scripts/genesis/prod_pregenesis.sh (1 hunks)
- protocol/testing/containertest/containertest.sh (1 hunks)
- protocol/testing/genesis.sh (3 hunks)
- protocol/testing/testnet-external/pregenesis.sh (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/scripts/genesis/prod_pregenesis.sh
- protocol/testing/containertest/containertest.sh
Additional comments (Suppressed): 4
protocol/testing/testnet-external/pregenesis.sh (1)
- 182-182: The
edit_genesisfunction call has been updated with an additional empty string argument. Ensure that the function definition has been updated to handle this additional argument and that all other calls to this function throughout the codebase have been updated to match the new signature.protocol/testing/genesis.sh (3)
70-74: The new hunk introduces a variable
REWARDS_VESTER_ACCOUNT_BALANCEwhich is set to the 7th argument passed to the script. If this argument is not provided, it defaults to 10 million full coins. This change allows the rewards vester account balance to be taken as an argument, which is a good practice for flexibility and configurability. However, ensure that all calls to this script throughout the codebase have been updated to include this new argument.992-994: The bridge module account balance is initialized as the total native token supply. This is a logical step as the total supply should be distributed among different accounts. However, it's important to note that this balance will be adjusted later to subtract the
REWARDS_VESTER_ACCOUNT_BALANCEif it's greater than 0.1022-1032: The new hunk introduces logic to initialize the bank balance of the rewards vester account if
REWARDS_VESTER_ACCOUNT_BALANCEis greater than 0. It also adjusts thebridge_module_account_balanceby subtracting theREWARDS_VESTER_ACCOUNT_BALANCE. This is a good practice to ensure that the total supply remains constant. However, it's important to ensure that theREWARDS_VESTER_ACCOUNT_BALANCEis not greater than the total supply to avoid negative balances.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- protocol/testing/genesis.sh (3 hunks)
Additional comments (Suppressed): 3
protocol/testing/genesis.sh (3)
70-74: The new hunk introduces a variable
REWARDS_VESTER_ACCOUNT_BALANCEwhich is set to the 7th argument passed to the script. If this argument is not provided, it defaults to 10 million full coins. This is a good practice as it allows for flexibility in setting the rewards vester account balance while providing a sensible default.991-994: The bridge module account balance is now initialized as the total native token supply. This is a logical change as the bridge module account should hold all the native tokens initially.
1022-1032: The new hunk introduces a condition to check if
REWARDS_VESTER_ACCOUNT_BALANCEis greater than 0. If it is, the bank balance of the rewards vester account is initialized and the balance is deducted from the bridge module account balance. This is a sensible change as it ensures that the rewards vester account is only initialized if it has a balance and that the total supply of tokens remains constant.
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[0].denom" -v "${REWARD_TOKEN}" | ||
| dasel put -t string -f "$GENESIS" ".app_state.bank.balances.[$next_bank_idx].coins.[0].amount" -v "10000000$EIGHTEEN_ZEROS" # 10 million full coins | ||
| next_bank_idx=$(($next_bank_idx+1)) | ||
| if [ $(echo "$REWARDS_VESTER_ACCOUNT_BALANCE > 0" | bc -l) -eq 1 ]; then |
There was a problem hiding this comment.
Can you run localnet and check that localhost:26657/genesis contains the correct balance for reward vester account?
There was a problem hiding this comment.
confirmed:
{"address":"dydx1ltyc6y4skclzafvpznpt2qjwmfwgsndp458rmp","coins":[{"denom":"adv4tnt","amount":"200000000000000000000000000"}]}
protocol/testing/genesis.sh
Outdated
| REWARDS_VESTER_ACCOUNT_BALANCE="$7" | ||
| if [ -z "$REWARDS_VESTER_ACCOUNT_BALANCE" ]; then | ||
| # Default to 10 million full coins. | ||
| REWARDS_VESTER_ACCOUNT_BALANCE="10000000$EIGHTEEN_ZEROS" |
There was a problem hiding this comment.
Can you change this to 200 million full coins from realism?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/scripts/genesis/prod_pregenesis.sh (1 hunks)
- protocol/testing/genesis.sh (3 hunks)
- protocol/testing/testnet-external/pregenesis.sh (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/scripts/genesis/prod_pregenesis.sh
- protocol/testing/testnet-external/pregenesis.sh
Additional comments: 2
protocol/testing/genesis.sh (2)
70-74: The new hunk introduces a variable
REWARDS_VESTER_ACCOUNT_BALANCEwhich is set to the 7th argument passed to the script. If this argument is not provided, it defaults to 200 million full coins. This is a change from the old hunk where the rewards vester account balance was hardcoded to 10 million full coins. Ensure that all calls to this script throughout the codebase have been updated to pass the correct argument for the rewards vester account balance.1024-1034: The new hunk introduces a condition to check if
REWARDS_VESTER_ACCOUNT_BALANCEis greater than 0. If it is, the bank balance of the rewards vester account is initialized and thebridge_module_account_balanceis updated by subtracting theREWARDS_VESTER_ACCOUNT_BALANCE. This is a change from the old hunk where the bank balance of the rewards vester account was always initialized, regardless of theREWARDS_VESTER_ACCOUNT_BALANCE. This change seems to be in line with the intention to make the rewards vester account balance configurable.
Test Plan
Tested by running localnet and verifying bank total supply, bridge module account balance, and rewards vester account balance