refactor(x/authz)!: remove Accounts.String() #19783
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThe recent updates focus on enhancing the 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? 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.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (15)
- x/authz/CHANGELOG.md (1 hunks)
- x/authz/client/cli/tx.go (3 hunks)
- x/authz/client/cli/tx_test.go (22 hunks)
- x/authz/keeper/genesis.go (1 hunks)
- x/authz/keeper/genesis_test.go (1 hunks)
- x/authz/keeper/grpc_query_test.go (16 hunks)
- x/authz/keeper/keeper.go (3 hunks)
- x/authz/keeper/keeper_test.go (8 hunks)
- x/authz/keeper/msg_server_test.go (23 hunks)
- x/authz/module/abci_test.go (2 hunks)
- x/authz/module/module.go (1 hunks)
- x/authz/msgs.go (3 hunks)
- x/authz/msgs_test.go (1 hunks)
- x/authz/simulation/genesis.go (4 hunks)
- x/authz/simulation/operations.go (5 hunks)
Additional comments: 63
x/authz/keeper/genesis.go (2)
- 41-67: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [12-33]
The changes in
InitGenesiscorrectly handle address conversion and error propagation. Good use ofAddressCodec().StringToBytesfor converting address strings to bytes.
- 44-66: The changes in
ExportGenesiscorrectly handle address conversion from bytes to strings and error propagation. Proper use ofAddressCodec().BytesToStringfor address conversion.x/authz/simulation/genesis.go (2)
- 27-37: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [20-34]
The changes in
genGrantcorrectly handle address conversion from bytes to strings using the passedaddress.Codec. Proper use ofBytesToStringfor address conversion.
- 65-65: The changes in
RandomizedGenStatecorrectly passsimState.AddressCodectogenGrant, ensuring consistent address conversion.x/authz/msgs.go (3)
- 23-26: The changes to
NewMsgGrantcorrectly updategranterandgranteeto use string types for addresses, aligning with the broader refactor.- 74-77: The changes to
NewMsgRevokecorrectly updategranterandgranteeto use string types for addresses.- 71-86: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [83-95]
The changes to
NewMsgExeccorrectly updategranteeto use a string type for the address.x/authz/keeper/genesis_test.go (1)
- 84-97: The changes in
TestImportExportGenesiscorrectly handle errors returned byExportGenesisandInitGenesis, ensuring proper testing of genesis state import and export operations.x/authz/module/abci_test.go (1)
- 71-78: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-91]
The changes in
TestExpiredGrantsQueuecorrectly use the address codec to convert addresses to strings for error messages and query parameters, aligning with the broader refactor.x/authz/module/module.go (1)
- 137-140: The changes in
ExportGenesiscorrectly handle errors returned byam.keeper.ExportGenesis(ctx), improving error handling in the module's genesis export process.x/authz/msgs_test.go (1)
- 75-75: The changes in
TestAminoJSONcorrectly update the method used for address conversion tovalAddressCodec.StringToBytes, ensuring consistency in address handling.x/authz/keeper/grpc_query_test.go (3)
- 24-27: The conversion of addresses from bytes to string using
BytesToStringis correctly implemented. However, ensure that theAddressCodecis properly mocked or initialized in the test setup to avoid potential nil pointer dereferences during tests.- 47-47: The changes to use string representations for
GranterandGranteein various requests align with the PR's objective to standardize address handling. This approach simplifies the handling of addresses across the module. Ensure that all corresponding functions and methods that consume these addresses are updated to work with string types instead ofsdk.AccAddress.Also applies to: 57-58, 69-70, 82-83, 103-104, 140-141, 164-164, 175-175, 185-185, 213-214, 239-239, 248-248, 259-259, 268-268
- 305-308: The conversion of an address to a string for inclusion in the
AllowListof aSendAuthorizationis correctly implemented. This change is consistent with the overall goal of standardizing address handling. Just ensure that theAllowListis properly utilized in all relevant authorization checks.x/authz/client/cli/tx.go (3)
- 70-73: The conversion of the grantee address from bytes to string using
BytesToStringwithinNewCmdExecAuthorizationis correctly implemented. This change is necessary for the updated handling of addresses. Ensure that error handling is consistent and informative throughout the conversion process.- 112-114: The validation logic for grantee and granter addresses within
NewCmdGrantAuthorizationhas been updated to use theAddressCodecfor conversion and comparison. This is a good practice as it ensures that the addresses are in a valid format before proceeding with the authorization grant. Additionally, the check to ensure that grantee and granter are different addresses is crucial for avoiding self-grants, which could lead to unintended permissions escalation.Also applies to: 117-124
- 241-241: The creation of
MsgGrantwithinNewCmdGrantAuthorizationcorrectly uses the updated parameters, including the converted grantee and granter addresses. This change aligns with the PR's objective to standardize address handling. Ensure that theMsgGrantconstructor's error handling is robust to catch any issues with the provided arguments.x/authz/simulation/operations.go (3)
- 126-133: The conversion of granter and grantee addresses from bytes to string within
SimulateMsgGrantis correctly implemented. This change is necessary for the updated handling of addresses in simulations. Ensure that the simulation framework and any related components are properly updated to handle string addresses.- 216-223: The conversion of granter and grantee addresses from bytes to string within
SimulateMsgRevokeis correctly implemented. This standardizes address handling in line with the PR's objectives. Ensure that all related simulation components are updated accordingly.- 326-326: The use of string addresses in
SimulateMsgExecfor the creation ofMsgExecis consistent with the changes across the module. This approach simplifies the handling of addresses in simulations. Ensure that theMsgExechandling and execution logic are fully compatible with string addresses.x/authz/keeper/msg_server_test.go (29)
- 39-42: The conversion of
sdk.AccAddressto string usingBytesToStringis correctly implemented for bothgranterandgrantee. This aligns with the PR's objective to standardize address handling.- 56-57: Using string representations for
GranterandGranteeinauthz.MsgGrantis consistent with the changes made across the module for address handling. This ensures that the tests reflect the updated logic in the main codebase.- 71-71: Correctly using the string representation for
Granteein the test case "invalid granter" ensures that the test accurately reflects the expected behavior with the updated address handling.- 84-84: The use of string representation for
Granterin the "invalid grantee" test case is consistent with the module's updated address handling approach. This change is necessary for the tests to remain valid and effective.- 96-97: The test case for "invalid grant" correctly uses string representations for both
GranterandGrantee. This consistency is crucial for ensuring that the tests accurately reflect the changes made in the module.- 113-114: In the "invalid grant, past time" test case, using string representations for addresses aligns with the module's updated logic. This ensures that the tests are in sync with the main codebase's approach to address handling.
- 134-140: The test case for "grantee account does not exist on chain: valid grant" correctly converts a new account address to a string representation. This demonstrates the flexibility and correctness of the updated address handling in test scenarios.
- 151-152: Using string representations for
GranterandGranteein the "valid grant" test case is appropriate and aligns with the module's updated address handling logic. This ensures that the tests accurately reflect the changes.- 163-173: The test case "valid grant, same grantee, granter pair but different msgType" correctly uses string representations for addresses. This consistency across test cases is crucial for validating the updated logic in the module.
- 184-185: In the "valid grant with allow list" test case, using string representations for addresses ensures that the test is aligned with the module's updated address handling approach. This change is necessary for the tests to remain relevant.
- 196-197: The "valid grant with nil expiration time" test case correctly uses string representations for addresses. This consistency across test cases is important for ensuring that the tests accurately reflect the updated module logic.
- 221-224: The conversion of
sdk.AccAddressto string usingBytesToStringfor bothgranterandgranteein theTestRevokefunction is correctly implemented. This aligns with the PR's objective to standardize address handling in tests.- 236-237: Using string representations for
GranterandGranteein theauthz.MsgRevokestruct within the "identical grantee and granter" test case is consistent with the changes made across the module. This ensures that the tests reflect the updated logic.- 249-249: Correctly using the string representation for
Granteein the "invalid granter" test case of theTestRevokefunction ensures that the test accurately reflects the expected behavior with the updated address handling.- 260-260: The use of string representation for
Granterin the "invalid grantee" test case of theTestRevokefunction is consistent with the module's updated address handling approach. This change is necessary for the tests to remain valid and effective.- 272-273: In the "no msg given" test case of the
TestRevokefunction, using string representations for addresses aligns with the module's updated logic. This ensures that the tests are in sync with the main codebase's approach to address handling.- 286-287: Using string representations for
GranterandGranteein the "valid grant" test case of theTestRevokefunction is appropriate and aligns with the module's updated address handling logic. This ensures that the tests accurately reflect the changes.- 296-297: The "no existing grant to revoke" test case in the
TestRevokefunction correctly uses string representations for bothGranterandGrantee. This consistency is crucial for ensuring that the tests accurately reflect the changes made in the module.- 323-326: The conversion of
sdk.AccAddressto string usingBytesToStringfor bothgranterandgranteein theTestExecfunction is correctly implemented. This aligns with the PR's objective to standardize address handling in tests.- 330-331: In the
TestExecfunction, using string representations forFromAddressandToAddressin thebanktypes.MsgSendstruct is consistent with the changes made across the module. This ensures that the tests reflect the updated logic.- 344-344: The "invalid grantee (empty)" test case in the
TestExecfunction correctly uses an empty string for the grantee address. This test case is important for ensuring that the updated logic properly handles edge cases.- 352-352: Using the string representation for the grantee address in the "non existing grant" test case of the
TestExecfunction aligns with the module's updated address handling approach. This ensures that the test remains relevant and effective.- 360-360: In the "no message case" test case of the
TestExecfunction, using the string representation for the grantee address is consistent with the updated logic in the module. This change is necessary for the tests to accurately reflect the main codebase's approach.- 369-369: The "valid case" test case in the
TestExecfunction correctly uses string representations for addresses. This consistency across test cases is crucial for validating the updated logic in the module.- 391-394: The conversion of
sdk.AccAddressto string usingBytesToStringfor addresses in theTestPruneExpiredGrantsfunction is correctly implemented. This aligns with the PR's objective to standardize address handling in tests.- 403-404: In the
TestPruneExpiredGrantsfunction, using string representations forGranterandGranteein theauthz.MsgGrantstruct is consistent with the changes made across the module. This ensures that the tests reflect the updated logic.- 410-411: Correctly using the string representation for addresses in the second
authz.MsgGrantstruct within theTestPruneExpiredGrantsfunction ensures that the test accurately reflects the expected behavior with the updated address handling.- 417-419: The iteration over grants in the
TestPruneExpiredGrantsfunction correctly counts the total grants without any changes to the logic. This part of the test remains unaffected by the address handling updates and is correctly implemented.- 428-434: The pruning of expired grants in the
TestPruneExpiredGrantsfunction is correctly implemented. The test accurately reflects the updated logic in the module, ensuring that expired grants are properly handled.x/authz/keeper/keeper.go (3)
- 200-208: The conversion of
sdk.AccAddressto string usingBytesToStringfor bothgranterandgranteein theSaveGrantfunction is correctly implemented. This aligns with the PR's objective to standardize address handling and ensures that events are emitted with the correct address format.- 238-245: In the
DeleteGrantfunction, convertingsdk.AccAddressto string for bothgranterandgranteebefore emitting theEventRevokeis correctly done. This change is necessary for the events to reflect the updated address handling logic.- 302-319: The update to the
IterateGrantsfunction signature to include an error in the return type of the handler function is a good practice. It allows for better error handling and propagation, aligning with the PR's objective to enhance error reporting in the module.x/authz/keeper/keeper_test.go (4)
- 158-161: The
IterateGrantsfunction now correctly handles errors, which is a significant improvement in error handling. However, it's important to ensure that all callers of this function properly handle the returned error to avoid ignoring potential issues during iteration.- 172-177: Converting addresses from bytes to strings using
s.accountKeeper.AddressCodec().BytesToStringis a good practice for consistency and readability. Ensure that error handling is consistently applied wherever address conversions are performed to prevent runtime panics.- 190-194: Using string representations for addresses in message creation is consistent with the changes made throughout the module. This approach simplifies address handling and aligns with the broader refactor. Ensure that all message creation and handling throughout the tests and module correctly use the string representation.
- 322-327: The conversion of addresses to strings before using them in message creation is correctly implemented here. This change enhances consistency in how addresses are handled across the module. It's crucial to ensure that all parts of the codebase that interact with addresses follow this pattern to maintain consistency.
x/authz/client/cli/tx_test.go (7)
- 83-84: Converting the validator address to a string using
s.baseCtx.AddressCodec.BytesToStringis consistent with the module's changes. This ensures that addresses are handled uniformly across the module. It's important to apply this conversion consistently and handle any potential errors.- 98-99: The conversion of grantee addresses to strings before using them in authorization commands is correctly implemented. This change aligns with the broader refactor of address handling within the module. Consistent error handling for address conversions is crucial to avoid runtime issues.
- 119-120: Correctly converting grantee addresses to strings for use in authorization commands is a good practice. This consistency in address handling simplifies the code and aligns with the module's refactor. Ensure that error handling for address conversions is consistently applied.
- 138-139: The conversion of grantee addresses to strings before using them in authorization commands is correctly implemented here. This approach enhances consistency in address handling across the module. It's important to ensure that error handling for address conversions is consistently applied to prevent issues.
- 181-187: Using string representations for addresses in the
MsgSendmessage creation is consistent with the changes made throughout the module. This simplifies address handling and aligns with the broader refactor. Ensure that all message creation and handling throughout the tests and module correctly use the string representation.- 197-200: Converting addresses to strings using
s.clientCtx.ValidatorAddressCodec.BytesToStringands.baseCtx.AddressCodec.BytesToStringis a good practice for consistency and readability. Ensure that error handling is consistently applied wherever address conversions are performed to prevent runtime panics.- 203-204: The conversion of grantee addresses to strings before using them in test cases is correctly implemented. This change aligns with the broader refactor of address handling within the module. Consistent error handling for address conversions is crucial to avoid runtime issues.
likhita-809
approved these changes
Mar 20, 2024
facundomedica
approved these changes
Mar 20, 2024
This was referenced Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
ref:
#13140
#7448
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.
I have...
Summary by CodeRabbit
NewKeeper.