refactor(baseapp): remove AccAddress.String() #21521
Conversation
JulianToledano
added
the
backport/v0.52.x
JulianToledano
requested review from
facundomedica,
aaronc,
testinginprod,
kocubinski and
a team
as code owners
WalkthroughWalkthroughThe changes involve significant updates to the handling of address encoding across various test files in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant AddressCodec
participant Transaction
TestSuite->>AddressCodec: Initialize AddressCodec
TestSuite->>Transaction: Call newTxCounter with AddressCodec
Transaction->>AddressCodec: Convert address using BytesToString
Transaction-->>TestSuite: Return formatted address
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- baseapp/abci_test.go (36 hunks)
- baseapp/abci_utils.go (2 hunks)
- baseapp/abci_utils_test.go (5 hunks)
- baseapp/baseapp_test.go (14 hunks)
- baseapp/msg_service_router_test.go (1 hunks)
- baseapp/streaming_test.go (1 hunks)
- baseapp/utils_test.go (2 hunks)
Additional context used
Path-based instructions (7)
baseapp/msg_service_router_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"baseapp/streaming_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"baseapp/utils_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"baseapp/abci_utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/abci_utils_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"baseapp/baseapp_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"baseapp/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"
Additional comments not posted (31)
baseapp/msg_service_router_test.go (1)
158-162: LGTM!The code changes are approved for the following reasons:
- Using the
BytesToStringmethod from theAddressCodecof thesigningCtxto convert theaddrto a string ensures that the address is correctly formatted before being assigned to theOwnerfield.- Explicitly checking the error from the
BytesToStringmethod usingrequire.NoErrorimproves the robustness of the test by ensuring that any issues during the conversion are caught.- The changes align with the best practices of using the
AddressCodecfor address formatting and error handling.baseapp/utils_test.go (4)
297-301: LGTM!The changes to the
newTxCounterfunction signature and the usage of theaddress.Codecfor address conversion improve the clarity and correctness of address handling. The error handling ensures robustness.
304-304: LGTM!Using the string representation of the address obtained from the codec for constructing the
MsgCountermessage ensures consistency with the rest of the code that uses the codec for address handling.
347-354: LGTM!The changes to the
setFailOnHandlerfunction signature and the usage of theaddress.Codecfor address conversion improve the flexibility of the function by allowing the address to be derived from the codec rather than being hardcoded. The error handling ensures robustness.
369-376: LGTM!The changes to the
wonkyMsgfunction signature and the usage of theaddress.Codecfor deriving the address for theMsgCounter2message improve flexibility by allowing the address to be derived from the codec rather than being hardcoded. The error handling ensures robustness.baseapp/abci_utils.go (1)
303-304: LGTM!The code changes enhance clarity by explicitly converting
signer.Signerto a string before using it as the map key. This improves readability and maintainability while preserving the overall functionality.Also applies to: 306-306, 317-317
baseapp/abci_utils_test.go (5)
Line range hint
724-753: LGTM!The code changes to the
buildMsgfunction are approved:
- The function signature has been correctly updated to accept an
address.Codecparameter.- The
address.Codecis used to convert the public key bytes to a string address, improving the address encoding.
489-490: LGTM!The code changes to the
TestDefaultProposalHandler_NoOpMempoolTxSelectiontest are approved:
- The test correctly uses the
address.Codecfrom the signing context to convert the address bytes to a string.Also applies to: 493-493
598-615: LGTM!The code changes to the
TestDefaultProposalHandler_PriorityNonceMempoolTxSelectiontest are approved:
- The test correctly calls the
buildMsgfunction with theaddress.Codecfrom the signing context, improving the address encoding.
5-5: LGTM!The code change to the
TestABCIUtilsTestSuitestruct is approved:
- The struct correctly imports the
cosmossdk.io/core/addresspackage, allowing the test suite to use theaddresspackage.
741-743: LGTM!The code changes to the
buildMsgfunction are approved:
- The function correctly uses the
address.Codecto convert the public key bytes to a string address, improving the address encoding.baseapp/baseapp_test.go (12)
6-6: LGTM!The import of
cosmossdk.io/core/addresspackage is approved.
54-54: LGTM!The addition of the
acfield to theBaseAppSuitestruct is approved.
95-95: LGTM!The initialization of the
acfield with the address codec from the signing context is approved.
157-158: LGTM!The usage of the
acfield to convert address bytes to string is approved.
160-160: LGTM!The usage of the converted address string to create a new message is approved.
234-234: LGTM!The usage of the
acfield to create a new transaction is approved.
516-516: LGTM!The usage of the
signingCtx.AddressCodec()to create a new transaction is approved.
560-560: LGTM!The usage of the
suite.acfield to create a new transaction is approved.
590-590: LGTM!The usage of the
suite.acfield to create a new transaction is approved.
607-608: LGTM!The usage of the
suite.acfield to create a new transaction and set it to fail on the handler is approved.
625-625: LGTM!The usage of the
suite.acfield to create a new transaction is approved.
665-665: LGTM!The usage of the
suite.acfield to create new transactions and set them to fail on the handler or use a wonky message is approved.Also applies to: 679-679, 690-690, 911-911
baseapp/abci_test.go (8)
43-43: Looks good!The new import statement for
codectestutilis approved.
604-606: Approved.Passing the
suite.acaddress codec tonewTxCounterlooks good.
659-661: LGTM.The update to pass
suite.actonewTxCounteris approved.
707-709: Looks good to me.Passing
suite.actonewTxCounteris approved.
728-736: Approved.The code changes to construct a new transaction with additional
MsgCounter2messages look good. Usingsuite.ac.BytesToStringto convert the address is the correct approach.
795-801: Looks good!Initializing the address codec and converting test addresses from bytes to string using the codec's
BytesToStringmethod is the correct approach. The error handling forBytesToStringcalls is also done properly.
810-811: Approved.Updating the test messages to use the string representation of addresses (
addrStrandtoAddrStr) instead of the byte representation looks good. The changes are consistent across all the test messages.Also applies to: 818-819, 836-837, 845-846, 854-858, 867-871, 881-885
937-943: LGTM.Initializing the address codec, converting test addresses to string, and using the string representation in the test messages is done correctly. The changes are consistent with the previous updates.
Also applies to: 954-955, 964-965
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- baseapp/abci_utils_test.go (5 hunks)
- baseapp/baseapp_test.go (14 hunks)
Files skipped from review as they are similar to previous changes (2)
- baseapp/abci_utils_test.go
- baseapp/baseapp_test.go
(cherry picked from commit a48a75f) # Conflicts: # baseapp/abci_test.go
Co-authored-by: Julián Toledano <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Ref:
#7448
#20955
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
Bug Fixes
Refactor
Tests