refactor(x/bank)!: remove Address.String() #19954
Conversation
github-actions
bot
added
C:CLI
C:Simulations
C:x/bank
C:x/genutil
WalkthroughWalkthroughThe changes involve significant improvements in address handling across various files, emphasizing the transition from direct address objects to string representations. These enhancements include introducing error handling for address conversions, utilizing Changes
Possibly related issues
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.
Actionable comments posted: 35
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (37)
- simapp/simd/cmd/testnet.go (1 hunks)
- tests/integration/bank/app_test.go (9 hunks)
- tests/integration/bank/bench_test.go (3 hunks)
- tests/integration/bank/keeper/deterministic_test.go (6 hunks)
- tests/sims/authz/operations_test.go (2 hunks)
- types/query/pagination_test.go (19 hunks)
- x/accounts/defaults/lockup/lockup.go (1 hunks)
- x/authz/client/cli/tx.go (1 hunks)
- x/authz/keeper/keeper_test.go (3 hunks)
- x/authz/keeper/msg_server_test.go (10 hunks)
- x/authz/migrations/v2/store_test.go (1 hunks)
- x/authz/module/abci_test.go (1 hunks)
- x/authz/msgs_test.go (1 hunks)
- x/authz/simulation/decoder_test.go (1 hunks)
- x/authz/simulation/genesis.go (1 hunks)
- x/authz/simulation/operations.go (3 hunks)
- x/bank/CHANGELOG.md (1 hunks)
- x/bank/client/cli/tx.go (2 hunks)
- x/bank/keeper/collections_test.go (1 hunks)
- x/bank/keeper/genesis.go (2 hunks)
- x/bank/keeper/genesis_test.go (3 hunks)
- x/bank/keeper/grpc_query.go (1 hunks)
- x/bank/keeper/grpc_query_test.go (9 hunks)
- x/bank/keeper/keeper_test.go (22 hunks)
- x/bank/keeper/msg_server_test.go (22 hunks)
- x/bank/simulation/genesis.go (2 hunks)
- x/bank/simulation/operations.go (6 hunks)
- x/bank/simulation/proposals.go (1 hunks)
- x/bank/simulation/proposals_test.go (2 hunks)
- x/bank/types/balance.go (4 hunks)
- x/bank/types/balance_test.go (4 hunks)
- x/bank/types/inputs_outputs.go (2 hunks)
- x/bank/types/msgs_test.go (4 hunks)
- x/bank/types/querier.go (2 hunks)
- x/bank/types/send_authorization.go (3 hunks)
- x/bank/types/send_authorization_test.go (9 hunks)
- x/genutil/genaccounts.go (1 hunks)
Additional Context Used
Path-based Instructions (37)
x/bank/simulation/proposals.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/simulation/proposals_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/simulation/decoder_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/bank/types/inputs_outputs.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/types/querier.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/simulation/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/bank/types/send_authorization.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/types/balance.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/keeper/collections_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/bank/client/cli/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/migrations/v2/store_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/bank/simulation/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/types/send_authorization_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"tests/integration/bank/bench_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/genutil/genaccounts.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/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/bank/types/msgs_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/bank/types/balance_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/msgs_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"tests/sims/authz/operations_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/client/cli/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/simulation/operations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/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/bank/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/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/bank/simulation/operations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/bank/app_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/query/pagination_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"tests/integration/bank/keeper/deterministic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"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/accounts/defaults/lockup/lockup.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/simd/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/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/bank/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"
Learnings (4)
x/bank/types/balance.go (1)
User: testinginprod" PR: cosmos/cosmos-sdk#18542 File: x/bank/types/balance.go:64-77 Timestamp: 2023-11-22T14:48:50.376Z Learning: - The `SanitizeGenesisBalances` function in `x/bank/types/balance.go` of the Cosmos SDK should panic when encountering duplicate account addresses in the genesis state. - A test case is required to verify the behavior of `SanitizeGenesisBalances` when duplicates are present.x/bank/simulation/genesis.go (1)
User: testinginprod" PR: cosmos/cosmos-sdk#18542 File: x/bank/types/balance.go:64-77 Timestamp: 2023-11-22T14:48:50.376Z Learning: - The `SanitizeGenesisBalances` function in `x/bank/types/balance.go` of the Cosmos SDK should panic when encountering duplicate account addresses in the genesis state. - A test case is required to verify the behavior of `SanitizeGenesisBalances` when duplicates are present.x/genutil/genaccounts.go (1)
User: testinginprod" PR: cosmos/cosmos-sdk#18542 File: x/bank/types/balance.go:64-77 Timestamp: 2023-11-22T14:48:50.376Z Learning: - The `SanitizeGenesisBalances` function in `x/bank/types/balance.go` of the Cosmos SDK should panic when encountering duplicate account addresses in the genesis state. - A test case is required to verify the behavior of `SanitizeGenesisBalances` when duplicates are present.x/bank/types/balance_test.go (1)
User: testinginprod" PR: cosmos/cosmos-sdk#18542 File: x/bank/types/balance.go:64-77 Timestamp: 2023-11-22T14:48:50.376Z Learning: - The `SanitizeGenesisBalances` function in `x/bank/types/balance.go` of the Cosmos SDK should panic when encountering duplicate account addresses in the genesis state. - A test case is required to verify the behavior of `SanitizeGenesisBalances` when duplicates are present.
Additional comments not posted (80)
x/bank/simulation/proposals_test.go (1)
18-19: Initializeacwith a more robust approach or ensure thatcodectestutil.CodecOptions{}.GetAddressCodec()is the best method for obtaining the address codec in this context.x/authz/simulation/decoder_test.go (1)
30-30: Consider validating the result ofcodectestutil.CodecOptions{}.GetAddressCodec()before using it to ensure it meets the expected criteria.x/bank/types/inputs_outputs.go (2)
54-56: Ensure that the address string passed toNewInputis validated or sanitized before use to prevent potential security issues.
79-81: Ensure that the address string passed toNewOutputis validated or sanitized before use to prevent potential security issues.x/bank/keeper/genesis.go (2)
16-17: Direct assignment oferrfromk.SetParamsis a good practice for error handling. Ensure that all similar instances in the codebase follow this pattern for consistency.
26-29: Properly handling errors fromtypes.SanitizeGenesisBalancesis crucial. Ensure that the error messages are clear and informative for easier debugging.x/bank/types/querier.go (3)
16-17: Ensure that the address string passed toNewQueryBalanceRequestis validated or sanitized before use to prevent potential security issues.
21-22: Ensure that the address string passed toNewQueryAllBalancesRequestis validated or sanitized before use to prevent potential security issues.
27-28: Ensure that the address string passed toNewQuerySpendableBalancesRequestis validated or sanitized before use to prevent potential security issues.x/authz/simulation/genesis.go (1)
43-45: PassingaddressCodectogenerateRandomGrantis a good practice for consistency in address handling. Ensure that the codec is used correctly within the function.x/bank/CHANGELOG.md (1)
38-42: Ensure that the changelog entries are clear and accurately reflect the changes made in the PR. Specifically, verify that all function signature changes and the addition of address codec arguments are correctly documented.x/bank/types/send_authorization.go (1)
19-21: Consider renaming theaddressCodecparameter to more accurately reflect its purpose, such asaddrCodec, to maintain consistency with common naming conventions in the Cosmos SDK.x/bank/types/balance.go (1)
69-78: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [57-86]
Ensure that the
SanitizeGenesisBalancesfunction properly handles duplicate account addresses by returning an error instead of panicking. This aligns with the best practice of avoiding panic in library code and allows calling code to handle errors gracefully.- panic(fmt.Sprintf("genesis state has a duplicate account: %q aka %x", balances[i].Address, addr)) + return nil, fmt.Errorf("genesis state has a duplicate account: %q aka %x", balances[i].Address, addr)Skipped due to learnings
User: testinginprod" PR: cosmos/cosmos-sdk#18542 File: x/bank/types/balance.go:64-77 Timestamp: 2023-11-22T14:48:50.376Z Learning: - The `SanitizeGenesisBalances` function in `x/bank/types/balance.go` of the Cosmos SDK should panic when encountering duplicate account addresses in the genesis state. - A test case is required to verify the behavior of `SanitizeGenesisBalances` when duplicates are present.x/bank/client/cli/tx.go (1)
84-89: Verify the necessity of convertingargto bytes and then back to string in theNewMultiSendTxCmdfunction. Ifargis already in the correct format, this conversion might be redundant.x/authz/migrations/v2/store_test.go (1)
45-45: Ensure that theaddressCodecused in theNewSendAuthorizationcall is properly initialized and represents a realistic scenario for the test. Consider using a mock or a standard codec implementation that reflects the actual usage.x/authz/module/abci_test.go (1)
57-57: Ensure that theaddressCodecused in theNewSendAuthorizationcall is properly initialized and represents a realistic scenario for the test. Consider using a mock or a standard codec implementation that reflects the actual usage.x/bank/types/send_authorization_test.go (1)
29-33: Ensure that theaddressCodecused in theNewSendAuthorizationcall is properly initialized and represents a realistic scenario for the test. Consider using a mock or a standard codec implementation that reflects the actual usage.x/genutil/genaccounts.go (1)
134-136: Properly handle errors frombanktypes.SanitizeGenesisBalancesby returning the error to the caller.- bankGenState.Balances, err = banktypes.SanitizeGenesisBalances(bankGenState.Balances, addressCodec) - if err != nil { - return fmt.Errorf("failed to sanitize genesis balance: %w", err) - }This change correctly introduces error handling for the sanitization process, ensuring that any issues encountered during this step are not silently ignored.
x/authz/msgs_test.go (1)
72-72: The addition ofcodectestutil.CodecOptions{}.GetValidatorCodec()as an argument inNewSendAuthorizationis consistent with the need for a codec in the authorization setup. This change ensures that the test setup aligns with the updated requirements for authorization creation.tests/sims/authz/operations_test.go (2)
172-172: Ensuresuite.accountKeeper.AddressCodec()is properly initialized and consistently used throughout the test suite for address handling.
203-203: Ensuresuite.accountKeeper.AddressCodec()is properly initialized and consistently used throughout the test suite for address handling.x/authz/client/cli/tx.go (1)
162-162: EnsureclientCtx.AddressCodecis properly initialized and consistently used across CLI commands for address handling.Verification successful
The review comment is verified. The script output confirms that
clientCtx.AddressCodecis properly initialized and consistently used across CLI commands for address handling withinx/authz/client/cli/tx.go.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the initialization of AddressCodec in the CLI setup rg "AddressCodec" x/authz/client/cli/tx.go # Ensure consistent use of AddressCodec for address handling in CLI commands rg "AddressCodec" x/authz/client/cli/tx.goLength of output: 2085
x/authz/simulation/operations.go (2)
125-125: Ensure thegenerateRandomAuthorizationfunction's newaddressCodecparameter is properly documented.
Adding documentation for theaddressCodecparameter in thegenerateRandomAuthorizationfunction will help maintainers and developers understand its purpose and usage.
162-164: Validate theaddressCodecparameter ingenerateRandomAuthorization.
Before using theaddressCodecparameter within thegenerateRandomAuthorizationfunction, ensure it is validated to prevent potential issues if a nil or invalid codec is passed. This is a precautionary measure to enhance the robustness of the code.x/authz/keeper/msg_server_test.go (1)
399-399: Ensure theNewGrantfunction's newaddressCodecparameter is properly documented.
Adding documentation for theaddressCodecparameter in theNewGrantfunction will help maintainers and developers understand its purpose and usage. This is crucial for ensuring that the refactor's intent and the parameter's role are clear.tests/integration/bank/app_test.go (4)
195-199: Validate the use of string addresses inMsgMultiSendcreation aligns with the updated address handling approach.
211-215: Validate the use of string addresses inMsgMultiSendcreation within test cases, ensuring consistency with the new address handling strategy.
223-229: Confirm that the conversion of module account addresses to strings and their use inMsgMultiSendis correctly implemented and aligns with the refactor objectives.
237-241: Ensure that the test case "multiple inputs not allowed" correctly reflects the updated logic and address handling after the refactor.types/query/pagination_test.go (10)
129-129: Validate the use of string addresses inQueryAllBalancesRequestaligns with the updated address handling approach.
138-138: Confirm that the conversion of addresses to strings and their use inQueryAllBalancesRequestis correctly implemented and aligns with the refactor objectives.
147-147: Ensure that the test case "paginate with custom limit and countTotal true" correctly reflects the updated logic and address handling after the refactor.
156-156: Validate the use of string addresses inQueryAllBalancesRequestwithin test cases, ensuring consistency with the new address handling strategy.
165-165: Confirm that the conversion of addresses to strings and their use inQueryAllBalancesRequestfor pagination with custom limit, key, and countTotal false is correctly implemented.
174-174: Ensure that the test case "paginate for last page" correctly reflects the updated logic and address handling after the refactor.
184-184: Validate the use of string addresses inQueryAllBalancesRequestfor pagination with offset and limit aligns with the updated address handling approach.
194-194: Confirm that the conversion of addresses to strings and their use inQueryAllBalancesRequestfor pagination with offset and limit is correctly implemented.
203-203: Ensure error handling is consistent and informative when both offset and key are provided inQueryAllBalancesRequest, which is an invalid request scenario.
210-210: Validate the handling of pagination with offset greater than total results inQueryAllBalancesRequest, ensuring it aligns with expectations.tests/integration/bank/keeper/deterministic_test.go (1)
172-172: Consider caching theaddressCodecinstance for reuse across tests to improve performance.x/authz/keeper/keeper_test.go (2)
150-150: The update to includeAddressCodecin theSendAuthorizationcreation aligns well with the PR's objectives of standardizing address handling. Good job on this change.
178-178: IncludingAddressCodecin theSendAuthorizationcreation here is consistent with the PR's goal of enhancing address handling. This is a positive step towards standardizing how addresses are managed.x/accounts/defaults/lockup/lockup.go (1)
463-463: The update to callNewQueryBalanceRequestwithout thesdk.AccAddresstype conversion for thesenderparameter simplifies the code and aligns with the PR's goal of standardizing address handling. This is a positive change.x/bank/keeper/grpc_query_test.go (14)
14-14: Importcodectestutilto facilitate address to string conversion in tests.
This import is necessary for the updated test cases that now require address to string conversion using thecodectestutilpackage, aligning with the PR's objective to handle addresses as strings.
24-26: Convert the address to a string usingcodectestutilfor query requests.
This conversion is part of the refactor to handle addresses as strings, ensuring that the test cases are updated to reflect the changes in how addresses are managed within the SDK.
45-45: Use the string representation of the address in theNewQueryBalanceRequest.
This change is consistent with the PR's objective to standardize address handling by using strings instead of direct address objects.
51-51: Pass an empty string as the address inNewQueryBalanceRequestto test error handling for empty address strings.
This test case validates the SDK's error handling when an empty string is passed as an address, aligning with the changes to use string representations of addresses.
63-63: Query balance with a valid address string but missing denom to test error handling.
This test case checks the SDK's behavior when a denom is not provided, ensuring that the error handling works as expected with the new string-based address handling.
69-69: Query balance with a valid address string and denom, expecting an empty result.
This test case ensures that querying with a valid address string and denom works as expected, even when the result is empty, demonstrating the SDK's ability to handle string-based addresses.
77-77: Query balance with a valid address string and denom, expecting a specific result.
This test case confirms that querying with a valid address string and denom returns the correct balance, aligning with the PR's goal to standardize address handling as strings.
110-112: Convert the address to a string usingcodectestutilfor querying all balances.
This conversion is necessary for the test case that queries all balances using the string representation of the address, reflecting the PR's changes to address handling.
118-118: Query all balances with a valid address string and pagination request.
This change updates the test case to use a string representation of the address, ensuring it aligns with the SDK's new approach to address handling.
147-147: Query the second page of all balances using a valid address string and updated pagination request.
This test case demonstrates the SDK's ability to handle paginated queries for all balances using string-based addresses, in line with the PR's objectives.
161-161: Query the third page of all balances with a valid address string, testing the SDK's pagination handling with string-based addresses.
This test case further validates the SDK's support for paginated queries using string representations of addresses, consistent with the PR's changes.
173-173: Query the third page of all balances with address string and resolve IBC denom, testing the SDK's handling of IBC denoms with string-based addresses.
This test case checks the SDK's ability to resolve IBC denoms in paginated queries using string representations of addresses, aligning with the PR's goals.
183-184: Convert the address to a string usingcodectestutilfor querying spendable balances.
This conversion is part of the update to handle addresses as strings in test cases, ensuring consistency with the PR's changes to address management.
198-198: Query spendable balances with a valid address string and pagination request.
This test case updates the query for spendable balances to use a string representation of the address, reflecting the SDK's new approach to address handling.x/bank/keeper/keeper_test.go (23)
137-142: Ensure the address codec is properly initialized and used for address conversions.
153-154: Verify that the map initialization withaddr: trueis correctly used and serves the intended purpose within the test setup.Verification successful
The map initialization with
addr: trueis used in the construction of aNewBaseKeeperobject in bothcollections_test.goandkeeper_test.go, suggesting it serves a specific purpose related to the test setup, likely concerning permissions or access control. The consistent use across different test files and the context provided by the surrounding code indicate that the initialization is correctly used and serves the intended purpose within the test setup.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of the initialized map in tests to ensure it's utilized as intended. grep -R "map\[string]bool{addr: true}" .Length of output: 171
Script:
#!/bin/bash # Examine the usage context of the map initialization in collections_test.go and keeper_test.go grep -C 5 "map\[string]bool{addr: true}" ./x/bank/keeper/collections_test.go grep -C 5 "map\[string]bool{addr: true}" ./x/bank/keeper/keeper_test.goLength of output: 597
319-322: Ensure error handling is properly implemented for address conversion failures inTestGetAuthority.
326-327: Confirm that the test cases inTestGetAuthorityaccurately reflect the intended functionality and cover all necessary scenarios.
643-647: InTestInputOutputNewAccount, ensure that address conversion is correctly handled and that errors are properly managed.
650-653: Check that theInputandOutputstructs are correctly populated with string addresses and that the test logic aligns with the expected behavior.
670-676: InTestInputOutputCoins, verify that address conversion is correctly handled and that errors are properly managed.
678-682: Ensure that theInputandOutputstructs inTestInputOutputCoinsare correctly populated and that the test accurately reflects the intended functionality.
782-791: InTestInputOutputCoinsWithRestrictions, ensure that address conversion is correctly handled and that errors are properly managed.
810-810: Verify that theOutputstruct is correctly populated with string addresses inTestInputOutputCoinsWithRestrictions.
822-822: Ensure that theOutputstruct is correctly populated with string addresses in the passthrough restriction test case.
842-842: Check that theOutputstruct is correctly populated with string addresses in the new to restriction test case.
862-862: In the error restriction test case, ensure that theOutputstruct is correctly populated with string addresses.
884-885: Verify that theOutputstructs are correctly populated with string addresses in the passthrough restriction two outputs test case.
913-914: Check that theOutputstructs are correctly populated with string addresses in the error restriction two outputs test case.
943-944: Ensure that theOutputstructs are correctly populated with string addresses in the new to restriction two outputs test case.
1364-1368: InTestMsgSendEvents, verify that address conversion is correctly handled and that errors are properly managed.
1381-1385: Check that the event attributes are correctly populated with string addresses inTestMsgSendEvents.
1406-1412: InTestMsgMultiSendEvents, ensure that address conversion is correctly handled and that errors are properly managed.
1459-1465: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1462-1473]
Verify that the event attributes in
TestMsgMultiSendEventsare correctly populated with string addresses.
1896-1899: InTestBalanceTrackingEvents, ensure that address conversion is correctly handled when processing events.
1904-1906: Check that address conversion is correctly handled inTestBalanceTrackingEventswhen processing received coin events.
1922-1925: Ensure that address conversion is correctly handled inTestBalanceTrackingEventswhen iterating account balances.
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| assert.NilError(t, err) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling for the address-to-string conversion.
- assert.NilError(t, err)
+ if err != nil {
+ t.Fatalf("Failed to convert address to string: %v", 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.
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| assert.NilError(t, err) | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| if err != nil { | |
| t.Fatalf("Failed to convert address to string: %v", err) | |
| } |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | ||
| assert.NilError(t, err) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling for the address-to-string conversion.
- assert.NilError(t, err)
+ if err != nil {
+ t.Fatalf("Failed to convert address to string: %v", 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.
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| assert.NilError(t, err) | |
| addr1Str, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr1) | |
| if err != nil { | |
| t.Fatalf("Failed to convert address to string: %v", err) | |
| } | |
| addrStr, err := addressCodec.BytesToString(addr) | ||
| assert.NilError(t, err) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling for the address-to-string conversion.
- assert.NilError(t, err)
+ if err != nil {
+ t.Fatalf("Failed to convert address to string: %v", 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.
| addrStr, err := addressCodec.BytesToString(addr) | |
| assert.NilError(t, err) | |
| addrStr, err := addressCodec.BytesToString(addr) | |
| if err != nil { | |
| t.Fatalf("Failed to convert address to string: %v", err) | |
| } | |
| addr1Str, err := addressCodec.BytesToString(addr1) | ||
| assert.NilError(t, err) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling for the address-to-string conversion.
- assert.NilError(t, err)
+ if err != nil {
+ t.Fatalf("Failed to convert address to string: %v", 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.
| addr1Str, err := addressCodec.BytesToString(addr1) | |
| assert.NilError(t, err) | |
| addr1Str, err := addressCodec.BytesToString(addr1) | |
| if err != nil { | |
| t.Fatalf("Failed to convert address to string: %v", err) | |
| } | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | ||
| assert.NilError(t, err) |
There was a problem hiding this comment.
Ensure proper error handling for the address-to-string conversion.
- assert.NilError(t, err)
+ if err != nil {
+ t.Fatalf("Failed to convert address to string: %v", 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.
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| assert.NilError(t, err) | |
| addrStr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addr) | |
| if err != nil { | |
| t.Fatalf("Failed to convert address to string: %v", err) | |
| } |
| ac := testutil.CodecOptions{}.GetAddressCodec() | ||
| addr1, err := ac.BytesToString([]byte("input")) | ||
| require.NoError(t, err) | ||
| addr2, err := ac.BytesToString([]byte("output")) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Ensure error handling is consistent by checking for errors immediately after function calls that can fail.
- addr2, err := ac.BytesToString([]byte("output"))
+ addr2, err := ac.BytesToString([]byte("output"))
+ require.NoError(t, 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.
| ac := testutil.CodecOptions{}.GetAddressCodec() | |
| addr1, err := ac.BytesToString([]byte("input")) | |
| require.NoError(t, err) | |
| addr2, err := ac.BytesToString([]byte("output")) | |
| require.NoError(t, err) | |
| ac := testutil.CodecOptions{}.GetAddressCodec() | |
| addr1, err := ac.BytesToString([]byte("input")) | |
| require.NoError(t, err) | |
| addr2, err := ac.BytesToString([]byte("output")) | |
| require.NoError(t, err) | |
| require.NoError(t, err) | |
| addrStr, err := ac.BytesToString(addr) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Ensure error handling is consistent by checking for errors immediately after function calls that can fail.
- addrStr, err := ac.BytesToString(addr)
+ addrStr, err := ac.BytesToString(addr)
+ require.NoError(t, 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.
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) | |
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) |
| addrStr, err := ac.BytesToString(addr) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Ensure error handling is consistent by checking for errors immediately after function calls that can fail.
- addrStr, err := ac.BytesToString(addr)
+ addrStr, err := ac.BytesToString(addr)
+ require.NoError(t, 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.
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) | |
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) |
| addrStr, err := ac.BytesToString(addr) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Ensure error handling is consistent by checking for errors immediately after function calls that can fail.
- addrStr, err := ac.BytesToString(addr)
+ addrStr, err := ac.BytesToString(addr)
+ require.NoError(t, 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.
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) | |
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(t, err) |
| addrStr, err := ac.BytesToString(addr) | ||
| require.NoError(b, err) |
There was a problem hiding this comment.
Ensure error handling is consistent by checking for errors immediately after function calls that can fail.
- addrStr, err := ac.BytesToString(addr)
+ addrStr, err := ac.BytesToString(addr)
+ require.NoError(b, 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.
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(b, err) | |
| addrStr, err := ac.BytesToString(addr) | |
| require.NoError(b, err) |
Co-authored-by: Marko <marko@baricevic.me>
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/bank/CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
x/bank/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
| * [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes: | ||
| * Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`. | ||
| * Added an address codec as an argument to `NewSendAuthorization`. | ||
| * Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error. | ||
| * (simulation) `RandomGenesisBalances` also returns an error. |
There was a problem hiding this comment.
Consider adding more descriptive details about the impact of these changes for better clarity.
* [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes:
+ * This change impacts various functions and methods across the SDK, requiring updates to how addresses are handled and converted. Specifically:
* Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`.
* Added an address codec as an argument to `NewSendAuthorization`.
* Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error.
* (simulation) `RandomGenesisBalances` also returns an error.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.
| * [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes: | |
| * Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`. | |
| * Added an address codec as an argument to `NewSendAuthorization`. | |
| * Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error. | |
| * (simulation) `RandomGenesisBalances` also returns an error. | |
| * [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes: | |
| * This change impacts various functions and methods across the SDK, requiring updates to how addresses are handled and converted. Specifically: | |
| * Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`. | |
| * Added an address codec as an argument to `NewSendAuthorization`. | |
| * Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error. | |
| * (simulation) `RandomGenesisBalances` also returns an error. |
x/bank/simulation/genesis.go
Outdated
| @@ -83,9 +87,14 @@ func RandomizedGenState(simState *module.SimulationState) { | |||
| totalSupply := simState.InitialStake.Mul(sdkmath.NewInt((numAccs + simState.NumBonded))) | |||
| supply := sdk.NewCoins(sdk.NewCoin(simState.BondDenom, totalSupply)) | |||
|
|
|||
| balancer, err := RandomGenesisBalances(simState) | |||
There was a problem hiding this comment.
nit: balancer should be balances?
* main: (45 commits) build(deps): Bump github.com/decred/dcrd/dcrec/secp256k1/v4 from 4.2.0 to 4.3.0 (cosmos#19913) build(deps): Bump google.golang.org/grpc from 1.62.1 to 1.63.0 (cosmos#19929) test(types/address): add additional unit tests for address.LengthPrefix and a… (cosmos#19964) refactor(x/bank)!: remove Address.String() (cosmos#19954) build(deps): Bump github.com/prometheus/common from 0.51.1 to 0.52.2 (cosmos#19930) fix(x/epochs): Fix init genesis (cosmos#19958) feat(core,runtime): transaction service (exec mode) (cosmos#19953) fix(x/authz): non utf-8 characters in logs (cosmos#19923) build(deps): Bump golang.org/x/crypto from 0.21.0 to 0.22.0 (cosmos#19960) chore: fix spelling errors (cosmos#19957) fix(x/tx): don't shadow Amino marshalling error messages (cosmos#19955) refactor(types): loosen module.AppModule interface (cosmos#19951) feat(core): add `TxValidator` interface (cosmos#19950) build(deps): Bump cosmossdk.io/store from 1.0.2 to 1.1.0 in /x/epochs (cosmos#19947) feat(x/epochs): upstream osmosis epoch module (cosmos#19697) build(deps): Bump bufbuild/buf-setup-action from 1.30.0 to 1.30.1 (cosmos#19928) refactor(x/genutil)!: remove Address.String() (cosmos#19926) docs(x/mint): Fix inconsistency in mint docs (cosmos#19915) build(deps): Bump github.com/regen-network/gocuke from 1.1.0 to 1.1.1 in /orm (cosmos#19920) feat: Integrate grpc configuration into client.toml (cosmos#19905) ...
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
New Features
Bug Fixes
initGenFilesfunction andSanitizeGenesisBalances, enhancing the robustness of genesis file initialization and balance sanitization.Refactor
Tests
Documentation
Address.String()method and related adjustments for a clearer understanding of the changes.Chores