Dependency on go-amino for generating addresses

[amrali]

On update-go-amino branch there's this code

go-crypto/pub_key.go

Line 49 in 73407e7

bz, err := cdc.MarshalBinary(pubKey)

and similar code on the master branch.

In TM core the PrivValidatorFS struct depends on PubKeyEd25519.Address method to get an address, using amino.MarshalBinary effectively makes the address generation process dependent on go-amino. Is this intentional? Shouldn't marshalling data be reserved for communication across the wire or for serialization on disk?

Pain: I wasted hours trying to figure out why my ripemd160 hash of append([]byte{TypeEd25519}, pubkey...) doesn't match the address fixture for public keys in unit tests.

[jaekwon]

Bytes() is marshalling. Sorry about the pain, we're going through a final refactor for our marshaling library (from go-wire to go-amino). It is now stable.

[jaekwon]

@amrali Are you implementing go-crypto's functionality in another language?

If not, in Go, users of go-crypto should never have to do things like append([]byte{TypeEd25519}, pubkey...) because the high-level exported functions of go-crypto provide that logic for you.

[jaekwon]

Also, it may not make sense now, but we certainly will have more and more PubKey etc types that require a codec for properly (un)marshaling complex structures, so that we can compute the address in a safe and efficient way. For example, we will support ThresholdPubKey soon which combines multiple PubKeys together. Amino helps a lot here.

[amrali]

I'm implementing a small address function in Python to generate an address based on Ed25519 public key. The problem I'm anticipating is that if we rely on go-amino for address generation then by extension anyone wanting to write a wallet software for example would have no choice but to do it in golang or port go-amino to their language of choice.

I find it odd that address generation relies on marshalling (of any type and form) where it should be a rather simple structure (e.g., prefix key-type code to the publickey before applying ripemd160 to obtain the address), having other bytes that describe the length or data type of the public key is part of a marshalling protocol and is not suitable for address generation.

To reiterate my position, I'm not against go-amino's marshalling (although why not just use protobuf, kinda defeats the point by reinventing the wheel and sacrificing a lot of support that already exists with protobuf) I just believe that things like address generation should be as portable and easy to implement as possible.

[ebuchman]

having other bytes that describe the length or data type of the public key is part of a marshalling protocol and is not suitable for address generation

There is some precedent, ie. Bitcoin's pubkeys use the OpenSSL encoding (prefix a 02 or 03 for compressed keys depending on the y-value, or a 04 for uncompressed).

Ethereum also just keeps the 04 around.

why not just use protobuf

Amino is now identical to Protobuf3 except for how we handle interface types (oneof) using prefix bytes. Have you seen https://github.com/tendermint/go-amino#amino-vs-protobuf3 ?

To implement the address generation then, I believe you'd just need: <prefix bytes><varint of length><raw bytes>

[amrali]

There is some precedent, ie. Bitcoin's pubkeys use the OpenSSL encoding (prefix a 02 or 03 for compressed keys depending on the y-value, or a 04 for uncompressed).

That qualifies as key type, but not wire protocol details (i.e., data length and type for object schema)

[ebuchman]

Ah yes, very fair point.

What would you propose in the case of say threshold signature address when we have many pubkeys ? Just concatenate them together ?

Our Secp256k1 address uses the identical bitcoin format, and we're locked to that because it was used in the Cosmos fundraiser. For others we have some flexibility. I agree, the address generation should be as easy and straightforward as possible. That said, with Amino, "types" are denoted via a set of 4-7 bytes, rather than just any one, so it might be weird to maintain two distinct typing schemes for the same objects like only using 1-byte for the address generation and 4-bytes for everything else

[amrali]

I agree that this is a hard problem to solve without having a schema to adhere to. And having an ad-hoc schema to just describe addresses is, well, ad-hoc. We should at least agree that the lack of schema documentation and the lock-in of Amino is not desirable, specially when the goal is to garner adoption and support from the community.

If Amino is essentially protobuf with a custom built schema binding layer for golang, then sure, but where's the protobuf schema definition files? Here comes a cost on people like me where we need immediate access to tools in C/C++/Python (the major 3 that most code is written in and the most developer support as well) to allow them to carry out simple tasks like generating an address.

What would you propose in the case of say threshold signature address when we have many pubkeys ? Just concatenate them together ?

Yes and no, threshold signature keys need not be together, so I don't see the need to put them together either through concatenating them or otherwise. I do see what you are getting at, and again, I agree. But let's use widely supported tools to do that, with multi-language support. If not, let's start with documenting a schema first before jumping right to implementation so that people can implement said schema in a compatible way with their own tools. Although having a custom schema and not relying on some sort of "standard" increases the barrier to entry, and for a young project like tendermint/cosmos I'd imagine that's not something we want.

[schuyler]

This has been a serious pain point for us as well. We have lost days of work trying to ascertain how this all works.

[ebuchman]

Sorry for the trouble. Closing this issue for #103

We'll make sure to have excellent documentation and lots of communication about it before this is released.