refactor(types)!: removed global config for verifier #18607
Conversation
… feature/sdkConfig
… feature/sdkConfig
WalkthroughThe changes primarily focus on the decoupling of address verification logic from the global SDK configuration and its integration into more specific areas, such as the Bech32 codec. This adjustment aligns with the goal of removing global configurations and clarifying the use cases for address verification. By relocating address verification to contextually appropriate locations, the changes ensure that Bech32 checks are explicitly used when intended, and custom address verifiers can be implemented without conflicting with the default behavior. 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 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 (
|
bizk
changed the title
…-sdk into feature/sdkConfig-verifier
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lets do this in this pr, otherwise lets can close this pr and remake it with the changes proposed. This pr becomes a bit mute if we avoid doing it here. |
|
So we are using the codec then, right? |
yea |
There was a problem hiding this comment.
see above, please update AccAddressFromBech32, ValAddressFromBech32,... as in https://github.com/cosmos/cosmos-sdk/compare/julien/address-cdc#diff-f275f0ae24edfd997f8b93477171779ea2caa786d98cd249f4cb7699e58a67b2L6-L530
done |
|
|
||
| // AccAddressFromBech32 creates an AccAddress from a Bech32 string. | ||
| func AccAddressFromBech32(address string) (addr AccAddress, err error) { | ||
| if len(strings.TrimSpace(address)) == 0 { | ||
| return AccAddress{}, errors.New("empty address string is not allowed") | ||
| } | ||
|
|
||
| bech32PrefixAccAddr := GetConfig().GetBech32AccountAddrPrefix() | ||
|
|
||
| bz, err := GetFromBech32(address, bech32PrefixAccAddr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = VerifyAddressFormat(bz) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return AccAddress(bz), nil | ||
| addrCdc := addresscodec.NewBech32Codec(bech32PrefixAccAddr) | ||
| return addrCdc.StringToBytes(address) | ||
| } | ||
|
|
||
| // Returns boolean for whether two AccAddresses are Equal |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [16-311]
The refactoring to use addresscodec.NewBech32Codec for address decoding introduces a significant change in how addresses are handled within the SDK. It's important to ensure that documentation, examples, and migration guides are updated to reflect these changes. This will help developers understand how to use the new address codecs and adjust their code accordingly.
Would you like assistance in updating the documentation or creating a migration guide for these changes?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 134-134: The changelog entry for PR refactor(types)!: removed global config for verifier #18607 has been updated to include a note about the breaking change, which is good practice for transparency and informing users. However, ensure that all related documentation, migration guides, or upgrade instructions are also updated to reflect this breaking change and guide users through any necessary adjustments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/address.go (5 hunks)
Additional comments: 4
types/address.go (4)
- 15-15: The import of the
addresscodecpackage fromgithub.com/cosmos/cosmos-sdk/codec/addressintroduces a new dependency for handling address encoding and decoding. This change aligns with the PR's objective to decouple address verification from global configuration, promoting modularity.- 170-171: The
AccAddressFromBech32function now uses theaddresscodec.NewBech32Codecto decode Bech32 addresses, which is a significant change from the previous global verifier approach. This modification enhances modularity by allowing for different address codecs to be used, aligning with the PR's objectives. Ensure that all calls to this function across the codebase are updated to handle the new return type, as this change could impact error handling and address parsing logic.Verification successful
The verification process has identified three instances in
types/address.gowhereAccAddressFromBech32is used, specifically at lines 158, 223, and 244. Each of these usages correctly handles the return type, which includes both the address and an error, indicating that the changes toAccAddressFromBech32have been properly integrated within the codebase. This confirms that the modifications align with the PR's objectives of enhancing modularity by allowing for different address codecs to be used and ensuring that error handling and address parsing logic are appropriately managed across the codebase.* 309-310: The `ValAddressFromBech32` function's adaptation to use `addresscodec.NewBech32Codec` for decoding addresses is consistent with the changes made to `AccAddressFromBech32`. This ensures a uniform approach to address decoding across different address types, supporting the PR's goal of removing global verifier dependencies. Similar to `AccAddressFromBech32`, verify that all usages of this function are compatible with the changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of AccAddressFromBech32 handle the new return type correctly. ast-grep --lang go --pattern $'AccAddressFromBech32($_)'Length of output: 244
Verification successful
The verification process has identified three usages of the
ValAddressFromBech32function within thetypes/address.gofile. These usages are at lines 315, 373, and 395, respectively. Each instance correctly handles the return type, indicating that the changes made to theValAddressFromBech32function are properly integrated across its usages within the same file.Given this information, it can be concluded that the adaptation of the
ValAddressFromBech32function to useaddresscodec.NewBech32Codecfor decoding addresses, and the subsequent handling of its new return type, is correctly implemented in the identified usages within the codebase. This supports the PR's goal of removing global verifier dependencies and ensures a uniform approach to address decoding across different address types.* 461-462: The changes to `ConsAddressFromBech32` mirror those applied to `AccAddressFromBech32` and `ValAddressFromBech32`, utilizing `addresscodec.NewBech32Codec` for address decoding. This consistent application of the new codec across different address types is crucial for maintaining a coherent and modular approach to address handling within the SDK. As with the other address types, ensure compatibility with the updated function across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of ValAddressFromBech32 handle the new return type correctly. ast-grep --lang go --pattern $'ValAddressFromBech32($_)'Length of output: 244
Verification successful
The new context from the shell scripts provides the necessary details around the usage of
ConsAddressFromBech32at lines 520 and 542 intypes/address.go. From the provided output, it's clear that in both instances, the functionConsAddressFromBech32is used within a conditional block where the error returned by the function is checked. If an error is present, the function returns the error, effectively handling the new return type correctly.This behavior aligns with the original review comment's emphasis on ensuring compatibility across the codebase with the updated function. The usages of
ConsAddressFromBech32correctly handle the return values, especially focusing on error handling, which is crucial for maintaining robustness in address handling within the SDK.Given this information, it can be concluded that the changes to
ConsAddressFromBech32and its usages within the codebase are correctly implemented and handle the new return type as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all usages of ConsAddressFromBech32 handle the new return type correctly. ast-grep --lang go --pattern $'ConsAddressFromBech32($_)'Length of output: 180
Script:
#!/bin/bash # Extract and review the usage of ConsAddressFromBech32 at lines 520 and 542 in types/address.go fd "address.go" --exec grep -C 3 "ConsAddressFromBech32" {}Length of output: 548
Description
Closes: #18608
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