Problem: can't support prevrando opcode #1366
Conversation
mmsqe
requested review from
calvinaco and
devashishdxt
and removed request for
a team
WalkthroughThis series of updates introduces a new Solidity smart contract for random number generation, enhances testing for EVM parameter updates, and simplifies the upgrade process in the application code. It also includes updates to configuration files and testing utilities to support these changes. The modifications aim to address issues with random number generation post-upgrade and improve the overall robustness and flexibility of the system. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration_tests/contracts/contracts/Random.sol (1 hunks)
- integration_tests/test_gov_update_params.py (1 hunks)
- integration_tests/utils.py (1 hunks)
Additional Context Used
Additional comments not posted (3)
integration_tests/contracts/contracts/Random.sol (1)
4-8: The contract implementation looks correct and follows best practices for generating a pseudo-random number in Solidity. However, it's important to note that the use of
block.prevrandaofor randomness is environment-specific and might not be available on all EVM-compatible chains. Consider adding a comment to clarify this dependency.integration_tests/test_gov_update_params.py (2)
11-51: The test
test_evm_update_paramis well-implemented and covers the process of deploying contracts, querying parameters, creating a governance proposal to update EVM parameters, and testing the parameter update process. Consider adding comments to explain the logic behind specific steps, especially the rationale for expecting certain error messages, to improve readability and maintainability.
53-53: The test
test_gov_update_paramsappears to be correctly implemented but lacks detailed comments explaining its purpose and the steps involved. Adding such comments would enhance the readability and maintainability of the test.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_gov_update_params.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration_tests/test_gov_update_params.py
Additional Context Used
mmsqe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- app/upgrades.go (1 hunks)
- gomod2nix.toml (1 hunks)
- integration_tests/configs/cosmovisor.jsonnet (1 hunks)
- integration_tests/configs/upgrade-test-package.nix (1 hunks)
- integration_tests/cosmoscli.py (1 hunks)
- integration_tests/test_upgrade.py (11 hunks)
Files not reviewed due to errors (2)
- gomod2nix.toml (Error: unable to parse review)
- CHANGELOG.md (Error: unable to parse review)
Additional Context Used
Additional comments not posted (7)
integration_tests/configs/upgrade-test-package.nix (1)
10-16: The update of the package version fromv1.0.12tov1.1.1and the adjustment of the package path naming from"v1.1.0"to"v1.1"are noted. Please ensure that these changes are consistently applied across the entire project to avoid any potential issues with dependencies or package resolution.Verification successful
The verification process confirms that the version update to
v1.1.1and the adjustment of the package path naming to"v1.1"are consistently applied across the project where necessary. The changes align with the project's requirements and objectives.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new version and path naming are consistently used across the project. rg --type nix 'v1\.1\.1' || echo "Version v1.1.1 not found in other Nix files." rg --type nix 'v1\.1' || echo "Path naming v1.1 not found in other Nix files."Length of output: 330
integration_tests/configs/cosmovisor.jsonnet (2)
11-15: The addition ofconsensus_paramswith block settings is noted. Please ensure that these settings are aligned with the network's governance and consensus rules.
18-27: The restructuring ofapp_stateby movingsend_enabledunderbankand adjustingfeemarketparameters is noted. Please ensure that these changes are thoroughly reviewed and approved as per the network's governance processes.integration_tests/test_upgrade.py (3)
64-68: Refactoring fixture names and adjusting function parameters in the setup for the upgrade test are noted. Please ensure that these changes accurately reflect the intended setup for testing the upgrade process.
85-91: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [88-100]
The changes to the upgrade process, including proposing an upgrade and passing it, are noted. Please ensure that the upgrade process is thoroughly reviewed and tested to confirm its accuracy and reliability.
118-124: The deployment of a new contract and the handling of contract calls differently post-upgrade are significant changes. Please ensure that these changes are thoroughly tested to confirm that they accurately reflect the intended behavior post-upgrade.integration_tests/cosmoscli.py (1)
898-899: The modification to theexportmethod to accept keyword arguments enhances its flexibility and is a good practice for future-proofing the method. Ensure that the use of this method across the codebase is audited to prevent the unintentional passing of parameters that could alter its behavior in unexpected ways.Verification successful
The usage of the
exportmethod inintegration_tests/test_upgrade.pywith a keyword argument demonstrates correct utilization of the method's enhanced flexibility. This indicates that the change to the method is compatible with its current uses in the codebase. However, it's advisable to conduct a broader audit of the method's usage if more extensive changes are made in the future or if the method is used in more complex scenarios.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the `export` method to ensure it's used correctly with the new keyword arguments capability. rg --type py 'export\('Length of output: 170
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1366 +/- ##
===========================================
+ Coverage 15.90% 35.37% +19.47%
===========================================
Files 80 129 +49
Lines 6230 11643 +5413
===========================================
+ Hits 991 4119 +3128
- Misses 5160 7147 +1987
- Partials 79 377 +298
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_gov_update_params.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration_tests/test_gov_update_params.py
Additional Context Used
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum
Files selected for processing (6)
- app/app.go (1 hunks)
- app/forks.go (1 hunks)
- gomod2nix.toml (1 hunks)
- integration_tests/configs/fork.jsonnet (1 hunks)
- integration_tests/network.py (2 hunks)
- integration_tests/test_fork.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- gomod2nix.toml
Additional Context Used
Additional comments not posted (8)
integration_tests/configs/fork.jsonnet (1)
1-9: The configuration changes are well-structured and follow best practices for modularity and reusability.integration_tests/test_fork.py (2)
11-18: The fixture setup is clear and well-structured. Consider adding a comment explaining why the specific chain ID and configuration are chosen for this test.
21-35: The test case is well-implemented. It would be beneficial to add more comments explaining each step of the test and the expected outcomes, especially why the "invalid memory address or nil pointer dereference" error is expected initially and why deploying the "Greeter" contract changes the outcome.app/forks.go (2)
34-39: TheRandomOpcodeForkHeightsmap is crucial for determining when the fork is enabled. Ensure that the fork heights are finalized and accurate. Consider removing the TODO comment after finalizing the values or providing more context about the decision-making process.
41-43: The implementation ofRandomOpcodeForkEnabledis straightforward and effective. It's important to ensure that the fork heights inRandomOpcodeForkHeightsare accurate for this function to work as intended.integration_tests/network.py (2)
161-161: Adding a default value for thechain_idparameter insetup_custom_cronosincreases flexibility for testing. Consider adding a comment explaining why "cronos_777-1" is chosen as the default value.
190-190: The modification to yield aCronosinstance with a dynamic path based on thechain_idis a good practice for ensuring test isolation and clarity. This change enhances the flexibility and readability of the testing setup.app/app.go (1)
545-545: The addition ofRandomOpcodeForkEnabledas a parameter in theNewfunction is noted. Please ensure that this parameter is effectively used to toggle the support for theprevrandoopcode, particularly in the EVM keeper's initialization or configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/upgrades.go (1 hunks)
- default.nix (1 hunks)
- integration_tests/configs/upgrade-test-package.nix (1 hunks)
- integration_tests/test_upgrade.py (11 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (2)
- integration_tests/configs/upgrade-test-package.nix
- integration_tests/test_upgrade.py
Additional Context Used
Additional comments not posted (2)
app/upgrades.go (1)
12-12: The upgrade plan name has been updated to "v1.2". Ensure that all references to the upgrade plan across the codebase and documentation are consistently updated to reflect this change.Verification successful
The search did not find any references to the old upgrade plan name "v1.1" in Go files, indicating that the upgrade plan name has been consistently updated to "v1.2" across the codebase in Go files.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any occurrences of the old upgrade plan name "v1.1" to ensure they have been updated. rg 'v1\.1' --type go || echo "No references to the old upgrade plan name found."Length of output: 131
default.nix (1)
14-14: The version has been updated to "v1.2.0". This change should be reflected in all relevant documentation, release notes, and any other places where the version number is mentioned to ensure consistency across the project.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum
Files selected for processing (1)
- gomod2nix.toml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- gomod2nix.toml
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
Closes #1367
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
Randomfor generating random token IDs.RandomOpcodeForkto enable specific functionality based on block height and chain ID.RandomOpcodeForkEnabledparameter in theNewfunction call.RandomOpcodeForkHeightsto store fork heights and a functionRandomOpcodeForkEnabledfor checking random opcode fork status.Bug Fixes
exportmethod incosmoscli.pyfor improved flexibility and customization.Documentation
CHANGELOG.mdto reflect changes in random opcode behavior.Chores
gomod2nix.toml.upgrade-test-package.nix.Tests