chore(x/authz)!: Remove account keeper dependency #21632
Conversation
📝 WalkthroughWalkthroughThe changes involve modifications to the initialization and handling of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
facundomedica
changed the title
github-actions
bot
added
C:x/genutil
alpe
force-pushed
the
alex/sims2_main
branch
4 times, most recently
from
c220ca1
to
4690044
Compare
facundomedica
force-pushed
the
facu/try-no-auth
branch
from
5631f45
to
fe95ddf
Compare
facundomedica
requested review from
akhilkumarpilli,
raynaudoe,
a team and
julienrbrt
as code owners
… into facu/try-no-auth
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
x/authz/keeper/genesis.go (2)
21-28: LGTM! Consider enhancing error handling.The changes successfully replace
k.authKeeper.AddressCodec()withk.addrCdc, which aligns with the PR objective of removing the account keeper dependency. The logic remains intact, and error handling is preserved.Consider wrapping the errors with more context for easier debugging:
grantee, err := k.addrCdc.StringToBytes(entry.Grantee) if err != nil { - return err + return fmt.Errorf("failed to convert grantee address: %w", err) } granter, err := k.addrCdc.StringToBytes(entry.Granter) if err != nil { - return err + return fmt.Errorf("failed to convert granter address: %w", err) }
47-54: LGTM! Consider enhancing error handling.The changes successfully replace
k.authKeeper.AddressCodec()withk.addrCdc, which is consistent with the modifications in theInitGenesisfunction and aligns with the PR objective of removing the account keeper dependency. The logic remains intact, and error handling is preserved.Consider wrapping the errors with more context for easier debugging:
granterAddr, err := k.addrCdc.BytesToString(granter) if err != nil { - return false, err + return false, fmt.Errorf("failed to convert granter address: %w", err) } granteeAddr, err := k.addrCdc.BytesToString(grantee) if err != nil { - return false, err + return false, fmt.Errorf("failed to convert grantee address: %w", err) }x/authz/keeper/genesis_test.go (1)
66-67: LGTM: SetupTest method updated correctly with a minor suggestionThe changes in the
SetupTestmethod align with the PR objective of removing the account keeper dependency. The introduction of the address codec and the updated keeper initialization are correct.Consider adding a brief comment explaining the purpose of the address codec for improved readability:
- addrCdc := addresscodec.NewBech32Codec("cosmos") + // Create a Bech32 codec for address encoding/decoding + addrCdc := addresscodec.NewBech32Codec("cosmos")x/authz/module/abci_test.go (2)
57-59: LGTM with a minor suggestion for improved readability.The changes successfully remove the account keeper dependency and introduce the address codec. This aligns well with the PR objectives.
Consider adding a brief comment explaining the purpose of
addrCdcfor improved code readability:+// Create an address codec for Cosmos addresses addrCdc := address.NewBech32Codec("cosmos")
63-64: LGTM with a suggestion for improved error handling.The changes correctly use the new address codec to convert the grantee address to a string, providing more meaningful error messages.
Consider handling the potential error from
addrCdc.BytesToString(grantee):-addr, _ := addrCdc.BytesToString(grantee) -require.NoError(t, err, "Grant from %s", addr) +addr, err := addrCdc.BytesToString(grantee) +require.NoError(t, err, "Failed to convert address to string") +require.NoError(t, err, "Grant from %s", addr)x/authz/CHANGELOG.md (3)
Line range hint
40-55: LGTM with a minor suggestion for clarityThe "API Breaking Changes" section is well-structured and provides clear information about the significant changes. Each item is properly formatted with a GitHub issue reference.
Consider adding a brief explanation of the impact of the change in the
NewKeepermethod (line 41). For example:- * [#21632](https://github.com/cosmos/cosmos-sdk/pull/21632) `NewKeeper` now takes `address.Codec` instead of `authKeeper`. + * [#21632](https://github.com/cosmos/cosmos-sdk/pull/21632) `NewKeeper` now takes `address.Codec` instead of `authKeeper`. This change decouples the authz module from the auth module, improving modularity.
Line range hint
57-58: Consider adding more detail about the impactThe consensus breaking change is clearly stated, but it would be helpful to provide more information about its impact on the system and any necessary actions for users or developers.
Consider expanding the description to include the impact and any required actions:
- * [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist. + * [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist. This change prevents automatic account creation, potentially affecting transactions to new addresses. Developers should ensure their applications handle cases where the recipient account doesn't exist.
Line range hint
60-61: Consider clarifying the previous behaviorThe bug fix is clearly stated, but it would be helpful to briefly mention the previous behavior for context.
Consider modifying the description to include the previous behavior:
- * [#19874](https://github.com/cosmos/cosmos-sdk/pull/19923) Now when querying transaction events (cosmos.tx.v1beta1.Service/GetTxsEvent) the response will contain only UTF-8 characters + * [#19874](https://github.com/cosmos/cosmos-sdk/pull/19923) Fix transaction event queries (cosmos.tx.v1beta1.Service/GetTxsEvent) to ensure responses contain only UTF-8 characters. Previously, responses could include non-UTF-8 characters, potentially causing issues for clients.x/authz/keeper/msg_server.go (2)
90-93: LGTM: Consistent address conversion across methodsThe changes in the
RevokeAllandExecmethods are consistent with those in other methods, applying the same address conversion simplification. The error handling remains appropriate and consistent across all methods.Consider refactoring the address conversion logic into a separate helper function to reduce code duplication. For example:
func (k Keeper) convertAddress(address string) ([]byte, error) { converted, err := k.addrCdc.StringToBytes(address) if err != nil { return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid address: %s", err) } return converted, nil }This helper function could then be used across all methods, reducing duplication and improving maintainability.
Also applies to: 108-111
Line range hint
1-158: Summary: Successful removal of account keeper dependencyThe changes in this file consistently replace
k.authKeeper.AddressCodec().StringToByteswithk.addrCdc.StringToBytesacross multiple methods (Grant,Revoke,RevokeAll, andExec). This modification successfully removes the account keeper dependency as per the PR objectives.Key points:
- The changes are consistent and maintain proper error handling.
- The code adheres to the Uber Go Style Guide.
- The functionality remains the same, focusing only on the address conversion process.
To further improve the code:
- Consider implementing the suggested helper function for address conversion to reduce code duplication.
- Ensure that the
addrCdcfield is properly initialized in theKeeperstruct to maintain the functionality provided by the removedauthKeeper.- Update any relevant documentation or comments to reflect these changes, especially regarding the removal of the account keeper dependency.
Overall, these changes effectively achieve the PR's goal while maintaining code quality and consistency.
x/authz/keeper/grpc_query.go (3)
28-35: LGTM! Consider consolidating error handling.The changes look good. The switch from
k.authKeeper.AddressCodec()tok.addrCdcaligns with the PR objective of removing the account keeper dependency.Consider consolidating the error handling for both address conversions to reduce code duplication:
granter, grantee, err := k.convertAddresses(req.Granter, req.Grantee) if err != nil { return nil, err } // Helper method func (k Keeper) convertAddresses(granter, grantee string) ([]byte, []byte, error) { granterBytes, err := k.addrCdc.StringToBytes(granter) if err != nil { return nil, nil, err } granteeBytes, err := k.addrCdc.StringToBytes(grantee) if err != nil { return nil, nil, err } return granterBytes, granteeBytes, nil }This approach would make the code more DRY and easier to maintain.
98-101: LGTM! Consider moving address conversion to a separate function.The changes look good and are consistent with the previous method. The switch from
k.authKeeper.AddressCodec()tok.addrCdcaligns with the PR objective of removing the account keeper dependency.Consider moving the address conversion logic to separate helper functions to improve code readability and reusability:
func (k Keeper) stringToBytes(address string) ([]byte, error) { return k.addrCdc.StringToBytes(address) } func (k Keeper) bytesToString(address []byte) (string, error) { return k.addrCdc.BytesToString(address) }Then you can use these helper functions in your code:
granter, err := k.stringToBytes(req.Granter) if err != nil { return nil, err } // ... granteeAddr, err := k.bytesToString(grantee) if err != nil { return nil, err }This approach would make the code more modular and easier to maintain across all methods in this file.
Also applies to: 119-122
149-152: LGTM! Consider implementing consistent error handling.The changes look good and are consistent with the previous methods. The switch from
k.authKeeper.AddressCodec()tok.addrCdcaligns with the PR objective of removing the account keeper dependency.To improve consistency and error handling across all methods in this file, consider implementing a uniform approach to address conversion and error handling. For example:
func (k Keeper) mustStringToBytes(address string) []byte { bytes, err := k.addrCdc.StringToBytes(address) if err != nil { panic(err) } return bytes } func (k Keeper) mustBytesToString(address []byte) string { str, err := k.addrCdc.BytesToString(address) if err != nil { panic(err) } return str }Then use these functions consistently across all methods:
grantee := k.mustStringToBytes(req.Grantee) // ... granterAddr := k.mustBytesToString(granter)This approach would provide a consistent way of handling address conversions and potential errors across the entire file, making the code more robust and easier to maintain.
Also applies to: 172-175
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (14)
- simapp/app.go (1 hunks)
- x/authz/CHANGELOG.md (1 hunks)
- x/authz/expected_keepers.go (0 hunks)
- x/authz/keeper/genesis.go (2 hunks)
- x/authz/keeper/genesis_test.go (3 hunks)
- x/authz/keeper/grpc_query.go (5 hunks)
- x/authz/keeper/grpc_query_test.go (4 hunks)
- x/authz/keeper/keeper.go (6 hunks)
- x/authz/keeper/keeper_test.go (9 hunks)
- x/authz/keeper/msg_server.go (4 hunks)
- x/authz/keeper/msg_server_test.go (14 hunks)
- x/authz/module/abci_test.go (2 hunks)
- x/authz/module/depinject.go (3 hunks)
- x/authz/testutil/expected_keepers_mocks.go (0 hunks)
💤 Files with no reviewable changes (2)
- x/authz/expected_keepers.go
- x/authz/testutil/expected_keepers_mocks.go
🧰 Additional context used
📓 Path-based instructions (12)
simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/authz/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/genesis_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/grpc_query_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/msg_server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/module/abci_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/module/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/authz/module/depinject.go (1)
Learnt from: alpe PR: cosmos/cosmos-sdk#21613 File: x/authz/module/depinject.go:45-45 Timestamp: 2024-09-12T08:07:56.996Z Learning: The AccountKeeper in ModuleInputs for the authz module is used to set up the authz keeper and should not be removed, even if it's no longer a parameter in the NewAppModule function.
🔇 Additional comments (42)
x/authz/module/depinject.go (3)
5-5: LGTM: Import addition is appropriate.The addition of the
cosmossdk.io/core/addressimport is necessary and aligns with the changes made to theModuleInputsstructure.
44-44: Verify keeper functionality with AddressCodec.The
keeper.NewKeeperfunction now usesin.AddressCodecinstead ofin.AccountKeeper. While this change is consistent with the modifications in theModuleInputsstructure, it's crucial to ensure that:
- The
NewKeeperfunction in the keeper package has been updated to acceptAddressCodecinstead ofAccountKeeper.- All functionalities previously dependent on
AccountKeeperwithin the keeper are now correctly implemented usingAddressCodec.- There are no breaking changes in the keeper's behavior.
Let's verify the changes in the keeper package:
#!/bin/bash # Check the NewKeeper function signature in the keeper package rg --type go 'func NewKeeper' x/authz/keeper # Search for AddressCodec usage in the keeper package rg --type go 'AddressCodec' x/authz/keeper
30-33: Verify the removal of AccountKeeper and its impact.The removal of
AccountKeeperand addition ofAddressCodecin theModuleInputsstructure aligns with the PR objective of removing the account keeper dependency. However, a previous review comment suggests that theAccountKeeperis used to set up the authz keeper and should not be removed.Please verify:
- The impact of this change on the authz keeper setup.
- That all functionalities previously dependent on
AccountKeeperare now correctly handled usingAddressCodec.- Any potential breaking changes in the module's behavior.
To help verify the impact, let's check the usage of AccountKeeper in the authz module:
✅ Verification successful
AccountKeeper successfully removed from x/authz module.
The
AccountKeeperdependency has been successfully removed from thex/authzmodule. No references toAccountKeeperwere found within the module, ensuring that the authz keeper setup is no longer dependent on it.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AccountKeeper usage in the authz module rg --type go 'AccountKeeper' x/authzLength of output: 205
Script:
#!/bin/bash # Search for AccountKeeper usage across the entire codebase rg --type go 'AccountKeeper'Length of output: 87965
Script:
#!/bin/bash # Inspect the initialization of the authz keeper to ensure it's set up without AccountKeeper ast-grep --lang go --pattern $' func NewKeeper($$$) Keeper { $$$ } ' x/authz/keeper/keeper.goLength of output: 557
Script:
#!/bin/bash # Corrected ast-grep pattern to inspect the NewKeeper function without AccountKeeper ast-grep --lang go --pattern $' func NewKeeper($args) Keeper { // Ensure AccountKeeper is not used within this function $_ } ' x/authz/keeper/keeper.goLength of output: 677
Script:
#!/bin/bash # Alternative approach using ripgrep to search for AccountKeeper within NewKeeper function rg --type go -A 5 'func NewKeeper' x/authz/keeper/keeper.go | rg 'AccountKeeper'Length of output: 80
x/authz/keeper/genesis.go (1)
Line range hint
1-67: Overall changes look good. Consider adding tests.The modifications successfully remove the dependency on the account keeper by replacing
k.authKeeper.AddressCodec()withk.addrCdc. This change is consistent throughout the file and aligns with the PR objective. The code adheres to the Uber Golang style guide.To ensure the changes work as expected, consider adding or updating unit tests to cover the following scenarios:
- Conversion of valid addresses using the new
k.addrCdc.- Handling of invalid addresses.
- End-to-end test for
InitGenesisandExportGenesisfunctions with the new address codec.You can use the following script to check for existing tests and identify areas that might need additional coverage:
x/authz/keeper/genesis_test.go (3)
19-19: LGTM: Import statement added correctlyThe new import for
addresscodecis correctly placed and follows the Uber Golang style guide for import grouping.
Line range hint
1-97: Overall assessment: Changes successfully implement PR objectivesThe modifications to this test file successfully remove the account keeper dependency and introduce the address codec, aligning with the PR objectives. The core test logic in
TestImportExportGenesisremains unchanged, suggesting that the changes do not affect the fundamental functionality being tested.Key points:
- The
accountKeeperfield has been removed from theGenesisTestSuitestruct.- An address codec has been introduced to replace the functionality previously provided by the account keeper.
- The keeper initialization in
SetupTesthas been updated to use the new address codec.These changes maintain the integrity of the test suite while achieving the goal of removing the account keeper dependency.
38-41: LGTM: GenesisTestSuite struct updated correctlyThe
accountKeeperfield has been removed from theGenesisTestSuitestruct, aligning with the PR objective of removing the account keeper dependency. The remaining fields are unchanged and follow the Uber Golang style guide for struct field naming.To ensure the account keeper dependency has been completely removed, run the following command:
✅ Verification successful
Verified: accountKeeper dependency removed successfully
No remaining references to
accountKeeperfound inx/authz/keeper/genesis_test.go, confirming the complete removal of the account keeper dependency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to accountKeeper in the file rg 'accountKeeper' x/authz/keeper/genesis_test.goLength of output: 49
x/authz/module/abci_test.go (2)
79-81: LGTM! Proper error handling implemented.The changes correctly use the new address codec to convert the granter address to a string, and the error handling has been properly implemented. This is consistent with the PR objectives and improves the overall robustness of the code.
Line range hint
1-101: Overall assessment: Changes successfully implement PR objectives with maintained test coverage.The modifications in this file successfully remove the account keeper dependency and introduce the address codec, aligning well with the PR objectives. The core test logic remains intact, ensuring that the expiration of grants and querying of grants associated with a granter are still thoroughly tested.
The changes maintain good test coverage for the authz module's functionality, particularly:
- Saving grants with different expiration times
- Querying grants at different points in time
- Verifying the correct expiration of grants
The code adheres to the Uber Golang style guide, with clear and concise modifications that improve error handling and maintain readability.
x/authz/CHANGELOG.md (1)
Line range hint
1-61: Excellent changelog structure and contentThe changelog is well-organized and follows the guiding principles outlined at the beginning of the file. It effectively categorizes changes into appropriate sections, providing clear and concise information for each update. The consistent use of GitHub issue references is commendable, as it allows readers to easily find more detailed information if needed.
The structure and content of this changelog serve as an excellent example for maintaining clear communication about project changes. Keep up the good work!
x/authz/keeper/msg_server.go (3)
23-26: LGTM: Address conversion simplifiedThe change from
k.authKeeper.AddressCodec().StringToBytestok.addrCdc.StringToBytessimplifies the address conversion process and aligns with the PR objective of removing the account keeper dependency. The error handling remains consistent, which is good.
28-31: LGTM: Consistent address conversion for granterThis change is consistent with the previous one, applying the same simplification to the granter address conversion. The error handling remains appropriate and consistent.
67-70: LGTM: Consistent address conversion in Revoke methodThe changes in the
Revokemethod are consistent with those in theGrantmethod, applying the same address conversion simplification for both grantee and granter. The error handling remains appropriate and consistent across methods.Also applies to: 72-75
x/authz/keeper/grpc_query_test.go (5)
24-26: LGTM: Address conversion and error handling look good.The change from
suite.accountKeeper.AddressCodec().BytesToStringtosuite.addrCdc.BytesToStringis consistent with the PR objective of removing the account keeper dependency. The added error handling is a good practice for robustness.
140-141: LGTM: Consistent address conversion.This change maintains consistency with the previous modifications, using
suite.addrCdc.BytesToStringfor address conversion.
212-215: LGTM: Consistent address conversions with proper error handling.The changes maintain consistency with the previous modifications, using
suite.addrCdc.BytesToStringfor address conversions. Error handling is correctly implemented for both conversions, which is a good practice for robust code.
302-303: LGTM: Consistent address conversion in createSendAuthorizationWithAllowList.The address conversion using
suite.addrCdc.BytesToStringis consistent with the previous changes. The added error handling is appropriate and maintains code robustness.
Line range hint
1-307: Overall LGTM: Consistent changes and comprehensive test coverage.The modifications throughout the file are consistent with the PR objective of removing the account keeper dependency. The test coverage is comprehensive, including various scenarios such as error cases and pagination. The test structure follows good practices with clear test case descriptions and appropriate assertions.
Some notable points:
- Address conversions are consistently updated across all test functions.
- Error handling is added for all address conversions, improving code robustness.
- Test cases cover both success and failure scenarios, ensuring thorough validation of the GRPC query functionality.
- Pagination tests are included, which is crucial for testing real-world scenarios with large datasets.
The overall quality of the changes and test coverage is high, contributing to the maintainability and reliability of the codebase.
x/authz/keeper/msg_server_test.go (12)
31-33: LGTM: Address encoding change implemented correctlyThe change from
accountKeeper.AddressCodec()tosuite.addrCdc.BytesToString()is implemented correctly and aligns with the PR objective of removing the account keeper dependency.
45-46: LGTM: Address codec parameter added correctlyThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationis consistent with the PR objective and previous changes.
59-60: LGTM: Consistent use of address codecThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationis consistent with previous changes and the PR objective.
73-74: LGTM: Consistent address codec usageThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationmaintains consistency with previous changes and aligns with the PR objective.
102-103: LGTM: Address codec consistently appliedThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationis consistent with previous changes and the PR objective.
120-123: LGTM: Consistent address handlingThe changes in this segment, including the use of
suite.addrCdcinNewSendAuthorizationandBytesToString, are consistent with the PR objective and maintain the new address handling approach.
136-137: LGTM: Consistent address codec usageThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationmaintains consistency with previous changes and aligns with the PR objective.
148-149: LGTM: Address codec consistently appliedThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationis consistent with previous changes and the PR objective.
169-170: LGTM: Address codec and allow list correctly implementedThe changes in this segment, including the use of
suite.addrCdcinNewSendAuthorizationand the correct passing of the allow list, are consistent with the PR objective and maintain the new address handling approach.
181-182: LGTM: Consistent address codec usageThe addition of
suite.addrCdcas a parameter toNewSendAuthorizationmaintains consistency with previous changes and aligns with the PR objective.
223-225: LGTM: Consistent address encoding throughout the fileThe consistent use of
suite.addrCdc.BytesToString()for address encoding across multiple test functions (TestRevoke, TestExec, TestPruneExpiredGrants, and TestRevokeAllGrants) ensures uniform address handling throughout the file. This change aligns well with the PR objective of removing the account keeper dependency.Also applies to: 325-327, 393-395, 401-401, 445-445
Line range hint
1-501: Overall assessment: Changes successfully implement PR objectiveThe modifications in this file consistently replace
accountKeeper.AddressCodec()withsuite.addrCdc, effectively removing the account keeper dependency as intended. The changes are well-implemented across all test functions, maintaining consistency and ensuring proper address handling. The test cases have been appropriately updated to use the new address codec, which should help maintain the integrity of the authz keeper's functionality.Some points to consider:
- The changes do not introduce any new bugs or inconsistencies.
- The test coverage appears to remain comprehensive after the modifications.
- The code readability and maintainability have not been negatively impacted.
x/authz/keeper/keeper_test.go (8)
12-12: LGTM: Import changes align with PR objectivesThe addition of
"cosmossdk.io/core/address"and the renaming of the address codec import toaddresscodecare consistent with the PR's goal of removing the account keeper dependency. These changes facilitate the transition to the new address handling mechanism.Also applies to: 24-24
51-51: LGTM: TestSuite struct updated to remove account keeper dependencyThe removal of
accountKeeperand addition ofaddrCdcin theTestSuitestruct aligns with the PR's objective of eliminating the account keeper dependency. This change appropriately replaces the address encoding/decoding functionality.
79-80: LGTM: SetupTest method updated to use new address codecThe initialization of
s.addrCdcwithaddresscodec.NewBech32Codec("cosmos")correctly replaces the previousaccountKeepersetup. This change is consistent with the PR's objective and ensures that the address encoding/decoding functionality is properly set up for the tests.
184-184: LGTM: TestKeeperIter updated to use new address codecThe modification of
sendAuthzinitialization to includes.addrCdcis appropriate and consistent with the changes made to remove the account keeper dependency. This ensures that the test continues to function correctly with the new address handling mechanism.
208-208: LGTM: TestKeeperGranterGrantsIter updated to use new address codecThe modification of
sendAuthzinitialization to includes.addrCdcis consistent with the changes made throughout the file. This update ensures that the test remains functional with the new address handling mechanism.
239-245: LGTM: TestDispatchAction updated to use new address codecThe addition of address string conversions using
s.addrCdc.BytesToString()and the modification ofsendAuthzinitialization to includes.addrCdcare appropriate changes. These updates ensure that the test functions correctly with the new address handling mechanism, maintaining consistency with the PR's objectives.
403-407: LGTM: TestDispatchedEvents updated to use new address codecThe addition of address string conversions using
s.addrCdc.BytesToString()is consistent with the changes made throughout the file. These modifications ensure that the test remains functional with the new address handling mechanism, aligning with the PR's goal of removing the account keeper dependency.
Line range hint
1-605: Overall assessment: Changes successfully remove account keeper dependencyThe modifications throughout this file consistently implement the transition from using the account keeper to directly utilizing the address codec. Key points:
- Import statements have been updated to reflect the new dependencies.
- The
TestSuitestruct now includesaddrCdcinstead ofaccountKeeper.- All relevant test functions have been updated to use the new address codec for string conversions and authorization creation.
- The changes maintain test coverage while adapting to the new address handling mechanism.
These updates successfully achieve the PR's objective of removing the account keeper dependency in the authz keeper tests.
simapp/app.go (1)
401-401: LGTM: AuthzKeeper initialization updated to remove account keeper dependency.The change from
app.AuthKeepertosigningCtx.AddressCodec()in the AuthzKeeper initialization aligns well with the PR objective of removing the account keeper dependency. This modification enhances modularity by using a more specific address encoding mechanism.x/authz/keeper/keeper.go (3)
12-12: Import statement added correctlyThe addition of
"cosmossdk.io/core/address"is appropriate for the use ofaddress.Codecin the code.
34-35: Struct updated with new fieldaddrCdcThe
Keeperstruct now includes theaddrCdc address.Codecfield, which is correctly declared alongsidecdc codec.Codec.
39-43:NewKeeperfunction signature updated appropriatelyThe
NewKeeperfunction signature has been updated to acceptaddrCdc address.Codecinstead ofauthz.AccountKeeper. The initialization within the function correctly assignsaddrCdcto theKeeperstruct.
| granterAddr, err := k.addrCdc.BytesToString(granter) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive address conversions into a helper method
The address conversion using k.addrCdc.BytesToString is repeated multiple times. To enhance maintainability and reduce duplication, consider creating a helper method for this operation.
Define the following helper method in the Keeper struct (outside the selected line ranges):
func (k Keeper) addressToString(addr sdk.AccAddress) (string, error) {
return k.addrCdc.BytesToString(addr)
}Then, replace the repeated code with calls to this helper method. For example, at line 210:
- granterAddr, err := k.addrCdc.BytesToString(granter)
+ granterAddr, err := k.addressToString(granter)
if err != nil {
return err
}Apply this change to all similar occurrences where address conversion is performed.
Also applies to: 214-216, 233-235, 238-240, 259-261, 263-265, 295-297
| granterAddr, err := k.addrCdc.BytesToString(granter) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(authz.ErrNoAuthorizationFound, | ||
| "could not convert granter address to string") | ||
| } | ||
| granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee) | ||
| granteeAddr, err := k.addrCdc.BytesToString(grantee) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(authz.ErrNoAuthorizationFound, |
There was a problem hiding this comment.
Return appropriate error when address conversion fails
Wrapping the error with ErrNoAuthorizationFound when address conversion fails might obscure the actual issue. It's better to return the underlying error to provide clearer information for debugging.
Apply this diff to return the actual error:
granterAddr, err := k.addrCdc.BytesToString(granter)
if err != nil {
- return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
- "could not convert granter address to string")
+ return err
}
granteeAddr, err := k.addrCdc.BytesToString(grantee)
if err != nil {
- return errorsmod.Wrapf(authz.ErrNoAuthorizationFound,
- "could not convert grantee address to string")
+ return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| granterAddr, err := k.addrCdc.BytesToString(granter) | |
| if err != nil { | |
| return errorsmod.Wrapf(authz.ErrNoAuthorizationFound, | |
| "could not convert granter address to string") | |
| } | |
| granteeAddr, err := k.authKeeper.AddressCodec().BytesToString(grantee) | |
| granteeAddr, err := k.addrCdc.BytesToString(grantee) | |
| if err != nil { | |
| return errorsmod.Wrapf(authz.ErrNoAuthorizationFound, | |
| granterAddr, err := k.addrCdc.BytesToString(granter) | |
| if err != nil { | |
| return err | |
| } | |
| granteeAddr, err := k.addrCdc.BytesToString(grantee) | |
| if err != nil { | |
| return err | |
| } |
| } | ||
|
|
||
| // NewKeeper constructs a message authorization Keeper | ||
| func NewKeeper(env appmodule.Environment, cdc codec.Codec, ak authz.AccountKeeper) Keeper { | ||
| func NewKeeper(env appmodule.Environment, cdc codec.Codec, addrCdc address.Codec) Keeper { |
There was a problem hiding this comment.
💡 Codebase verification
Multiple NewKeeper invocations have not been updated to include addrCdc address.Codec as the third parameter:
- Instances in various test files and modules (e.g.,
orm/model/ormdb/module_test.go,x/upgrade/keeper/keeper_test.go,x/authz/keeper/genesis_test.go, etc.) are missing the updated parameter. - This inconsistency may lead to potential issues in the application's authorization mechanisms.
Recommended Actions:
- Update all identified
NewKeepercalls to includeaddrCdc address.Codecas the third argument. - Review and test the changes to ensure compatibility and correct functionality across the codebase.
🔗 Analysis chain
Verify all NewKeeper invocations are updated
Ensure that all calls to NewKeeper in the codebase have been updated to match the new signature accepting addrCdc address.Codec as the third parameter.
Run the following script to find all invocations of NewKeeper:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all invocations of 'NewKeeper' in Go files.
# Command: Find all instances of 'NewKeeper(' in Go files.
rg --type go 'NewKeeper\('
# Expected result: All calls should use 'addrCdc address.Codec' as the third argument.
Length of output: 19562
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632)
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632) chore(contributing): delete link (#21990) test(gov): Migrate e2e to system test (#21927) test: e2e/client to system tests (#21981)
Description
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
RevokeAllmethod for revoking all grants simultaneously.Improvements
Bug Fixes
Documentation
x/authzmodule.