cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.18k stars 3.57k forks source link

Keep or remove SDK crypto.Address #8775

Open robert-zaremba opened 3 years ago

robert-zaremba commented 3 years ago

Summary

SDK /crypto/types.Address is an alias to tendermint crypto.Address.

Problem Definition

Type aliases are used for refactoring or when there is a need for moving a type in a local scope (usually using a private types). Otherwise they bring zero value. We end up with a dead type which we can't customize.

Proposal

Remove SDK Address. Otherwise provide justification for keeping the type alias. In the latter case - update SDK consistently to use SDK Address

Related to:


For Admin Use

robert-zaremba commented 3 years ago

cc: @aaronc , @amaurym , @alessio , @alexanderbez - What do you think about removing the type alias. It's a quick thing, removes disambiguation.

aaronc commented 3 years ago

We actually want the SDK's PubKey type to not have a dependency on tendermint. So I actually think we want to make the type alias a proper type and independent of tendermint.

robert-zaremba commented 3 years ago

We have many dependencies to tendermint. If the interface is good, why we would need to create an alias? Other, better approach would be to extend the interface when needed, eg:

type Address interface {
  tmcrypto.Address

  SomethingExtra()
}
robert-zaremba commented 3 years ago

In general - we should avoid changing interfaces when possible.

aaronc commented 3 years ago

We wanted to diverge from the TM PubKey for a variety of reasons. There are prior issues you can find where this was discussed. I think we should stay on that course.

aaronc commented 3 years ago

That said maybe there's no need to do anything about Address

robert-zaremba commented 3 years ago

Currently in SDK we are using both tmcrypto.Address and sdkcrypto.Address. Changing an interface is a breaking change and will require more work anyway. There is a chance that most of the user code could work with tmcrypto.Address if we only extend the interface and use won't use the extensions. So my preference would be to remove the alias and do a new type once needed.

robert-zaremba commented 3 years ago

@alessio , @alexanderbez - do you have an opinion here?

alessio commented 3 years ago

This

https://github.com/cosmos/cosmos-sdk/blob/e73cf2b410793070c5cec48014dbac21281185f5/crypto/types/types.go#L42

is an alias of tmcrypto.Address, which in turn is an alias of HexBytes:

type Address = bytes.HexBytes

Yes, let's remove github.coms/cosmos/cosmos-sdk/crypto.Address, please.

robert-zaremba commented 3 years ago

Shall we include it in 0.44?