refactor(bank): remove .String() calls #18175
Conversation
github-actions
bot
added
C:CLI
C:x/gov
C:Simulations
C:x/bank
C:x/genutil
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughThe overarching change involves the removal of global bech32 prefixes and the transition to localized address handling within the Cosmos SDK. This includes the introduction of an address codec for converting between address bytes and strings, the deprecation of Changes
Assessment against linked 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 X ? TipsChat with CodeRabbit Bot (
|
![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: 13
Configuration used: CodeRabbit UI
Files selected for processing (22)
- tests/e2e/auth/suite.go (7 hunks)
- tests/e2e/bank/suite.go (5 hunks)
- tests/integration/auth/client/cli/suite_test.go (3 hunks)
- tests/integration/bank/app_test.go (2 hunks)
- tests/starship/tests/transfer_test.go (1 hunks)
- x/authz/simulation/operations.go (1 hunks)
- x/bank/client/cli/tx_test.go (5 hunks)
- x/bank/keeper/grpc_query_test.go (1 hunks)
- x/bank/keeper/keeper.go (3 hunks)
- x/bank/keeper/send.go (5 hunks)
- x/bank/keeper/view.go (2 hunks)
- x/bank/module.go (2 hunks)
- x/bank/simulation/operations.go (3 hunks)
- x/bank/types/events.go (1 hunks)
- x/bank/types/msgs.go (1 hunks)
- x/bank/types/msgs_test.go (1 hunks)
- x/bank/types/querier.go (1 hunks)
- x/bank/types/send_authorization_test.go (5 hunks)
- x/genutil/gentx_test.go (3 hunks)
- x/gov/abci_test.go (2 hunks)
- x/gov/keeper/common_test.go (2 hunks)
- x/gov/types/v1/msgs_test.go (2 hunks)
Additional comments: 51
tests/e2e/auth/suite.go (6)
11-17: The import of
"cosmossdk.io/core/address"and"github.com/cosmos/cosmos-sdk/codec/address"aligns with the PR objectives to refactor address handling usingaddress.Codec.42-45: The addition of the
acfield to theE2ETestSuitestruct is in line with the PR objectives to useaddress.Codecfor address handling.82-82: The initialization of the
acfield withaddresscodec.NewBech32Codec("cosmos")in theSetupSuitemethod aligns with the PR objectives.1307-1310: The use of
address.Codecto convert addresses to strings before constructing messages in theTestGetBroadcastCommandWithoutOfflineFlagfunction aligns with the PR objectives.1331-1337: The use of
address.Codecto convert addresses to strings before constructing messages in theTestTxWithoutPublicKeyfunction aligns with the PR objectives.1398-1412: The use of
address.Codecto convert addresses to strings before constructing messages in theTestSignWithMultiSignersAminoJSONfunction aligns with the PR objectives.tests/e2e/bank/suite.go (5)
6-19: The addition of imports for
addressandaddresscodecis consistent with the PR's objective to refactor address handling usingaddress.Codec.25-30: The addition of the
acfield of typeaddress.Codecto theE2ETestSuitestruct aligns with the PR's objective to useaddress.Codecfor address conversion.88-92: Initialization of the
acfield withaddresscodec.NewBech32Codec("cosmos")in theSetupSuitefunction is in line with the PR's objective to use localized and modular address codecs.105-117: Conversion of
fromandtobyte slices to strings using theaccodec and the usage of these strings in theMsgSendstruct and assertions are consistent with the PR's objective to replace.String()calls withaddress.Codecconversions.128-130: The assertion in
TestNewSendTxCmdGenOnlyusing the string representations of the addresses is consistent with the PR's objective and the summary provided.tests/integration/bank/app_test.go (2)
67-67: The change to
NewMsgSendto accept string representations of addresses aligns with the PR objective to remove direct.String()calls on addresses and is consistent with the provided summary.170-174: The conversion of addresses to strings using the
AddressCodecbefore creating thesendMsgobject aligns with the PR objective to replace.String()calls with an address codec. This change is consistent with the provided summary.x/bank/client/cli/tx_test.go (5)
54-64: The changes in the
TestMultiSendTxCmdfunction align with the PR objectives to refactor address handling by using anaddress.Codecto convert account addresses to strings. This is consistent with the broader initiative to eliminate global bech32 prefixes and improve modularity.85-94: The test case "valid transaction" correctly uses the string representations of addresses, which aligns with the PR objectives and the provided summary.
104-111: The test case "invalid from Address" uses a hardcoded invalid address string "foo", which is appropriate for testing error handling in the context of the PR's objectives.
119-127: The test case "invalid recipients" includes an intentionally invalid recipient address "bar" to test error handling, which is consistent with the PR's objectives.
136-145: The test case "invalid amount" is designed to test the error handling when no amount is provided. This is unrelated to the address handling changes but is a necessary part of the test suite's robustness.
x/bank/keeper/grpc_query_test.go (1)
- 234-243: The changes in the hunk align with the PR objectives by using
AddressCodecto convert theaddrvariable to a string representation before using it in theNewQuerySpendableBalanceByDenomRequestmethod. This is consistent with the goal of removing direct.String()calls on address objects.x/bank/keeper/keeper.go (3)
152-167: The changes in the
DelegateCoinsfunction align with the PR objectives by using the address codec to convertdelegatorAddrto a string before emitting the event. The logic appears to be correct and consistent with the PR's goal of removing.String()calls.374-386: The changes in the
MintCoinsfunction are consistent with the PR objectives. The address codec is used to convert the module account address to a string before emitting the mint event. This change is in line with the goal of eliminating.String()calls on addresses.413-426: The changes in the
BurnCoinsfunction are consistent with the PR objectives. The address codec is used to convert the account address to a string before emitting the burn event. This change is in line with the goal of eliminating.String()calls on addresses.x/bank/keeper/send.go (4)
178-184: The event emission in
InputOutputCoinsmethod usesout.Addressdirectly, which is consistent with the PR objective to avoid.String()calls. However, ensure thatout.Addressis already in the correct string format expected by the event manager.229-245: The
SendCoinsmethod correctly usesAddressCodec().BytesToStringto convertfromAddrandtoAddrto strings before emitting the event. This change aligns with the PR objective and the provided summary.289-296: The
subUnlockedCoinsmethod correctly usesAddressCodec().BytesToStringto convertaddrto a string before emitting theCoinSpentEvent. This change aligns with the PR objective and the provided summary.318-327: The
addCoinsmethod correctly usesAddressCodec().BytesToStringto convertaddrto a string before emitting theCoinReceivedEvent. This change aligns with the PR objective and the provided summary.x/bank/keeper/view.go (1)
- 119-129: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [121-140]
The changes in these hunks align with the PR objectives by replacing direct
.String()calls with the use ofAddressCodecfor address conversion. The introduction ofaddrStrand its use inmapAddressToBalancesIdxandbalancesis consistent with the goal of improving modularity in address handling. The error handling withpanicon failure to convert the address is a strong choice, ensuring that any issues with address conversion are caught immediately. However, it's important to consider if panicking is the best approach in all contexts where this function might be used, as it could lead to a less graceful handling of errors in some scenarios.x/bank/module.go (3)
218-235: The changes in the hunk correctly replace direct
.String()calls withAddressCodecconversions for blocked module accounts, aligning with the PR objective to improve modularity in address handling.240-255: The changes in the hunk correctly replace direct
.String()calls withAddressCodecconversions for theauthorityvariable, aligning with the PR objective to improve modularity in address handling.240-240: Verify the use of
NewModuleAddressOrBech32Addressfunction and its implications on the PR objectives, as it is not mentioned in the provided summaries or PR objectives. This function might have broader implications that need to be considered.x/bank/simulation/operations.go (4)
- 85-92: > 💡 NOTE
Codebase verification is a beta feature.
The changes in
x/bank/simulation/operations.goto useak.AddressCodec().BytesToStringfor address conversion align with the PR's objective of refactoring address handling. No remaining.String()calls on address objects were found in the codebase, suggesting that the refactor is consistent throughout.
- 94-94: > 💡 NOTE
Codebase verification is a beta feature.
The
NewMsgSendfunction usage has been correctly updated to use string parameters for addresses in theoperations.gofile. Additionally, the following files and lines have been identified whereNewMsgSendis used with string parameters, which aligns with the changes:
./tests/integration/bank/app_test.go: lines 67, 174./tests/e2e/bank/suite.go: line 130./x/bank/types/send_authorization_test.go: lines 38, 51, 72, 85, 98, 108./x/bank/simulation/operations.go: lines 94, 138
159-162: The conversion from a string address to bytes using
ak.AddressCodec().StringToBytesis consistent with the PR objective. This change should be verified across the codebase to ensure all instances of address byte conversion are updated.159-162: The error handling for address conversion in
sendMsgSendis appropriate and ensures that the simulation will not proceed with an invalid address. This is a good practice for robustness.x/bank/types/events.go (1)
- 26-59: The changes in the event construction functions correctly replace
sdk.AccAddresswithstringfor thespender,receiver,minter, andburnerparameters, in line with the PR objectives to refactor address handling. This is a significant change that will affect any downstream code that relies on these events, so it's important to ensure that all dependent code has been updated accordingly.x/bank/types/msgs.go (2)
14-16: The change from
sdk.AccAddresstostringforfromAddrandtoAddrinNewMsgSendaligns with the PR objectives to remove.String()calls on addresses and use an address codec instead.14-16: > 💡 NOTE
Codebase verification is a beta feature.
The
NewMsgSendfunction signature has been successfully updated to acceptstringparameters forfromAddrandtoAddrinstead ofsdk.AccAddress. The following location has been updated to reflect this change:
./x/bank/types/msgs_test.go: line 16x/bank/types/msgs_test.go (1)
- 15-19: > 💡 NOTE
Codebase verification is a beta feature.
The
MsgSendstructure appears to be unchanged, with fields for sender and receiver addresses still present as strings. The test case modifications to use string literals for addresses are consistent with the structure ofMsgSend. No further action is required unless additional context indicates a change in the JSON representation ofMsgSend.x/bank/types/querier.go (1)
- 32-36: The change in the function signature of
NewQuerySpendableBalanceByDenomRequestfrom accepting ansdk.AccAddressto a string for the address parameter is consistent with the PR objectives and the provided summary. This change should simplify the usage of the function by allowing direct string inputs for addresses. Ensure that all calls to this function are updated to use the new signature.x/genutil/gentx_test.go (3)
17-23: The addition of the address codec package and its usage in the test suite aligns with the PR objective to refactor address handling in the Cosmos SDK.
243-248: The instantiation of the address codec with the "cosmos" prefix and the conversion of addresses to strings using this codec are in line with the PR objective to replace
.String()calls with a more modular approach.274-277: The use of address strings in the
NewMsgSendfunction is consistent with the PR objective and the summary provided, which indicates a shift from direct address references to string representations.x/gov/abci_test.go (4)
20-20: The addition of the
addresscodecimport is consistent with the PR objectives to refactor address handling.404-407: The initialization of
ac,addrStr, andaddrStr1using theaddresscodecpackage aligns with the PR objectives to replace.String()calls with address codec conversions.413-413: The modification of the
banktypes.NewMsgSendfunction argument to useaddrStrandaddrStr1is consistent with the PR objectives to refactor address handling.410-415: The test logic has been updated to accommodate the new variable initialization and function argument changes, which is expected due to the refactor.
x/gov/keeper/common_test.go (2)
34-38: The hardcoded string for
govAcctStrmay not align with the PR's objective to useaddress.Codecfor address conversion. Verify that this is the intended approach and consider usingaddress.Codecto generategovAcctStrdynamically to maintain consistency with the PR's goal of modular address handling.55-55: The change to use string representations for addresses in
banktypes.NewMsgSendaligns with the PR's objectives and the provided summary.x/gov/types/v1/msgs_test.go (3)
21-27: The addition of
addrStrsand the modification ofNewMsgSendandfmt.Sprintfcalls to use these string addresses align with the PR objective to refactor address handling by removing.String()method calls. This change is also consistent with the provided summary.62-69: The test cases have been updated to use string addresses instead of
sdk.AccAddressobjects, which is in line with the PR objective to improve modularity and flexibility of address handling within the Cosmos SDK. The changes are correctly reflected in the summary.21-27: The summary mentions a modification to the
NewMsgSubmitProposalfunction call by removing thesdk.AccAddress{}.String()argument, but this change is not visible in the provided hunk. It's possible that this change is present in another part of the codebase not included in the hunk. It would be beneficial to verify this change elsewhere in the codebase to ensure consistency with the PR objectives.
| @@ -44,7 +44,7 @@ func (s *TestSuite) TestChainTokenTransfer() { | |||
| s.Require().NoError(err) | |||
|
|
|||
| // build tx into the txBuilder | |||
| msg := banktypes.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000)))) | |||
| msg := banktypes.NewMsgSend(addr1.String(), addr2.String(), sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000)))) | |||
There was a problem hiding this comment.
The PR objective mentions the removal of .String() method calls on addresses, but the hunk still uses addr1.String() and addr2.String(). Please verify if this aligns with the intended refactoring and if an address.Codec should be used instead.
| @@ -44,7 +44,7 @@ | |||
| s.Require().NoError(err) | |||
|
|
|||
| // build tx into the txBuilder | |||
| msg := banktypes.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000)))) | |||
| msg := banktypes.NewMsgSend(addr1.String(), addr2.String(), sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(1230000)))) | |||
| s.Require().NoError(err) | |||
There was a problem hiding this comment.
There is a redundant error check after the msg construction. The err variable is not reassigned after line 47, so this check is unnecessary.
- s.Require().NoError(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.
| s.Require().NoError(err) |
| @@ -34,6 +34,8 @@ var ( | |||
| _, _, addr = testdata.KeyTestPubAddr() | |||
| govAcct = authtypes.NewModuleAddress(types.ModuleName) | |||
| poolAcct = authtypes.NewModuleAddress(pooltypes.ModuleName) | |||
| govAcctStr = "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn" | |||
| addrStr = addr.String() | |||
There was a problem hiding this comment.
The use of .String() on addr contradicts the PR's objective to eliminate such calls. Replace this with the new address.Codec approach for converting addresses to strings.
- addrStr = addr.String()
+ addrStr = addressCodec.Encode(addr)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 = addr.String() | |
| addrStr = addressCodec.Encode(addr) |
x/authz/simulation/operations.go
Outdated
| @@ -285,7 +285,16 @@ func SimulateMsgExec( | |||
| return simtypes.NoOpMsg(authz.ModuleName, TypeMsgExec, err.Error()), nil, nil | |||
| } | |||
|
|
|||
| msg := []sdk.Msg{banktype.NewMsgSend(granterAddr, granteeAddr, coins)} | |||
| graStr, err := ak.AddressCodec().BytesToString(granteeAddr) | |||
There was a problem hiding this comment.
| graStr, err := ak.AddressCodec().BytesToString(granteeAddr) | |
| graStr, err := ak.AddressCodec().BytesToString(granterAddr) |
| @@ -29,10 +30,12 @@ func TestSendAuthorization(t *testing.T) { | |||
| allowList[0] = toAddr | |||
| authorization := types.NewSendAuthorization(coins1000, nil) | |||
|
|
|||
| fmt.Println(fromAddrStr, toAddrStr, unknownAddrStr) | |||
There was a problem hiding this comment.
| fmt.Println(fromAddrStr, toAddrStr, unknownAddrStr) |
| func NewMsgSend(fromAddr, toAddr string, amount sdk.Coins) *MsgSend { | ||
| return &MsgSend{FromAddress: fromAddr, ToAddress: toAddr, Amount: amount} |
There was a problem hiding this comment.
maybe requires a changelog entry?
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
Description
Closes: #13140
this pr removes .String calls in the bank module and its dependencies
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change