feat: Restrict running vats to 5 #257
Conversation
|
Review changes with Changed Files
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @usmanmani1122, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new feature to limit the number of concurrently running vats to five, a crucial step for resource management. Beyond this feature, it includes a substantial refactoring of the test preparation scripts, centralizing configuration parameters and standardizing file path handling to improve overall maintainability and robustness.
Highlights
- Vat Concurrency Restriction: Implemented the core feature of restricting the number of concurrently running vats to a maximum of 5. This is achieved by adding or updating the
max-vats-onlinesetting within theapp.tomlconfiguration file. - Script Parameterization and Centralization: Introduced a new
source.shfile to centralize and parameterize common configuration values. This includes variables forCHAIN_ID,MONIKER,MAX_RUNNING_VATS,VOTING_PERIOD, and standardized paths to configuration files, improving script flexibility and reducing hardcoding. - Test Script Refactoring: The
run_prepare_zero.shscript was significantly refactored to leverage the new environment variables defined insource.sh. This includes updatingagd init,agd gentx,sed, andjqcommands to use these parameterized values, enhancing the script's readability, maintainability, and robustness.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
| voting_period_s=10 | ||
| latest_height=$(agd status | jq -r .SyncInfo.latest_block_height) | ||
| height=$((latest_height + voting_period_s + 20)) | ||
| height=$((latest_height + VOTING_PERIOD + 20)) |
There was a problem hiding this comment.
The script will fail at this line because $VOTING_PERIOD is "10s" (a string with a suffix), which is not valid in a bash arithmetic expression. This will cause an error like bash: 10s: value too great for base (error token is "10s").
To fix this, you should strip the s suffix from the variable before using it in the calculation. The jq command on line 72 requires the s suffix, so the variable itself should not be changed.
| height=$((latest_height + VOTING_PERIOD + 20)) | |
| height=$((latest_height + ${VOTING_PERIOD%s} + 20)) |
| --expression 's|^enable-unsafe-cors =.*|^enable-unsafe-cors = true|' \ | ||
| --expression 's|^enabled-unsafe-cors =.*|enabled-unsafe-cors = true|' \ | ||
| --expression 's|^pruning =.*|pruning = "nothing"|' \ | ||
| --expression '/^\[api]/,/^\[/{s|^enable =.*|enable = true|}' \ | ||
| --expression '/^[swingset]/a max-vats-online = 5' \ | ||
| --in-place | ||
| sed "$CONFIG_FILE_PATH" \ |
There was a problem hiding this comment.
The expression on line 44, which adds max-vats-online = 5, is redundant. The if/else block on lines 52-61 correctly handles adding or updating the max-vats-online configuration using the $MAX_RUNNING_VATS variable.
Having both is confusing and could lead to issues if the variable's value changes. The logic in lines 52-61 is sufficient. I recommend removing the redundant expression from this sed command.
| sed "$APP_CONFIG_FILE_PATH" \ | |
| --expression 's|^enable-unsafe-cors =.*|^enable-unsafe-cors = true|' \ | |
| --expression 's|^enabled-unsafe-cors =.*|enabled-unsafe-cors = true|' \ | |
| --expression 's|^pruning =.*|pruning = "nothing"|' \ | |
| --expression '/^\[api]/,/^\[/{s|^enable =.*|enable = true|}' \ | |
| --expression '/^[swingset]/a max-vats-online = 5' \ | |
| --in-place | |
| sed "$APP_CONFIG_FILE_PATH" \ | |
| --expression 's|^enable-unsafe-cors =.*|^enable-unsafe-cors = true|' \ | |
| --expression 's|^enabled-unsafe-cors =.*|enabled-unsafe-cors = true|' \ | |
| --expression 's|^pruning =.*|pruning = "nothing"|' \ | |
| --expression '/^\[api]/,/^\[/{s|^enable =.*|enable = true|}' \ | |
| --in-place |
| jq --arg denom "$BLD_DENOM" \ | ||
| --arg voting_period "$VOTING_PERIOD" \ | ||
| ' | ||
| .app_state.crisis.constant_fee.denom = $denom | | ||
| .app_state.mint.params.mint_denom = $denom | | ||
| .app_state.gov.deposit_params.min_deposit[0].denom = $denom | | ||
| .app_state.staking.params.bond_denom = $denom | | ||
| .app_state.slashing.params.signed_blocks_window = "20000" | | ||
| .app_state.gov.voting_params.voting_period = $voting_period | ||
| ' <"$GENESIS_FILE_PATH" | ||
| )" | ||
|
|
There was a problem hiding this comment.
The logic for setting voting_period has changed. The original code used a deep merge (* operator), which is more robust as it works even if parts of the JSON path (like voting_params) don't exist. The new implementation uses direct assignment, which will fail if .app_state.gov.voting_params is null or doesn't exist.
To retain the robustness of the original approach while keeping the single jq call, you could perform the deep merge for voting_period first, and then apply the other assignments. This preserves the special handling that voting_period had previously.
| contents="$( | |
| jq --arg denom "$BLD_DENOM" \ | |
| --arg voting_period "$VOTING_PERIOD" \ | |
| ' | |
| .app_state.crisis.constant_fee.denom = $denom | | |
| .app_state.mint.params.mint_denom = $denom | | |
| .app_state.gov.deposit_params.min_deposit[0].denom = $denom | | |
| .app_state.staking.params.bond_denom = $denom | | |
| .app_state.slashing.params.signed_blocks_window = "20000" | | |
| .app_state.gov.voting_params.voting_period = $voting_period | |
| ' <"$GENESIS_FILE_PATH" | |
| )" | |
| contents="$( | |
| jq --arg denom "$BLD_DENOM" \ | |
| --arg voting_period "$VOTING_PERIOD" \ | |
| '\ | |
| ( . * { app_state: { gov: { voting_params: { voting_period: $voting_period } } } } ) |\ | |
| .app_state.crisis.constant_fee.denom = $denom |\ | |
| .app_state.mint.params.mint_denom = $denom |\ | |
| .app_state.gov.deposit_params.min_deposit[0].denom = $denom |\ | |
| .app_state.staking.params.bond_denom = $denom |\ | |
| .app_state.slashing.params.signed_blocks_window = \"20000\"\ | |
| ' <"$GENESIS_FILE_PATH" | |
| )" |
image building process
TODO
@agoric/synthetic-chain library
TODO
tooling improvements
TODO