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

Keep or remove SDK crypto.Address #8775

Open
3 of 4 tasks
robert-zaremba opened this issue Mar 3, 2021 · 10 comments
Open
3 of 4 tasks

Keep or remove SDK crypto.Address #8775

robert-zaremba opened this issue Mar 3, 2021 · 10 comments
Labels
dependencies Pull requests that update a dependency file T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Mar 3, 2021

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

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). dependencies Pull requests that update a dependency file labels Mar 3, 2021
@robert-zaremba robert-zaremba self-assigned this Mar 3, 2021
@robert-zaremba
Copy link
Collaborator Author

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

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

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 added a commit that referenced this issue Mar 3, 2021
This reverts commit 15b866a.
This issue will be solved in #8775
@robert-zaremba
Copy link
Collaborator Author

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
Copy link
Collaborator Author

In general - we should avoid changing interfaces when possible.

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

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
Copy link
Member

aaronc commented Mar 3, 2021

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

@robert-zaremba
Copy link
Collaborator Author

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.

mergify bot pushed a commit that referenced this issue Mar 4, 2021
* Optimize secp256k1 hashing

* Add ADR-028 related functions

* Update ed25519

* fix errors/handle

* fix build

* fix build

* Add tests and update function names

* wip

* Use LengthPrefix for composed addresses

* add tests for NewComposed

* add module hash function

* fix append

* rollback ed25519 ADR-28 update

* rollback ed25519 ADR-28 test

* Adding Module tests and convert tests to test suite

* convert store_key_test.go to test suite

* rollback test check comment

* any.pb.go update

* generated proto files

* wip

* renames

* wip2

* add String method to PBBytes

* wip3

* add pubkey tests

* adding cryptotypes.PrivKey methods

* re-enable test

* fix equals test

* fix ecdsa object receiver

* add ProtoMarshaler implementation and tests

* move code to init and add interface registry

* add bytes tests

* merge Unmarshal with UnmarshalAmino

* implement ProtoMarshaler to ecdsaSK

* remove bytes.go

* add private key marshaling tests

* break tests into 2 suites

* add signature tests

* remove TODO

* remove bytes.proto

* adding changelog

* Update CHANGELOG.md

* Update crypto/keys/ecdsa/ecdsa_privkey.go

* Update crypto/keys/ecdsa/ecdsa_pubkey.go

* comments: add dot (.) at the end

* update comments

* update commented code

* rename files

* remove Amino methods

* use 2 spaces in protocgen.sh

* rollback changes in protocgen.sh

* add MessageName

* rework ecdsa proto structure

* move ecdsa to internal package

* add secp256r1 proto

* refactore proto definition for secp256r1

* fix err check

* update comments

* create const for fieldSize+1

* simplify the PubKey.String test

* Apply suggestions from code review

Co-authored-by: Jonathan Gimeno <[email protected]>

* Update doc comments: SDK Interface -> sdk.Interface

* rename init.go to doc.go

* Add PubKey.Type() test

* Revert "Update doc comments: SDK Interface -> sdk.Interface"

This reverts commit 01f2b4f.

* Use cryptotypes.Address instead of tmcrypto

* Revert "Use cryptotypes.Address instead of tmcrypto"

This reverts commit 15b866a.
This issue will be solved in #8775

* add link to ANSI X9.62

* move init.go -> doc.go

* use proto.MessageName()

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
@robert-zaremba
Copy link
Collaborator Author

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

@alessio
Copy link
Contributor

alessio commented Mar 11, 2021

This

Address = tmcrypto.Address

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.

@github-actions github-actions bot added the stale label Aug 7, 2021
daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this issue Aug 19, 2021
* Optimize secp256k1 hashing

* Add ADR-028 related functions

* Update ed25519

* fix errors/handle

* fix build

* fix build

* Add tests and update function names

* wip

* Use LengthPrefix for composed addresses

* add tests for NewComposed

* add module hash function

* fix append

* rollback ed25519 ADR-28 update

* rollback ed25519 ADR-28 test

* Adding Module tests and convert tests to test suite

* convert store_key_test.go to test suite

* rollback test check comment

* any.pb.go update

* generated proto files

* wip

* renames

* wip2

* add String method to PBBytes

* wip3

* add pubkey tests

* adding cryptotypes.PrivKey methods

* re-enable test

* fix equals test

* fix ecdsa object receiver

* add ProtoMarshaler implementation and tests

* move code to init and add interface registry

* add bytes tests

* merge Unmarshal with UnmarshalAmino

* implement ProtoMarshaler to ecdsaSK

* remove bytes.go

* add private key marshaling tests

* break tests into 2 suites

* add signature tests

* remove TODO

* remove bytes.proto

* adding changelog

* Update CHANGELOG.md

* Update crypto/keys/ecdsa/ecdsa_privkey.go

* Update crypto/keys/ecdsa/ecdsa_pubkey.go

* comments: add dot (.) at the end

* update comments

* update commented code

* rename files

* remove Amino methods

* use 2 spaces in protocgen.sh

* rollback changes in protocgen.sh

* add MessageName

* rework ecdsa proto structure

* move ecdsa to internal package

* add secp256r1 proto

* refactore proto definition for secp256r1

* fix err check

* update comments

* create const for fieldSize+1

* simplify the PubKey.String test

* Apply suggestions from code review

Co-authored-by: Jonathan Gimeno <[email protected]>

* Update doc comments: SDK Interface -> sdk.Interface

* rename init.go to doc.go

* Add PubKey.Type() test

* Revert "Update doc comments: SDK Interface -> sdk.Interface"

