Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(types): add MustValAddressFromBech32 function #18768

Merged
merged 7 commits into from
Dec 17, 2023

Conversation

luchenqun
Copy link
Contributor

Description

I noticed that MustAccAddressFromBech32 was implemented, but MustValAddressFromBech32 was not. I need to use it in actual coding scenarios. For example, in evmos I need to convert a string ValAddress into an evm hex string.
The pseudo code is as follows:

valAddr := sdk.MustValAddressFromBech32(valAddress)
valHexAddr := common.BytesToAddress(valAddr.Bytes())

@luchenqun luchenqun requested a review from a team as a code owner December 16, 2023 13:32
Copy link
Contributor

coderabbitai bot commented Dec 16, 2023

Walkthrough

The codebase has been updated with a new function, MustValAddressFromBech32, designed to validate and convert Bech32 strings into addresses within the address module. Additionally, the test suite has been expanded with a new test case to ensure the correct functionality of this new utility function by comparing its output with the existing ValAddressFromBech32 function.

Changes

Files Change Summary
CHANGELOG.md, types/address.go, types/address_test.go Addition of MustValAddressFromBech32 function, extending address module functionality. Modifications to Equals method for ValAddress. Addition of test TestMustAccAddressFromBech32 for new function.

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 ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

types/address.go Outdated
@@ -362,6 +362,16 @@ func ValAddressFromBech32(address string) (addr ValAddress, err error) {
return ValAddress(bz), nil
}

// MustValAddressFromBech32 calls MustValAddressFromBech32 and panics on error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 365 incorrectly references MustValAddressFromBech32 instead of ValAddressFromBech32. This should be corrected to avoid confusion.

- // MustValAddressFromBech32 calls MustValAddressFromBech32 and panics on error.
+ // MustValAddressFromBech32 calls ValAddressFromBech32 and panics on 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.

Suggested change
// MustValAddressFromBech32 calls MustValAddressFromBech32 and panics on error.
// MustValAddressFromBech32 calls ValAddressFromBech32 and panics on error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks rabbit. I fix it.

Comment on lines 571 to 579
func (s *addressTestSuite) TestMustAccAddressFromBech32() {
address := "cosmosvaloper1780p20ce3quhzqxdq0xfeqtnzrjgdraz0kwwjp"
valAddress1, err := types.ValAddressFromBech32(address)
s.Require().Nil(err)

valAddress2 := types.MustValAddressFromBech32(address)

s.Require().Equal(valAddress1, valAddress2)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case provided for MustValAddressFromBech32 only covers the happy path. It would be prudent to include additional test cases to cover edge cases, such as invalid Bech32 strings, to ensure that the function behaves as expected in error scenarios.

CHANGELOG.md Outdated
@@ -50,6 +50,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/auth/vesting) [#17810](https://github.com/cosmos/cosmos-sdk/pull/17810) Add the ability to specify a start time for continuous vesting accounts.
* (runtime) [#18475](https://github.com/cosmos/cosmos-sdk/pull/18475) Adds an implementation for core.branch.Service.
* (baseapp) [#18499](https://github.com/cosmos/cosmos-sdk/pull/18499) Add `MsgRouter` response type from message name function.
* (address) [#18768](https://github.com/cosmos/cosmos-sdk/pull/18768) Add MustValAddressFromBech32 function.
Copy link
Member

@julienrbrt julienrbrt Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@julienrbrt julienrbrt changed the title feat(address): add MustValAddressFromBech32 function feat(types/address): add MustValAddressFromBech32 function Dec 16, 2023
@julienrbrt julienrbrt changed the title feat(types/address): add MustValAddressFromBech32 function feat(types): add MustValAddressFromBech32 function Dec 16, 2023
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, modulo updating the changelog correctly

CHANGELOG.md Outdated Show resolved Hide resolved
@julienrbrt julienrbrt added this pull request to the merge queue Dec 17, 2023
Merged via the queue into cosmos:main with commit 98c6b1d Dec 17, 2023
55 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants