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

Change address from bytes to bech32 strings #7242

Merged
merged 109 commits into from
Sep 25, 2020
Merged

Conversation

anilcse
Copy link
Collaborator

@anilcse anilcse commented Sep 4, 2020

Description

closes: #7195


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@anilcse anilcse changed the title Chnage address from bytes to bech32 strings Change address from bytes to bech32 strings Sep 4, 2020
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

I think this PR is on a good path. I just have a question about casttype.

proto/cosmos/distribution/v1beta1/query.proto Outdated Show resolved Hide resolved
proto/cosmos/crisis/v1beta1/tx.proto Outdated Show resolved Hide resolved
atheeshp and others added 14 commits September 7, 2020 16:52
…m1314/cosmos-sdk into anil/7195_bech32_addresses

# Conflicts:
#	x/distribution/keeper/grpc_query_test.go
…bech32_addresses

# Conflicts:
#	proto/ibc/channel/channel.proto
#	proto/ibc/connection/connection.proto
#	x/distribution/types/distribution.pb.go
#	x/ibc-transfer/types/transfer.pb.go
#	x/ibc/02-client/types/client.pb.go
#	x/ibc/03-connection/types/connection.pb.go
#	x/ibc/04-channel/types/channel.pb.go
#	x/staking/types/staking.pb.go
@clevinson
Copy link
Contributor

Is there a reason this PR also introduces .clang-format ? Seems to really blow up the diff size with a lot of unrelated formatting changes.

@anilcse
Copy link
Collaborator Author

anilcse commented Sep 8, 2020

Is there a reason this PR also introduces .clang-format ? Seems to really blow up the diff size with a lot of unrelated formatting changes.

No strong reason but having a tool/configuration for format would be handy, may be I can split that into a separate PR. I am using clang-format locally as there's no other formatting tool configured with SDK. If you have any other recommendations, please let me know. I can revert irrelavant changes from this PR and create a separate PR

@anilcse anilcse added C:Encoding T: Proto Breaking Protobuf breaking changes: generally don't do this! C:Types labels Sep 8, 2020
@alexanderbez
Copy link
Contributor

alexanderbez commented Sep 8, 2020

clang-format is a must for formatting proto files, but it should be done in a separate PR (which we can merge before this one).

@amaury1093
Copy link
Contributor

I saw you removed interfacer, do you know what are the linting errors related to?

@anilcse
Copy link
Collaborator Author

anilcse commented Sep 22, 2020

I saw you removed interfacer, do you know what are the linting errors related to?

There are a few //nolint interfacer declarations previously, its failing there. May be I will revert interfacer and add //nolint wherever we need to ignore this rule.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

This looks like it's ready to be merged! Nice work here @anilcse and @atheeshp!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 25, 2020
…bech32_addresses

# Conflicts:
#	proto/cosmos/auth/v1beta1/auth.proto
#	x/auth/types/account.go
#	x/auth/types/auth.pb.go
#	x/staking/handler_test.go
@mergify mergify bot merged commit d55c1a2 into master Sep 25, 2020
@mergify mergify bot deleted the anil/7195_bech32_addresses branch September 25, 2020 10:25
@fedekunze fedekunze mentioned this pull request Sep 29, 2020
9 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* init

* Fix bank proto messages

* missing conversions

* remove casttype for addresses

* Fix tests

* Fix consaddress

* more test fixes

* Fix tests

* fixed tests

* migrate missing proto declarations

* format

* Fix format

* Fix alignment

* Fix more tests

* Fix ibc merge issue

* Fix fmt

* Fix more tests

* Fix missing address declarations

* Fix staking tests

* Fix more tests

* Fix config

* fixed tests

* Fix more tests

* Update staking grpc tests

* Fix merge issue

* fixed failing tests in x/distr

* fixed sim tests

* fixed failing tests

* Fix bugs

* Add logs

* fixed slashing issue

* Fix staking grpc tests

* Fix all bank tests :)

* Fix tests in distribution

* Fix more tests in distr

* Fix slashing tests

* Fix statking tests

* Fix evidence tests

* Fix gov tests

* Fix bug in create vesting account

* Fix test

* remove fmt

* fixed gov tests

* fixed x/ibc tests

* fixed x/ibc-transfer tests

* fixed staking tests

* fixed staking tests

* fixed test

* fixed distribution issue

* fix pagination test

* fmt

* lint

* fix build

* fix format

* revert tally tests

* revert tally tests

* lint

* Fix sim test

* revert

* revert

* fixed tally issue

* fix tests

* revert

* fmt

* refactor

* remove `GetAddress()`

* remove fmt

* revert fmt.Striger usage

* Fix tests

* Fix rest test

* disable interfacer lint check

* make proto-format

* add nolint rule

* remove stray println

Co-authored-by: aleem1314 <aleem.md789@gmail.com>
Co-authored-by: atheesh <atheesh@vitwit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Encoding C:Types T: Proto Breaking Protobuf breaking changes: generally don't do this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Properly handle gRPC and CLI's bech32 serialization of account addresses
9 participants