This reverts commit 01f2b4f5efcd79a452483bcda152db54a8fbfee2.

* Use cryptotypes.Address instead of tmcrypto

* Revert "Use cryptotypes.Address instead of tmcrypto"

This reverts commit 15b866ae67bdb7ca4872f4089fcab19f9e2e3608.
This issue will be solved in cosmos/cosmos-sdk#8775

* add link to ANSI X9.62

* move init.go -> doc.go

* use proto.MessageName()

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
@robert-zaremba
Copy link
Collaborator Author

Shall we include it in 0.44?

daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this issue Aug 20, 2021
* Optimize secp256k1 hashing

* Add ADR-028 related functions

* Update ed25519

* fix errors/handle

* fix build

* fix build

* Add tests and update function names

* wip

* Use LengthPrefix for composed addresses

* add tests for NewComposed

* add module hash function

* fix append

* rollback ed25519 ADR-28 update

* rollback ed25519 ADR-28 test

* Adding Module tests and convert tests to test suite

* convert store_key_test.go to test suite

* rollback test check comment

* any.pb.go update

* generated proto files

* wip

* renames

* wip2

* add String method to PBBytes

* wip3

* add pubkey tests

* adding cryptotypes.PrivKey methods

* re-enable test

* fix equals test

* fix ecdsa object receiver

* add ProtoMarshaler implementation and tests

* move code to init and add interface registry

* add bytes tests

* merge Unmarshal with UnmarshalAmino

* implement ProtoMarshaler to ecdsaSK

* remove bytes.go

* add private key marshaling tests

* break tests into 2 suites

* add signature tests

* remove TODO

* remove bytes.proto

* adding changelog

* Update CHANGELOG.md

* Update crypto/keys/ecdsa/ecdsa_privkey.go

* Update crypto/keys/ecdsa/ecdsa_pubkey.go

* comments: add dot (.) at the end

* update comments

* update commented code

* rename files

* remove Amino methods

* use 2 spaces in protocgen.sh

* rollback changes in protocgen.sh

* add MessageName

* rework ecdsa proto structure

* move ecdsa to internal package

* add secp256r1 proto

* refactore proto definition for secp256r1

* fix err check

* update comments

* create const for fieldSize+1

* simplify the PubKey.String test

* Apply suggestions from code review

Co-authored-by: Jonathan Gimeno <[email protected]>

* Update doc comments: SDK Interface -> sdk.Interface

* rename init.go to doc.go

* Add PubKey.Type() test

* Revert "Update doc comments: SDK Interface -> sdk.Interface"

This reverts commit 01f2b4f5efcd79a452483bcda152db54a8fbfee2.

* Use cryptotypes.Address instead of tmcrypto

* Revert "Use cryptotypes.Address instead of tmcrypto"

This reverts commit 15b866ae67bdb7ca4872f4089fcab19f9e2e3608.
This issue will be solved in cosmos/cosmos-sdk#8775

* add link to ANSI X9.62

* move init.go -> doc.go

* use proto.MessageName()

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
daeMOn63 pushed a commit to fetchai/cosmos-sdk that referenced this issue Mar 1, 2022
* Optimize secp256k1 hashing

* Add ADR-028 related functions

* Update ed25519

* fix errors/handle

* fix build

* fix build

* Add tests and update function names

* wip

* Use LengthPrefix for composed addresses

* add tests for NewComposed

* add module hash function

* fix append

* rollback ed25519 ADR-28 update

* rollback ed25519 ADR-28 test

* Adding Module tests and convert tests to test suite

* convert store_key_test.go to test suite

* rollback test check comment

* any.pb.go update

* generated proto files

* wip

* renames

* wip2

* add String method to PBBytes

* wip3

* add pubkey tests

* adding cryptotypes.PrivKey methods

* re-enable test

* fix equals test

* fix ecdsa object receiver

* add ProtoMarshaler implementation and tests

* move code to init and add interface registry

* add bytes tests

* merge Unmarshal with UnmarshalAmino

* implement ProtoMarshaler to ecdsaSK

* remove bytes.go

* add private key marshaling tests

* break tests into 2 suites

* add signature tests

* remove TODO

* remove bytes.proto

* adding changelog

* Update CHANGELOG.md

* Update crypto/keys/ecdsa/ecdsa_privkey.go

* Update crypto/keys/ecdsa/ecdsa_pubkey.go

* comments: add dot (.) at the end

* update comments

* update commented code

* rename files

* remove Amino methods

* use 2 spaces in protocgen.sh

* rollback changes in protocgen.sh

* add MessageName

* rework ecdsa proto structure

* move ecdsa to internal package

* add secp256r1 proto

* refactore proto definition for secp256r1

* fix err check

* update comments

* create const for fieldSize+1

* simplify the PubKey.String test

* Apply suggestions from code review

Co-authored-by: Jonathan Gimeno <[email protected]>

* Update doc comments: SDK Interface -> sdk.Interface

* rename init.go to doc.go

* Add PubKey.Type() test

* Revert "Update doc comments: SDK Interface -> sdk.Interface"

This reverts commit 01f2b4f5efcd79a452483bcda152db54a8fbfee2.

* Use cryptotypes.Address instead of tmcrypto

* Revert "Use cryptotypes.Address instead of tmcrypto"

This reverts commit 15b866ae67bdb7ca4872f4089fcab19f9e2e3608.
This issue will be solved in cosmos/cosmos-sdk#8775

* add link to ANSI X9.62

* move init.go -> doc.go

* use proto.MessageName()

Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Jonathan Gimeno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

No branches or pull requests

3 participants