chore: cleanup and improve x/mint params validation and test in cosmos-sdk (backport #25562)
#1919
Conversation
…mos-sdk (backport #25562) (crypto-org-chain#1918) * update cosmos-sdk (backport #26652) - improve mint params validation * add integration tests * fix python lint * remove comment Signed-off-by: randy-cro <[email protected]> --------- Signed-off-by: randy-cro <[email protected]>
randy-cro
requested review from
calvinaco and
thomas-nguy
and removed request for
a team
WalkthroughAdds mint integration tests and a pytest marker, updates the cosmos-sdk dependency (go.mod and gomod2nix.toml), and includes Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Mint Test
participant CLI as Cronos CLI
participant Gov as Gov Module
participant Chain as Blockchain
Test->>CLI: Query current mint params
CLI-->>Test: Return current BlocksPerYear
rect rgb(200,220,255)
Note over Test,Gov: Proposal submission flow
Test->>Test: Normalize decimal fields
Test->>CLI: Submit gov proposal (update BlocksPerYear)
CLI->>Gov: Create proposal
Gov->>Chain: Store proposal
Chain-->>CLI: Confirm submission
CLI-->>Test: Submission result
end
rect rgb(220,255,220)
Note over Test,Chain: Verification flow
Test->>Test: Wait for blocks / proposal enactment
Test->>CLI: Query mint params again
CLI->>Chain: Fetch state
Chain-->>CLI: Return updated params
CLI-->>Test: New BlocksPerYear
Test->>Test: Assert expected outcome
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
integration_tests/test_mint_blocks_per_year.py (2)
27-46: Consider validating decimal values instead of returning them as-is.Lines 34-35 return values containing "." without validation. This could accept malformed decimals (e.g., wrong number of decimal places). Consider normalizing existing decimal values or adding validation to ensure they conform to the 18-decimal-place LegacyDec format.
def normalize_legacy_dec(value: str) -> str: """ Ensure math.LegacyDec strings have an explicit decimal point (scale 18). This matches the Cosmos SDK's LegacyDec format. """ if not value: return "0.000000000000000000" if "." in value: - return value + # Validate and normalize existing decimal values + parts = value.split(".") + if len(parts) != 2: + raise ValueError(f"Invalid decimal format: {value}") + int_part = parts[0] or "0" + frac_part = parts[1].ljust(18, "0")[:18] + return f"{int_part}.{frac_part}" sign = "" if value[0] == "-": sign = "-" value = value[1:] stripped = value.lstrip("0") if not stripped: return "0.000000000000000000" padded = stripped.rjust(19, "0") int_part = padded[:-18] or "0" frac_part = padded[-18:] return f"{sign}{int_part}.{frac_part}"
86-101: Improve exception handling specificity.The current implementation catches all exceptions broadly and returns False, which can hide unexpected errors and make debugging difficult. Consider catching specific exceptions or re-raising unexpected errors.
Based on learnings
try: submit_gov_proposal( cronos, msg, messages=[ { "@type": msg, "authority": authority, "params": params, } ], ) - return True - except (AssertionError, Exception) as e: + except AssertionError as e: + # Expected validation failure print(f"Proposal failed as expected: {e}") return False + except Exception as e: + # Unexpected error - log and re-raise for debugging + print(f"Unexpected error during proposal submission: {e}") + raise + else: + return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks)go.mod(1 hunks)gomod2nix.toml(1 hunks)integration_tests/conftest.py(1 hunks)integration_tests/test_mint_blocks_per_year.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_mint_blocks_per_year.py (3)
integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (1)
query_params(1115-1126)integration_tests/conftest.py (1)
cronos(63-72)
🪛 Ruff (0.14.5)
integration_tests/test_mint_blocks_per_year.py
98-98: Consider moving this statement to an else block
(TRY300)
99-99: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
integration_tests/conftest.py (1)
26-26: LGTM!The new pytest marker for mint module tests follows the established pattern and is properly documented.
.github/workflows/test.yml (1)
22-22: LGTM!The mint test suite is properly integrated into the CI workflow matrix, ensuring the new tests run alongside existing integration tests.
gomod2nix.toml (1)
216-217: LGTM!The cosmos-sdk version and hash are correctly updated to match the go.mod changes. The verification requested for go.mod applies here as well.
integration_tests/test_mint_blocks_per_year.py (4)
1-24: LGTM!The module documentation clearly describes the test scenarios, imports are appropriate, and the constants are correctly defined for boundary testing.
49-52: LGTM!Simple and clear helper function for querying mint module parameters.
104-129: LGTM!The test correctly verifies that blocks_per_year = 0 is rejected and that the parameter remains unchanged after the failed proposal.
206-256: LGTM!Both overflow test cases correctly verify that values exceeding MAX_INT64 (both MAX_INT64+1 and UINT64_MAX) are properly rejected and that the parameter remains unchanged.
go.mod (1)
284-284: No issues found.The commit
8d0a31ef043dexists in the forked repository and contains the expected mint parameter validation changes ("chore: cleanup and improvex/mintparams validation and test"). The pseudo-version reference is valid.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1919 +/- ##
===========================================
+ Coverage 16.87% 37.25% +20.38%
===========================================
Files 72 127 +55
Lines 6163 9730 +3567
===========================================
+ Hits 1040 3625 +2585
- Misses 5000 5682 +682
- Partials 123 423 +300 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/test_mint_blocks_per_year.py (1)
86-101: Tighten exception handling insubmit_mint_param_updateto avoid masking unexpected errorsCatching
(AssertionError, Exception)and unconditionally treating any exception as an expected proposal failure both trips Ruff (BLE001/TRY300) and can hide genuinely unexpected issues (e.g., coding errors, I/O problems) behind aFalsereturn.Consider narrowing the expected-failure path and letting unexpected exceptions surface, and optionally using an
elsefor the success case, e.g.:- try: - submit_gov_proposal( - cronos, - msg, - messages=[ - { - "@type": msg, - "authority": authority, - "params": params, - } - ], - ) - return True - except (AssertionError, Exception) as e: - print(f"Proposal failed as expected: {e}") - return False + try: + submit_gov_proposal( + cronos, + msg, + messages=[ + { + "@type": msg, + "authority": authority, + "params": params, + } + ], + ) + except AssertionError as e: + # Expected failure path in tests where the proposal should be rejected + print(f"Proposal failed as expected: {e}") + return False + else: + return TrueAdjust the caught exception type(s) to whatever
submit_gov_proposalactually raises on expected failures so that unexpected exceptions still bubble up with full tracebacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_mint_blocks_per_year.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_mint_blocks_per_year.py (3)
integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (1)
query_params(1115-1126)integration_tests/conftest.py (1)
cronos(63-72)
🪛 Ruff (0.14.5)
integration_tests/test_mint_blocks_per_year.py
98-98: Consider moving this statement to an else block
(TRY300)
99-99: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
integration_tests/test_mint_blocks_per_year.py (1)
104-256: Boundary coverage forblocks_per_yearlooks solidThe tests exercise zero, a small valid value,
MaxInt64,MaxInt64 + 1, andUINT64_MAX, and they verify both parameter persistence and continued block production. This is a good, focused set of integration checks around the mint params update path.
randy-cro
changed the title
x/mint params validation and test in cosmos-sdk (backport #25562)
3 participants
👮🏻👮🏻👮🏻 !!!! 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
Tests
Chores