Problem: expedited related gov params are not adjusted #1623
Conversation
WalkthroughThe changes in this pull request encompass updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1623 +/- ##
==========================================
+ Coverage 35.86% 36.00% +0.13%
==========================================
Files 123 123
Lines 9755 9791 +36
==========================================
+ Hits 3499 3525 +26
- Misses 5845 5851 +6
- Partials 411 415 +4
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mmsqe
requested review from
JayT106 and
yihuang
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.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (13)
integration_tests/test_gov_update_params.py (3)
17-32: LGTM: Streamlined governance proposal submission process.The changes significantly simplify the process of submitting governance proposals and querying parameters. The use of
module_address("gov")for authority andsubmit_gov_proposalfunction improves code clarity and reduces potential errors.Consider adding a comment explaining the purpose of removing
merge_netsplit_blockandshanghai_timefrom the parameters, as it might not be immediately clear why these specific fields are being deleted.
53-65: LGTM: Consistent improvements in governance proposal submission.The changes in this function mirror the improvements made in
test_evm_update_param, leading to a more streamlined and consistent approach across the file.For consistency with the
test_evm_update_paramfunction, consider storing the result ofcronos.cosmos_cli().query_params()in a variable before assertion. This would make it easier to debug if the assertion fails, as you could print the actual values.Example:
queried_params = cronos.cosmos_cli().query_params() assert queried_params == params
Line range hint
1-65: Overall improvements in test implementation and clarity.The changes in this file significantly improve the implementation of governance-related tests. Key improvements include:
- Streamlined governance proposal submission process.
- More direct parameter querying and assertion.
- Consistent use of
module_address("gov")for authority.- Removal of unnecessary file I/O operations.
These changes align well with the PR objectives and maintain the core functionality of the tests while reducing complexity and potential points of failure.
Consider creating helper functions for common operations like parameter querying and assertion to further improve code reusability and maintainability across different test files.
integration_tests/cosmoscli.py (1)
Line range hint
1113-1123: LGTM! Consider adding type hints for improved readability.The changes to the
query_paramsmethod improve its flexibility by handling different response structures. This is a good improvement that makes the method more robust.Consider adding type hints to improve code readability:
def query_params(self, module: str = "cronos", **kwargs) -> dict: res = json.loads( self.raw( "query", module, "params", home=self.data_dir, **kwargs, ) ) return res.get("params") or resapp/upgrades.go (1)
61-95: Enhance testing for error handling inUpdateExpeditedParamsThe error handling code paths in
UpdateExpeditedParams(lines 64, 73, 77, 92) are currently not exercised by tests. To ensure that the function behaves correctly under error conditions, consider adding tests that simulate these scenarios.Let me know if you'd like help in creating these tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-64: app/upgrades.go#L64
Added line #L64 was not covered by tests
[warning] 73-73: app/upgrades.go#L73
Added line #L73 was not covered by tests
[warning] 77-77: app/upgrades.go#L77
Added line #L77 was not covered by tests
[warning] 92-92: app/upgrades.go#L92
Added line #L92 was not covered by testsapp/upgrades_test.go (2)
50-51: Use Constants for Magic NumbersIn line 50, a magic number is used for the
MinDepositamount:suite.govParams.MinDeposit = sdk.NewCoins(sdk.NewCoin(baseDenom, math.NewInt(2000000000000)))Using raw numerical literals can make the code less readable and harder to maintain.
Define a constant for the deposit amount to enhance readability and maintainability:
+ const minDepositAmount = 2_000_000_000_000 suite.govParams.MinDeposit = sdk.NewCoins(sdk.NewCoin(baseDenom, math.NewInt(minDepositAmount)))This clarifies the intent and makes future updates easier.
114-115: Error Handling ImprovementAt lines 114-115, errors are checked using
suite.Require().NoError, which will stop the test on failure.suite.Require().NoError(suite.app.GovKeeper.Params.Set(suite.ctx, suite.govParams)) suite.Require().NoError(app.UpdateExpeditedParams(suite.ctx, suite.app.GovKeeper))While this is acceptable, consider if using
suite.Assert().NoErrormight be more appropriate to allow the test to continue and report multiple failures in a single run.Evaluate whether switching to
suite.Assert()aligns better with your testing strategy.integration_tests/test_upgrade.py (4)
121-134: Parameterize hardcoded gas values incheck_basic_txThe function
check_basic_txuses hardcoded values formaxFeePerGasandmaxPriorityFeePerGas. While this may be acceptable for testing purposes, consider parameterizing these values or referencing predefined constants to improve flexibility and readability.
195-196: Handle potential exceptions fromdo_upgradebefore proceedingAfter calling
do_upgrade, the functioncheck_basic_tx(c)is executed without checking if the upgrade was successful. To prevent misleading test results, consider adding error handling to ensuredo_upgradecompletes successfully before proceeding with further checks.
Line range hint
204-213: Improve robustness of error handling during contract deploymentThe test expects a
ValueErrorwith a specific message when deploying theRandomcontract fails due to an "invalid memory address or nil pointer dereference". Relying on such a specific error message may not be robust across different environments or future changes.Consider handling the error more generally or updating the contract deployment to prevent such low-level exceptions. For example, you could check for any exception during deployment and assert that an error was raised without depending on the exact message.
289-292: Clarify the expected exception when queryingicaauthparametersThe code anticipates an
AssertionErrorwhen queryingicaauthparameters post-upgrade. If this behavior is intentional due to the parameters being unavailable or altered after the upgrade, consider adding a comment to explain why this exception is expected to aid future maintainability.integration_tests/utils.py (2)
13-13: Group import statements according to PEP 8 guidelinesThe import statement for
Decimalshould be grouped with other imports from the standard library to enhance readability and maintain consistency with PEP 8 style guidelines.Consider repositioning the import to group it with other standard library imports.
179-179: Use logging instead of print statementsUsing
loggingmodule provides more flexibility and control over logging levels and outputs.Replace the
-import print("check params have been updated now") +import logging +logging.info("Check params have been updated now")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- app/upgrades.go (3 hunks)
- app/upgrades_test.go (1 hunks)
- integration_tests/cosmoscli.py (2 hunks)
- integration_tests/gravity_utils.py (1 hunks)
- integration_tests/test_basic.py (3 hunks)
- integration_tests/test_gov_update_params.py (3 hunks)
- integration_tests/test_gravity_2.py (3 hunks)
- integration_tests/test_min_gas_price.py (1 hunks)
- integration_tests/test_upgrade.py (6 hunks)
- integration_tests/utils.py (3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/upgrades.go
[warning] 38-39: app/upgrades.go#L38-L39
Added lines #L38 - L39 were not covered by tests
[warning] 64-64: app/upgrades.go#L64
Added line #L64 was not covered by tests
[warning] 73-73: app/upgrades.go#L73
Added line #L73 was not covered by tests
[warning] 77-77: app/upgrades.go#L77
Added line #L77 was not covered by tests
[warning] 92-92: app/upgrades.go#L92
Added line #L92 was not covered by tests
🔇 Additional comments (20)
integration_tests/test_gov_update_params.py (2)
3-4: LGTM: Import statements updated appropriately.The changes in import statements align well with the modifications in the test functions. Unnecessary imports have been removed, and new required imports have been added.
Line range hint
8-8: LGTM: Consistent update of function signatures.The addition of the
tmp_pathparameter to both test functions is appropriate and consistent with the changes in their implementations. This update allows for better management of temporary resources during testing.Also applies to: 45-45
integration_tests/test_min_gas_price.py (2)
63-64: Simplification ofget_paramsfunction looks good, but verify impact.The simplification of the
get_paramsfunction improves code readability. However, please ensure this change doesn't affect the rest of the code that uses these parameters.
- Verify that
cli.query_params("feemarket")now returns the parameters directly without needing to access a "params" key.- Check if the removal of the dictionary comprehension (converting values to int/float) affects any calculations in
test_dynamic_fee_txandtest_base_fee_adjustment.- Consider adding a comment explaining this change, especially if it's part of a larger refactoring effort.
To verify the impact, please run the following script:
✅ Verification successful
Verification successful:
get_paramssimplification is correct and does not impact other code.The
get_paramsfunction is no longer used elsewhere intest_min_gas_price.py, and the current implementation does not introduce any type-related issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of get_params and potential type-related issues # Test: Search for get_params usage echo "Occurrences of get_params:" rg "get_params\(" integration_tests/test_min_gas_price.py # Test: Check for potential type-related issues echo "\nPotential type-related issues:" rg "params\[.+\]" integration_tests/test_min_gas_price.pyLength of output: 1105
63-64: Verify type consistency in test functions usingget_paramsThe change in
get_paramsmay affect thetest_dynamic_fee_txandtest_base_fee_adjustmentfunctions. Both these functions use the params returned byget_paramsin theadjust_base_feefunction, which performs integer arithmetic.Please ensure that:
- The parameters returned by
cli.query_params("feemarket")are of the correct type (integers) for use inadjust_base_fee.- If the parameters are not integers, add necessary type conversions either in
get_paramsor where the parameters are used.- Run the tests to confirm that no type-related errors occur during the calculations.
To verify the types used in calculations, please run the following script:
✅ Verification successful
Review Comment Not Applicable:
get_paramsStill Converts Parameters to IntegersThe
get_paramsfunction inintegration_tests/test_min_gas_price.pystill converts parameter values to integers usingint(float(v)), as shown below:params = cli.query_params("feemarket") return {k: int(float(v)) for k, v in params.items()}Since the integer conversion remains in place, the concerns about type consistency in the test functions do not apply at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check types used in adjust_base_fee calculations # Test: Examine adjust_base_fee function echo "adjust_base_fee function:" rg "def adjust_base_fee" -A 10 integration_tests/test_min_gas_price.py # Test: Check usage of params in test functions echo "\nUsage of params in test functions:" rg "params\[.+\]" integration_tests/test_min_gas_price.pyLength of output: 1105
Script:
#!/bin/bash # Description: Inspect types of parameters returned by cli.query_params("feemarket") # Extract the line where params are assigned rg "params\s*=\s*cli\.query_params" integration_tests/test_min_gas_price.py # Inspect the return statement to see if values are converted to integers rg "return\s*\{k:\s*int\(float\(v\)\)" integration_tests/test_min_gas_price.pyLength of output: 258
integration_tests/gravity_utils.py (1)
119-119: LGTM! Consider verifying API compatibility.The simplification of accessing
gravity_iddirectly from thequery_gravity_params()result is a good change. It makes the code more straightforward and easier to read.To ensure this change doesn't break compatibility with different versions of the API, please run the following verification script:
This script will help identify if there are any inconsistencies in how
query_gravity_params()is implemented across different versions or files in the project.✅ Verification successful
To address the issue with the previous script, please run the following verification script to accurately check the structure of
query_gravity_paramsacross different versions:[/run_scripts]
To ensure that the removal of the "params" key in
query_gravity_paramsdoes not impact other parts of the codebase, please run the following verification script:[/run_scripts]
✅ Verification Successful: No Compatibility Issues Found
The
gravity_idaccess has been updated correctly without affecting other parts of the codebase. No usages ofquery_gravity_params["params"]were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of gravity params across different versions # Test: Check the structure of gravity params in different versions rg --type python -A 5 'def query_gravity_params' | grep -v 'def query_gravity_params'Length of output: 118
Script:
#!/bin/bash # Description: Verify the structure of gravity params across different versions # Find all Python files and search for the 'query_gravity_params' function fd --type f --extension py | xargs grep -A 5 'def query_gravity_params'Length of output: 443
Script:
#!/bin/bash # Description: Find all definitions and usages of `query_gravity_params` across the codebase # Find all Python files python_files=$(fd --type f --extension py) # Search for the definition of `query_gravity_params` echo "=== Function Definitions ===" echo "$python_files" | xargs grep -H 'def query_gravity_params' # Search for usages where "params" key is accessed echo "" echo "=== Usages Accessing 'params' Key ===" echo "$python_files" | xargs grep -H 'query_gravity_params.*\["params"\]'Length of output: 8172
integration_tests/test_gravity_2.py (3)
316-316: LGTM: Simplified parameter accessThe change simplifies the access to the 'bridge_active' parameter, making the code more straightforward. This modification aligns with the overall goal of improving code readability while maintaining the same functionality.
335-335: LGTM: Consistent parameter accessThis change is consistent with the previous modification at line 316, maintaining a uniform approach to accessing the 'bridge_active' parameter. This consistency improves the overall readability and maintainability of the test function.
357-357: LGTM: Completed consistent parameter access patternThis change completes the pattern of simplified 'bridge_active' parameter access throughout the
test_gravity_detect_malicious_supplyfunction. It ensures consistency and improves the overall quality of the test implementation.CHANGELOG.md (5)
Line range hint
3-3: LGTM: Unreleased section is presentThe changelog includes an "UNRELEASED" section at the top, which is a good practice for tracking changes that will be included in the next release.
🧰 Tools
🪛 Markdownlint
27-27: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
5-8: State Machine Breaking change looks goodThe change related to memiavl's initial version logic is clearly described and includes a reference to the relevant pull request (#1618).
🧰 Tools
🪛 Markdownlint
27-27: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
10-17: Improvements section is well-documentedThe improvements section lists several changes with clear descriptions and references to their respective pull requests. This provides good traceability for the changes.
🧰 Tools
🪛 Markdownlint
27-27: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
19-25: Bug Fixes section is comprehensiveThe bug fixes are well-documented with clear descriptions and references to the relevant pull requests. The inclusion of CVE-2021-39137 hotfix is particularly important for security.
🧰 Tools
🪛 Markdownlint
27-27: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
27-61: v1.0.7 release notes are detailedThe release notes for v1.0.7 provide a good overview of the improvements and bug fixes included in this version. The inclusion of backported fixes and the release of static-linked binaries for the Linux platform are notable improvements.
🧰 Tools
🪛 Markdownlint
27-27: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
app/app.go (1)
957-957: Verify the impact of removingapp.IBCKeeper.ClientKeeperfromRegisterUpgradeHandlersThe
RegisterUpgradeHandlersmethod call has been modified to remove theapp.IBCKeeper.ClientKeeperparameter. This change could affect how IBC-related upgrades are handled. Please ensure that this modification is intentional and doesn't break any existing functionality related to IBC upgrades.To verify the impact of this change, please run the following script:
✅ Verification successful
[/run_scripts]
Removal of
app.IBCKeeper.ClientKeeperVerifiedThe
RegisterUpgradeHandlersmethod call has been successfully updated to remove theapp.IBCKeeper.ClientKeeperparameter. No additional references toIBCKeeper.ClientKeeperwere found in the codebase, indicating that this change is isolated and should not impact existing IBC upgrade functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to IBCKeeper.ClientKeeper in upgrade handlers # Search for references to IBCKeeper.ClientKeeper in upgrade-related files echo "Searching for IBCKeeper.ClientKeeper references in upgrade handlers:" rg --type go 'IBCKeeper.ClientKeeper' app/upgradesLength of output: 250
Script:
#!/bin/bash # Description: Recursively search for any references to IBCKeeper.ClientKeeper in the codebase echo "Recursively searching for IBCKeeper.ClientKeeper references in the entire codebase:" rg --type go 'IBCKeeper.ClientKeeper'Length of output: 214
app/upgrades.go (1)
20-20: Update function calls to match newRegisterUpgradeHandlerssignatureThe function
RegisterUpgradeHandlersno longer accepts theclientKeeperparameter. Please ensure that all invocations of this function have been updated to reflect the new signature to prevent build errors.You can verify by running:
✅ Verification successful
All calls to
RegisterUpgradeHandlershave been correctly updated to the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `RegisterUpgradeHandlers` and check that they only pass `cdc` as an argument. # Expected: All calls should match the new signature. rg --type go 'RegisterUpgradeHandlers\(' -A 1 -B 1Length of output: 339
integration_tests/test_upgrade.py (4)
115-118: Confirm the necessity of excludingheader_hash_numin parameter comparisonIn the
assert_evm_paramsfunction, theheader_hash_numfield is deleted from the parameters before performing the assertion. Please verify if excludingheader_hash_numis necessary for an accurate comparison. If so, consider adding a comment to explain why this field is omitted to aid future maintainability.
248-248: Verify the expected value ofmin_timeout_durationinicaauthparametersThe assertion checks if
min_timeout_durationequals"3600s". Ensure that this is the expected value after the upgrade. If the upgrade alters this parameter, consider updating the expected value or adding comments to clarify the intended behavior.
281-292: Clarify the usage ofgov_paramand validate governance parameters post-upgradeThe governance parameters are captured before the upgrade and asserted afterward using
assert_gov_params(cli, gov_param). Confirm that capturinggov_paramat this point reflects the intended state for post-upgrade comparison. Additionally, ensure thatassert_gov_paramscorrectly validates any changes or preservations of governance parameters during the upgrade process.
292-292: Ensureassert_gov_paramsis correctly imported and utilizedThe function
assert_gov_paramsis used to validate governance parameters, but make sure it is correctly imported and implemented. Verify that it functions as intended and that all necessary parameters are being checked.integration_tests/test_basic.py (1)
70-70: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid shadowing built-in function
typeand ensure correct field names.
Rename variable
typeto prevent shadowing the built-intypefunction.Assigning to
typeshadows Python's built-intype, which can lead to confusion or unexpected behavior. Consider renaming it tomsg_typefor clarity.Verify the correct field name in the message dictionary.
In the message, the key
"signer"is used, whereas in other messages,"authority"is used. Check if/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParamsexpects"authority"instead of"signer". Consistent use of field names helps maintain code readability and correctness.Apply the following diff to address these issues:
- type = "/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParams" + msg_type = "/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParams" submit_gov_proposal( cronos, tmp_path, messages=[ { - "@type": type, - "signer": authority, + "@type": msg_type, + "authority": authority, "params": p, } ], deposit="5basetcro", expedited=True, )Also applies to: 76-76
2 participants
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
to avoid invalid genesis ever since upgrade
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
Release Notes
New Features
Improvements
Bug Fixes
Documentation