Problem: submit-legacy-proposal doesn't work for rc5 #1699
Conversation
no upgrade-height related flags
mmsqe
requested review from
yihuang and
calvinaco
and removed request for
a team
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a new release version 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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1699 +/- ##
=======================================
Coverage 16.87% 16.88%
=======================================
Files 72 72
Lines 6163 6167 +4
=======================================
+ Hits 1040 1041 +1
- Misses 5000 5002 +2
- Partials 123 124 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
integration_tests/test_upgrade.py (2)
165-194: Simplify thedo_upgradefunction to reduce code duplicationThe
do_upgradefunction contains duplicated code in the upgrade proposal logic for differentplan_namevalues. Refactoring can enhance maintainability by abstracting common elements.Consider the following refactor:
- Create a common proposal data dictionary with shared parameters.
- Modify only the necessary fields based on the
plan_name.- Use a single method call for proposal submission if possible.
290-290: Add validation steps after upgrading to "v1.4"Currently, there are no checks following the upgrade to "v1.4". Including validation steps similar to previous upgrades can help verify that all functionalities operate as expected.
Consider adding tests to:
- Validate basic transaction capabilities.
- Ensure smart contract interactions function correctly.
- Confirm that upgraded parameters are in effect.
integration_tests/cosmoscli.py (3)
731-732: Consider removing--no-validateto ensure transaction validationUsing the
--no-validateflag skips client-side validation of the transaction, which could result in invalid or malformed transactions being submitted. It is generally advisable to allow the client to perform validation checks before broadcasting the transaction. Consider removing this flag to ensure transaction integrity.
722-722: Add a docstring tosoftware_upgrademethod for better documentationConsider adding a docstring to the
software_upgrademethod to describe its functionality, parameters, and return value. This will enhance code readability and assist future maintainers in understanding its purpose.
722-749: Evaluate overlap betweensoftware_upgradeandgov_propose_legacymethodsThe new
software_upgrademethod appears to have similar functionality to the existinggov_propose_legacymethod when handling software upgrade proposals. To avoid code duplication and improve maintainability, consider refactoring these methods by consolidating common logic or extending the existing method to support the new requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
integration_tests/configs/upgrade-test-package.nix(2 hunks)integration_tests/cosmoscli.py(1 hunks)integration_tests/test_basic.py(1 hunks)integration_tests/test_upgrade.py(3 hunks)
🔇 Additional comments (6)
integration_tests/configs/upgrade-test-package.nix (2)
48-52: Verify version consistency in upgrade path
The new entries in linkFarm establish an upgrade path from v1.4 to v1.4.0-rc5-testnet. This aligns with the PR objective to fix the submit-legacy-proposal for rc5.
#!/bin/bash
# Description: Verify version consistency across the codebase
# Check for any other references to these versions
rg "v1\.4\.0-rc5-testnet" --type nix --type python
# Look for upgrade height configurations
rg "upgrade.*height" --type python24-26: Verify the commit hash for v1.4.x release
The addition of v1.4.x release configuration follows the established pattern, but let's verify the referenced commit.
✅ Verification successful
Based on the search results, I can verify that the commit hash ce797fa995000530ee53cd1fbeb3c67180648002 is correctly used in the upgrade-test-package.nix file and follows the established pattern for release configurations. The file shows:
- The commit is used for v1.4.x release configuration
- It's properly referenced in the linkFarm entries
- The pattern matches other release configurations in the file
Additionally, the search results show this is the only occurrence of this commit hash in the Nix files, which is expected.
The v1.4.x release configuration is properly set up
The upgrade test package configuration follows the established pattern and correctly maps the versions in the linkFarm entries:
v1.4points to released1_4 (using the verified commit)v1.4.0-rc5-testnetpoints to current package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the commit hash exists and is from the correct branch
# Check if the commit exists and get its details
gh api \
-H "Accept: application/vnd.github+json" \
repos/crypto-org-chain/cronos/commits/ce797fa995000530ee53cd1fbeb3c67180648002 \
--jq '.commit.message'
# Verify it's from the v1.4.x branch
gh api \
-H "Accept: application/vnd.github+json" \
repos/crypto-org-chain/cronos/branches \
--jq '.[] | select(.name | contains("v1.4")) | .name'
Length of output: 423
Script:
#!/bin/bash
# Let's try a different approach to verify the commit and branch information
# 1. Search for v1.4 related configurations in Nix files
rg "v1\.4" --type nix -A 3 -B 3
# 2. Search for the specific commit hash pattern
rg "ce797fa995000530ee53cd1fbeb3c67180648002" --type nix
# 3. Check other version patterns to understand the structure
rg "fetchFlake.*cronos" --type nix
# 4. Look for any documentation about version configurations
fd "README|CHANGELOG" --type f --exec cat {}
Length of output: 72638
integration_tests/test_basic.py (1)
875-875: LGTM: CLI instance correctly initialized after configuration changes.
The placement of obtaining the Cosmos CLI instance is correct, ensuring it reflects any configuration changes made through the supervisor.
integration_tests/test_upgrade.py (3)
165-194: Verify the need for the mode parameter in do_upgrade
The mode parameter is only used in the cli.gov_propose_legacy call and defaults to None. If it's not required, consider removing it from the function signature to simplify the code.
If mode is necessary in specific scenarios, ensure that it's appropriately passed when calling do_upgrade.
286-287: Confirm transaction history remains consistent after the upgrade
The assertion assert txs == get_txs(base_port, height) checks that transaction results are unchanged post-upgrade. Ensure that the upgrade doesn't inadvertently alter transaction history or blockchain state.
301-303: Basic transaction check after upgrading to "v1.4.0-rc5-testnet"
The check_basic_tx(c) function confirms that basic transactions work post-upgrade. This is a good practice to ensure the network's core functionalities remain intact.
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: pypi/[email protected], pypi/[email protected], pypi/[email protected] |
no upgrade-height related flags
👮🏻👮🏻👮🏻 !!!! 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
Bug Fixes
Documentation
Refactor