fix: use versioned cms to take snapshot at simulation and check tx #208
Conversation
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (2)
You can disable this status message by setting the WalkthroughThe changes involve replacing the Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
=======================================
Coverage 38.97% 38.97%
=======================================
Files 251 251
Lines 24165 24165
=======================================
Hits 9418 9418
Misses 13233 13233
Partials 1514 1514 |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (7)
- app/app_test.go (2 hunks)
- x/move/abci_test.go (4 hunks)
- x/move/ante/ante_test.go (3 hunks)
- x/move/ante/fee_test.go (1 hunks)
- x/move/ante/gas_prices_test.go (1 hunks)
- x/move/common_test.go (2 hunks)
- x/reward/abci_test.go (5 hunks)
Files skipped from review due to trivial changes (1)
- x/move/ante/gas_prices_test.go
Additional comments not posted (14)
x/move/abci_test.go (4)
33-33: ReplacedFinalizeBlockwithCommit. This change should be consistent across all tests that simulate block finalization.
65-65: Usage ofNewUncachedContexthere ensures that the state from previous operations isn't inadvertently carried over, which is crucial for accurate testing.
26-26: Switched toNewUncachedContextto avoid caching in the context. Confirm this aligns with the intended use case.Verification successful
To proceed with the verification, we need to confirm the usage pattern of
NewUncachedContextinx/move/abci_test.goand ensure it aligns with the intended use case. Here is the next step:
The usage of
NewUncachedContextinx/move/abci_test.goappears to be for scenarios where a fresh context is needed without any caching, which is important to ensure a clean state for operations like initializing staking and generating rewards.Given that
NewUncachedContextis used appropriately in these test scenarios and there are no other instances ofNewUncachedContextor similar changes found in the rest of the codebase, the change aligns with the intended use case.
x/move/abci_test.go: Lines around the usage ofNewUncachedContextconfirm its purpose for avoiding caching in specific test scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the use of `NewUncachedContext` in other parts of the codebase. # Test: Search for the function usage. Expect: No other usage of the old `NewContext` in similar scenarios. rg --type go $'NewContext'Length of output: 7793
Script:
#!/bin/bash # Description: Extract the relevant section of `x/move/abci_test.go` to review the context of `NewUncachedContext`. # Extract lines around the usage of `NewUncachedContext` for detailed review. rg --type go --context 5 'NewUncachedContext' x/move/abci_test.goLength of output: 928
82-82: Multiple calls toCommitin test scenarios. Ensure that these are intentional and correctly reflect the test's requirements for state finalization.Also applies to: 89-89
x/move/ante/ante_test.go (3)
9-9: Updated import alias for clarity and consistency with other test files.
43-51: The function now initializes the app without theisCheckTxparameter, reflecting a simplified context creation. Ensure all dependent tests are updated accordingly.
61-63: Removal ofisCheckTxfromSetupTestsimplifies the test setup. Verify that this does not affect any tests that depended on this parameter to simulate different transaction checks.x/move/ante/fee_test.go (1)
64-64: Updated to use the newSetupTestmethod without parameters. This change should be reflected across all tests to maintain consistency.x/reward/abci_test.go (3)
14-14: Updated import alias to maintain consistency across the project.
50-50: Multiple calls toCommitacross various test scenarios. Ensure these are intentional and accurately reflect the tests' requirements for state finalization.Also applies to: 62-62, 69-69, 119-119, 126-126, 133-133
26-26: Switched toNewUncachedContextin multiple places to ensure consistency in context handling without caching. Verify that this change aligns with the testing and operational requirements.Also applies to: 97-97
Verification successful
The review comment asks to verify the consistent use of
NewUncachedContextacross similar test cases and ensure it aligns with the testing and operational requirements. Thergcommand output shows that this function is used consistently in multiple test files.To ensure that this change aligns with the testing and operational requirements, we need to verify that the context handling in these files is appropriate and consistent. We will inspect the context usage in these files to confirm it aligns with the operational requirements.
The inspection of context usage across the identified files shows that
NewUncachedContextis consistently used in various test scenarios. This indicates a uniform approach to context handling without caching, aligning with the operational requirements.Verified consistent usage:
x/reward/abci_test.gox/move/abci_test.gox/move/common_test.gox/move/ante/ante_test.gox/ibc/testing/chain.goapp/app_test.goThe consistent use of
NewUncachedContextacross these files ensures that the context handling is uniform and meets the testing requirements. Therefore, the change aligns with the operational requirements.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent use of `NewUncachedContext` across similar test cases. # Test: Search for the function usage. Expect: Consistent use across all tests. rg --type go $'NewUncachedContext'Length of output: 823
Script:
#!/bin/bash # Description: Verify the context usage in the identified files to ensure it aligns with the operational requirements. # Inspect context usage in `x/reward/abci_test.go` echo "Context usage in x/reward/abci_test.go:" rg --context 5 'NewUncachedContext' x/reward/abci_test.go # Inspect context usage in `x/move/abci_test.go` echo "Context usage in x/move/abci_test.go:" rg --context 5 'NewUncachedContext' x/move/abci_test.go # Inspect context usage in `x/move/common_test.go` echo "Context usage in x/move/common_test.go:" rg --context 5 'NewUncachedContext' x/move/common_test.go # Inspect context usage in `x/move/ante/ante_test.go` echo "Context usage in x/move/ante/ante_test.go:" rg --context 5 'NewUncachedContext' x/move/ante/ante_test.go # Inspect context usage in `x/ibc/testing/chain.go` echo "Context usage in x/ibc/testing/chain.go:" rg --context 5 'NewUncachedContext' x/ibc/testing/chain.go # Inspect context usage in `app/app_test.go` echo "Context usage in app/app_test.go:" rg --context 5 'NewUncachedContext' app/app_test.goLength of output: 4637
x/move/common_test.go (1)
76-76: The change fromNewContexttoNewUncachedContextaligns with the PR's goal to use uncached contexts for state simulation. This should help in avoiding state pollution during tests.app/app_test.go (2)
81-81: Switching toNewUncachedContextis appropriate for testing scenarios where a clean slate is required for each test to prevent side effects from caching. This should enhance test reliability.
130-130: UsingNewUncachedContexthere ensures that the test environment is strictly controlled, which is crucial for accurately testing the state upgrade logic.
| @@ -87,7 +87,7 @@ func createApp(t *testing.T) *initiaapp.InitiaApp { | |||
| // fund second bond coin | |||
| app.BankKeeper.SendCoins(ctx, types.StdAddr, addr1, sdk.NewCoins(secondBondCoin)) | |||
|
|
|||
| _, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) | |||
| _, err = app.Commit() | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The inspection reveals that there are still multiple instances of FinalizeBlock present in the test files. These should be replaced with Commit to ensure consistency and avoid potential regressions.
- Files with remaining
FinalizeBlockreferences:x/reward/abci_test.gox/reward/common_test.gox/move/abci_test.gox/move/common_test.gox/move/types/common_test.gox/gov/common_test.gox/distribution/abci_test.gox/distribution/common_test.gobenchmark/app_test.go
Please update the above files to replace FinalizeBlock with Commit.
Analysis chain
Replacing FinalizeBlock with Commit is consistent with the intended change to use the more direct commit operation in tests. Ensure this change is validated with appropriate test cases to avoid regression.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that replacing `FinalizeBlock` with `Commit` does not introduce regressions.
# Test: Search for `FinalizeBlock` usage in test files to ensure it's replaced everywhere as intended.
rg --type go --files-with-matches 'FinalizeBlock' --glob '*_test.go'
Length of output: 285
Script:
#!/bin/bash
# Description: Inspect the listed files for any remaining references to `FinalizeBlock`.
# Files to inspect
files=(
"x/reward/abci_test.go"
"x/reward/common_test.go"
"x/move/abci_test.go"
"x/move/common_test.go"
"x/move/types/common_test.go"
"x/gov/common_test.go"
"x/distribution/abci_test.go"
"x/distribution/common_test.go"
"benchmark/app_test.go"
)
# Search for `FinalizeBlock` in the specified files
for file in "${files[@]}"; do
echo "Inspecting $file"
rg 'FinalizeBlock' "$file"
done
Length of output: 3616
beer-1
changed the title
related: cosmos/cosmos-sdk#20686