tendermint / go-crypto

DEPRECATED: Merged into https://github.com/tendermint/tendermint under `crypto`
Other
44 stars 32 forks source link

Change XxxS anonymous field name to non-anonymous fields named Value #5

Closed rigelrozanski closed 7 years ago

rigelrozanski commented 7 years ago

This applies across the board in go-crypto, change all the type XxxS struct, to have a Value Xxx field instead of just the anonymous interface?

https://github.com/tendermint/go-crypto/blob/3f47cfac5fcd9e0f1727c7db980b3559913b3e3a/signature.go#L31

Example change, From:

type SignatureS struct {
    Signature
}

to

type SignatureS struct {
    Value Signature
}
ethanfrey commented 7 years ago

This solution does make the constructor safer, but it seems harder to use for me. I made an example of another approach in 8ff4ce2 (feature/improve-godata), which rejects manually constructing the XxxS objects (which is where the error came from), rather using a WrapXxx() constructor that makes sure we don't nest multiple levels.

This was inspired by the usage of embedding anonymous interfaces in github.com/pkg/errors Especially look at:

I combined the unwrapping and the constructor in one, as here there is no need to have multiple nested levels (while nested errors are useful in pkg/errors if handled properly).

Please take a look and let me know what you think about that. (It's not much code, but faster to write it than explain what I intend to do).

ethanfrey commented 7 years ago

Solution agreed to at the meeting (which solves this) is being worked on in #6

If you can "fix" the issue about not being able to use go-wire with private fields, that would be AWESOME. (But either hacking go runtime or modifying go-wire).

jaekwon commented 7 years ago

Seconded. Lets see how #6 goes.