refactor(x/auth): removal of Address.String() #19997
Conversation
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
WalkthroughWalkthroughThe changes involve a significant refactoring related to the handling of Bech32 address encoding in the Cosmos SDK. This includes initializing and using a new Bech32 codec, transitioning from direct string conversions to using an address codec, and modifying various test setups to integrate these changes. Changes
Possibly related issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (4)
Files skipped from review as they are similar to previous changes (4)
Warning Following problems were encountered
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 (
|
github-actions
bot
removed
C:x/slashing
C:x/mint
C:x/crisis
C:x/upgrade
C:x/consensus
C:x/circuit
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not convert address to string: %s", err.Error()) | ||
| } | ||
| return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr) |
There was a problem hiding this comment.
Improve error handling by separating the error message construction from the error check for clarity and maintainability.
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
- return fmt.Errorf("could not convert address to string: %s", err.Error())
+ errMsg := fmt.Errorf("could not convert address to string: %s", err.Error())
+ return errMsg
}This refactor makes the error handling more readable and maintainable by clearly separating the creation of the error message from the return statement.
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.
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | |
| if err != nil { | |
| return fmt.Errorf("could not convert address to string: %s", err.Error()) | |
| } | |
| return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr) | |
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | |
| if err != nil { | |
| errMsg := fmt.Errorf("could not convert address to string: %s", err.Error()) | |
| return errMsg | |
| } | |
| return fmt.Errorf("the account %s is without a pubkey and did not provide a pubkey in the tx to set it", addr) |
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not convert address to string: %s", err.Error()) | ||
| } | ||
| return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address()) |
There was a problem hiding this comment.
Ensure consistent error handling by using sdkerrors.ErrInvalidPubKey.Wrapf for all public key related errors.
addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress())
if err != nil {
- return fmt.Errorf("could not convert address to string: %s", err.Error())
+ return sdkerrors.ErrInvalidPubKey.Wrapf("could not convert address to string: %s", err.Error())
}
- return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address())
+ return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %s", addr, txPubKey.Address().String())This change ensures that all public key related errors are handled consistently using the sdkerrors.ErrInvalidPubKey.Wrapf method, improving the readability and maintainability of the code. Additionally, converting the address to a string for the error message enhances clarity.
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.
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | |
| if err != nil { | |
| return fmt.Errorf("could not convert address to string: %s", err.Error()) | |
| } | |
| return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %x", addr, txPubKey.Address()) | |
| addr, err := svd.ak.AddressCodec().BytesToString(acc.GetAddress()) | |
| if err != nil { | |
| return sdkerrors.ErrInvalidPubKey.Wrapf("could not convert address to string: %s", err.Error()) | |
| } | |
| return sdkerrors.ErrInvalidPubKey.Wrapf("the account %s cannot be claimed by public key with address %s", addr, txPubKey.Address().String()) |
github-actions
bot
removed
C:x/gov
C:x/distribution
JulianToledano
changed the title
|
lets leave this module as is. It's a bit messy and api breaking. Once mint is completed we will begin the work to use accounts as auth as an option. This will allow for auth to be deprecated. Thank you for this hard work. It always hurts a little when so much work is put into something and it's closed, sorry, 😢 |
Description
ref:
#13140
#7448
There are two calls to
String()that this PR doesn't remove:cosmos-sdk/x/auth/types/account.go
Lines 62 to 69 in 24d44b6
cosmos-sdk/x/auth/types/account.go
Lines 216 to 230 in 24d44b6
To remove those GenesisAccount and AccountI must be refactored.
The refactor of those will be done in an incoming PR to make reviews easier. (also easier to me 🙃)
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