Problem: no plan for testnet to update default max_callback_gas param #1252
Conversation
WalkthroughThe changes involve a new testnet plan Changes
TipsChat with CodeRabbit Bot (
|
Signed-off-by: mmsqe <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
- Coverage 35.79% 35.75% -0.04%
==========================================
Files 116 116
Lines 10651 10663 +12
==========================================
+ Hits 3812 3813 +1
- Misses 6464 6474 +10
- Partials 375 376 +1
|
yihuang
requested review from
calvinaco and
devashishdxt
and removed request for
a team
![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
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/upgrades.go (1 hunks)
- integration_tests/test_ica.py (4 hunks)
- integration_tests/test_upgrade.py (1 hunks)
- x/cronos/types/params.go (1 hunks)
Additional comments: 9
CHANGELOG.md (1)
- 20-20: The addition of the new plan
v1.1.0-testnetto update the defaultmax_callback_gasparameter is correctly documented in the CHANGELOG.md and aligns with the PR objectives and the AI-generated summaries.app/upgrades.go (3)
121-133: The addition of the
v1.1.0-testnetupgrade handler correctly implements the new plan to update themax_callback_gasparameter as per the PR objective.135-137: The code for reading upgrade information from disk and handling errors is correctly implemented and will panic if it fails to read the information, which is a standard approach for critical startup operations.
117-118: The change to enlarge the block gas limit is not mentioned in the PR objective or the AI-generated summaries. Ensure this change is intentional and documented appropriately.
integration_tests/test_ica.py (3)
36-81: The addition of
deploy_contractandcheck_for_ackfunctions, and modifications tosend_txare consistent with the summary provided. These changes introduce new functionality and modify existing behavior to accommodate the updatedmax_callback_gasparameter.98-104: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [100-125]
The
submit_msgsfunction has been modified to include atimeout_in_sparameter and atimeoutvariable to handle timeouts, which is not mentioned in the summary. This change is significant as it introduces new behavior for handling transaction timeouts.
- 123-128: The logic for reopening the ICA account after the channel gets closed is present in the code but not mentioned in the summary. This is an important aspect of the test that ensures the ICA account can be re-established after a channel closure.
integration_tests/test_upgrade.py (1)
- 180-182: The assertion for
max_callback_gashas been correctly updated to "50000" to reflect the new expected value as per the PR objectives and summary. However, there is an additional assertion formin_timeout_durationbeing "3600s" which is not mentioned in the summary. Please ensure that this change is intentional and documented if necessary.x/cronos/types/params.go (1)
- 28-28: The change in
MaxCallbackGasDefaultValuefrom300000to50000aligns with the PR objectives to update themax_callback_gasparameter on the testnet.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- app/upgrades.go (2 hunks)
- integration_tests/configs/cosmovisor.jsonnet (1 hunks)
- integration_tests/configs/upgrade-test-package.nix (1 hunks)
- integration_tests/cosmoscli.py (7 hunks)
- integration_tests/test_upgrade.py (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/upgrades.go
- integration_tests/test_upgrade.py
Additional comments: 8
integration_tests/configs/cosmovisor.jsonnet (1)
- 12-23: The summary states that the
paramsfield was removed frombank, but the hunk shows thatsend_enabledhas been added or modified withinbank. This is an inconsistency that should be clarified.integration_tests/configs/upgrade-test-package.nix (1)
- 10-16: The updates to the Nix expression file correctly reflect the new version and commit hash for the
v1.1.0-testnetupgrade plan as described in the PR objectives and AI-generated summaries.integration_tests/cosmoscli.py (6)
- 715-728: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [670-741]
The refactoring of the
gov_propose_legacymethod to include error handling based on the response code is a good practice, as it allows for more granular control over the function's output and error management.
- 715-728: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [670-741]
The refactoring of the
gov_propose_legacymethod to include error handling based on the response code is a good practice, as it allows for more granular control over the function's output and error management.
- 736-748: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [743-760]
The
gov_votemethod has been updated to include error handling similar to thegov_propose_legacymethod. This consistency in error handling across methods is beneficial for maintainability and reliability of the code.
- 736-748: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [743-760]
The
gov_votemethod has been updated to include error handling similar to thegov_propose_legacymethod. This consistency in error handling across methods is beneficial for maintainability and reliability of the code.
- 1147-1153: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1148-1164]
The
submit_gov_proposalmethod has been updated to include error handling based on the response code. This change aligns with the updates made to thegov_propose_legacyandgov_votemethods, ensuring a consistent approach to error handling across the class.
- 1147-1153: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1148-1164]
The
submit_gov_proposalmethod has been updated to include error handling based on the response code. This change aligns with the updates made to thegov_propose_legacyandgov_votemethods, ensuring a consistent approach to error handling across the class.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- app/upgrades.go (1 hunks)
- integration_tests/configs/cosmovisor.jsonnet (1 hunks)
- integration_tests/configs/cosmovisor_testnet.jsonnet (1 hunks)
- integration_tests/configs/default.jsonnet (1 hunks)
- integration_tests/configs/upgrade-test-package.nix (1 hunks)
- integration_tests/configs/upgrade-testnet-test-package.nix (1 hunks)
- integration_tests/cosmoscli.py (7 hunks)
- integration_tests/test_upgrade.py (1 hunks)
- integration_tests/test_upgrade_testnet.py (1 hunks)
- integration_tests/utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/upgrades.go
- integration_tests/configs/cosmovisor.jsonnet
- integration_tests/configs/upgrade-test-package.nix
- integration_tests/test_upgrade.py
Additional comments: 11
integration_tests/configs/cosmovisor_testnet.jsonnet (1)
- 1-33: The changes to the
cosmovisor_testnet.jsonnetfile reflect updates to thebankandfeemarketsections within theapp_stateof thegenesisconfiguration, as mentioned in the summary. However, the summary also mentions the removal of thegovsection withinapp_state, adjustments to the maximum gas limit for block validation, and updates to package paths and version references, which are not visible in the provided hunk. Please verify if these changes are part of other hunks or files.integration_tests/configs/default.jsonnet (2)
64-69: The change in the
max_gasparameter from81,500,000to60,000,000is correctly implemented in the hunk. This adjustment will affect the block validation process and should be communicated to the network participants.64-70: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-70]
No further issues or changes are detected in the provided hunk and surrounding context.
integration_tests/cosmoscli.py (4)
- 716-729: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [670-742]
The refactoring of the
gov_propose_legacymethod to handle different proposal types and conditionally process the response is a significant improvement in error handling and control flow. This change should make the method more robust and easier to maintain.
- 737-749: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [744-761]
The update to the
gov_votemethod to check the response code before querying the transaction event is a good practice for robust error handling.
- 1148-1154: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1149-1165]
The addition of default keyword arguments for gas prices and gas adjustment in the
submit_gov_proposalmethod is a good practice for providing sensible defaults.
- 1160-1168: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1167-1174]
The update to the
update_token_mappingmethod to check the response code before querying the transaction event is a good practice for robust error handling.integration_tests/test_upgrade_testnet.py (3)
77-197: The
test_cosmovisor_upgradefunction appears to be well-structured and follows the described upgrade testing process. It includes proposing an upgrade, stopping and starting services, waiting for blocks, deploying contracts, and checking various parameters post-upgrade. Ensure that the upgrade process is thoroughly tested and that all assertions accurately reflect the expected state of the system after the upgrade.25-53: The
init_cosmovisorandpost_initfunctions are utility functions that set up the cosmovisor directory structure and prepare cosmovisor for each node, respectively. They seem to be correctly implemented and serve their purpose without any apparent issues.56-74: The
custom_cronosfixture is responsible for setting up a custom Cronos test environment, which includes building the upgrade package and initializing with the genesis binary. It seems to be correctly implemented and serves its purpose without any apparent issues.integration_tests/utils.py (1)
- 145-151: The addition of the
event_query_txparameter to thegov_votemethod call within theapprove_proposalfunction is a significant change that alters the control flow. Ensure that all calls togov_voteare updated accordingly if this is a required change across the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- app/upgrades.go (1 hunks)
- integration_tests/configs/cosmovisor.jsonnet (1 hunks)
- integration_tests/configs/cosmovisor_testnet.jsonnet (1 hunks)
- integration_tests/configs/default.jsonnet (1 hunks)
- integration_tests/configs/upgrade-testnet-test-package.nix (1 hunks)
- integration_tests/cosmoscli.py (7 hunks)
- integration_tests/test_upgrade.py (3 hunks)
- integration_tests/test_upgrade_testnet.py (1 hunks)
- integration_tests/upgrade_utils.py (1 hunks)
- integration_tests/utils.py (1 hunks)
Files skipped from review due to trivial changes (1)
- integration_tests/configs/default.jsonnet
Files skipped from review as they are similar to previous changes (8)
- app/upgrades.go
- integration_tests/configs/cosmovisor.jsonnet
- integration_tests/configs/cosmovisor_testnet.jsonnet
- integration_tests/configs/upgrade-testnet-test-package.nix
- integration_tests/cosmoscli.py
- integration_tests/test_upgrade.py
- integration_tests/test_upgrade_testnet.py
- integration_tests/utils.py
Additional comments: 2
integration_tests/upgrade_utils.py (2)
8-15: The
init_cosmovisorfunction correctly sets up the cosmovisor directory structure.18-35: Verify that the hardcoded
chain_idis consistent with the rest of the codebase and that theconfig.jsonfile structure matches the expected format.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration_tests/test_upgrade.py (2 hunks)
- integration_tests/test_upgrade_testnet.py (1 hunks)
- integration_tests/upgrade_utils.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration_tests/test_upgrade.py
Additional comments: 7
integration_tests/test_upgrade_testnet.py (2)
10-28: The fixture
custom_cronosis correctly set up to build the test environment using Nix and to initialize the testnet with a genesis binary. Ensure that the paths and configurations specified are consistent with the new testnet setup.31-32: The
test_cosmovisor_upgradefunction appears to be correctly structured to test the upgrade process. Ensure that theexecfunction is properly implemented to handle the upgrade logic as intended.integration_tests/upgrade_utils.py (5)
1-189: The summary mentions changes to
MaxCallbackGasDefaultValueand the addition of anenlargeBlockGasLimitfunction, but these changes are not visible in the provided hunks. Please ensure that all relevant changes are included in the review to verify their correctness and impact.23-30: The
init_cosmovisorfunction correctly sets up the cosmovisor directory structure as described in the summary.1-189: The summary indicates updates to configuration files, but these changes are not visible in the provided hunks. Please ensure that all relevant changes are included in the review to verify their correctness and impact.
1-189: The summary describes significant changes to the
gov_propose_legacymethod in theintegration_tests/cosmoscli.pyfile, but this file is not included in the provided hunks. Please ensure that all relevant changes are included in the review to verify their correctness and impact.54-189: The
execfunction has been modified to handle the upgrade process, including proposing an upgrade, waiting for it to happen, and verifying the upgrade's success. Ensure that the logic within this function is robust and correctly implements the upgrade process as intended by the PR objectives.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- integration_tests/cosmoscli.py (8 hunks)
- integration_tests/test_broadcast.py (1 hunks)
- integration_tests/test_gov_update_params.py (1 hunks)
- integration_tests/test_upgrade.py (7 hunks)
- integration_tests/utils.py (3 hunks)
Files skipped from review due to trivial changes (1)
- integration_tests/test_broadcast.py
Files skipped from review as they are similar to previous changes (2)
- integration_tests/test_gov_update_params.py
- integration_tests/test_upgrade.py
Additional comments: 8
integration_tests/cosmoscli.py (5)
- 716-729: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [670-742]
The changes to the
gov_propose_legacymethod introduce new branches for handling different proposal types. Ensure that the logic for each branch correctly processes the proposal based on its kind and that therspvariable is correctly used to capture the response from theself.rawcalls.
- 737-749: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [744-761]
The
gov_votemethod now includes additional logic to check the response code and query the transaction event if the response code is 0. Verify that this new logic is intended and correctly implemented.
- 1124-1130: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1124-1150]
The
gov_propose_update_client_legacymethod has been updated to include default gas prices and amounts. Verify that these defaults are appropriate for the expected transactions and that the logic for handling the response is correct.
- 1145-1157: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1152-1168]
The
submit_gov_proposalmethod has been updated to include default gas prices and amounts. Verify that these defaults are appropriate for the expected transactions and that the logic for handling the response is correct.
- 1170-1171: The
update_token_mappingmethod has been updated to include default gas prices and amounts. Verify that these defaults are appropriate for the expected transactions and that the logic for handling the response is correct.integration_tests/utils.py (3)
136-136: The summary states that the
approve_proposalfunction'sevent_query_txparameter default value was changed fromTruetoFalse, but the provided hunk shows that the parameter's default value was alreadyFalse. This indicates a discrepancy between the summary and the actual code.652-652: The summary indicates that the
submit_any_proposalfunction had theevent_query_txparameter removed from its signature, but the provided hunk does not show the function signature or its call toapprove_proposal, so we cannot verify this change. We would need to see the previous version of the function to confirm this change.649-649: The function
module_addressis shown in the hunk, but there is no context provided in the summary about any changes to this function. Without additional context or a previous version of the code, it's unclear if any changes were made here.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
f97e655
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
v1.1.0-testnetto enhance system performance.Bug Fixes
max_callback_gasparameter to optimize resource usage.Documentation
Refactor
Tests
max_callback_gasparameter value.