-
Notifications
You must be signed in to change notification settings - Fork 240
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
Problem: authz module is risky #1274
Conversation
Solution: - it's already disabled in mempool in v1.0.x, now remove completely in v1.1.x.
WalkthroughThe unidiff snippets indicate the complete removal of the "authz" module from the codebase, signifying a significant alteration to the logic and functionality of the application, particularly in the area related to authorization. This involves the removal of related imports, module declarations, and initializations across various files, reflecting a substantial shift in how the application handles authorization. Changes
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? TipsChat with CodeRabbit Bot (
|
Signed-off-by: yihuang <huang@crypto.com>
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 (3)
- CHANGELOG.md (1 hunks)
- app/app.go (9 hunks)
- app/upgrades.go (2 hunks)
Additional comments: 11
app/upgrades.go (2)
12-12: The addition of the
authztypes
import is necessary for the removal of theauthz
module's store. Ensure that this import is used elsewhere in the file as intended and that there are no references toauthz
left that would require this import.149-158: The addition of
authztypes.StoreKey
to theDeleted
field ofstoreUpgrades
for both the mainnet and testnet upgrade plans is consistent with the objective of removing theauthz
module. Verify that the upgrade logic has been tested and that the deletion of theauthz
store does not affect other parts of the system unexpectedly.CHANGELOG.md (1)
- 6-6: The update to the
CHANGELOG.md
correctly reflects the significant change of removing theauthz
module in pull request Problem: authz module is risky #1274.app/app.go (8)
248-253: The removal of the
authz
module fromGenModuleBasics
aligns with the PR objective to remove theauthz
module from the codebase. Ensure that dependent modules and functionalities are not affected by this removal.278-281: The removal of
authz
related keys from theStoreKeys
function is consistent with the PR objective. Ensure that there are no remaining references to these keys elsewhere in the codebase that could cause runtime errors.340-344: The removal of
authz
related keepers from theApp
struct is consistent with the PR objective. Ensure that there are no remaining references to these keepers or methods that were specifically used by theauthz
module.462-467: The removal of
authz
related functionality from theApp
struct's initialization is consistent with the PR objective. Ensure that the initialization sequence of other modules is not disrupted by this change.697-702: The removal of the
authz
module from theNewAppModule
function calls is consistent with the PR objective. Confirm that the app module's functionality remains intact and that no other modules depend on theauthz
module.728-733: The removal of the
authz
module from theSetOrderBeginBlockers
function calls is consistent with the PR objective. Ensure that the order of operations during the begin block is not negatively impacted by this change.750-755: The removal of the
authz
module from theSetOrderEndBlockers
function calls is consistent with the PR objective. Confirm that the order of operations during the end block remains correct and that no dependencies on theauthz
module exist.783-788: The removal of the
authz
module from theSetOrderInitGenesis
function calls is consistent with the PR objective. Ensure that the initialization of the genesis state for other modules is not affected by this change.
Signed-off-by: yihuang <huang@crypto.com>
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.
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 (1)
- app/upgrades.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/upgrades.go
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1274 +/- ##
===========================================
+ Coverage 16.02% 35.77% +19.75%
===========================================
Files 80 116 +36
Lines 6184 10663 +4479
===========================================
+ Hits 991 3815 +2824
- Misses 5114 6471 +1357
- Partials 79 377 +298
|
Signed-off-by: yihuang <huang@crypto.com>
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 (1)
- CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
seems get wrong Block.Header.AppHash in upgrade test |
yeah, maybe some inconsistency between memiavl and iavl handling deletion of store. |
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)
- store/rootmulti/store.go (2 hunks)
Additional comments: 1
store/rootmulti/store.go (1)
- 371-373: The addition of an else block to initialize
lastCommitInfo
as an emptytypes.CommitInfo
struct when the database version is 0 is a good safety measure. This ensures thatlastCommitInfo
is never left uninitialized, which could prevent potential nil pointer dereferences.
42f2597
Solution:
👮🏻👮🏻👮🏻 !!!! 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
Bug Fixes
Refactor
Documentation
CHANGELOG.md
to reflect significant changes in authorization logic.