-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(simapp): un-wire crisis #18380
Conversation
WalkthroughThe overall changes revolve around the removal of the "crisis" module from the codebase. This includes the removal of import statements, variable declarations, function calls, and test functions related to the "crisis" module. The changes provide a more streamlined codebase by eliminating unnecessary components and dependencies. Changes
Poem
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- server/util.go (1 hunks)
- simapp/app.go (9 hunks)
- simapp/app_config.go (5 hunks)
- simapp/app_test.go (3 hunks)
- simapp/app_v2.go (4 hunks)
- simapp/export.go (1 hunks)
- simapp/sim_bench_test.go (2 hunks)
- simapp/simd/cmd/commands.go (3 hunks)
Files skipped from review due to trivial changes (6)
- simapp/app.go
- simapp/app_config.go
- simapp/app_test.go
- simapp/app_v2.go
- simapp/sim_bench_test.go
- simapp/simd/cmd/commands.go
Additional comments: 2
server/util.go (1)
- 339-345: The conditional check before calling
addStartFlags
is a good practice as it prevents potential null pointer exceptions ifaddStartFlags
is not provided. This change improves the robustness of the code.simapp/export.go (1)
- 72-77: The removal of the crisis module and its related logic seems to be correctly implemented in this function. Ensure that the removal of the crisis module does not affect the functionality of the remaining code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- simapp/upgrades.go (2 hunks)
Additional comments: 1
simapp/upgrades.go (1)
- 43-43: The
crisistypes.ModuleName
has been correctly added to theDeleted
field ofStoreUpgrades
to indicate that the crisis module has been discontinued.
Description
The crisis module does not work as intended (GHSA-qfc5-6r3j-jj22) and does not panic (needs something alike to work c91f5a9) and with its current design, can cause high load on chains.
The advisory mentions that crisis won't be fixed in order to make a chain alt.
And the simulator is going to replace the use case of crisis for invariant checking: #15706
With all this in mind, we should remove x/crisis from simapp so that developers that use simapp as an example do not simply add it to their chain. IMHO, we should simply remove it from the SDK from v0.51 thanks to the new simulator and x/circuit being already there.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit