-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
x/ibc proto migration #5704
x/ibc proto migration #5704
Conversation
protoValset, err := tmVals.ToProto() | ||
if err != nil { | ||
// TODO: should this return false instead? | ||
panic(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-reminder: address
@@ -34,6 +35,8 @@ func New() *Codec { | |||
// codec. | |||
func RegisterCrypto(cdc *Codec) { | |||
cryptoamino.RegisterAmino(cdc) | |||
cryptoamino.RegisterKeyType(protokeys.PrivateKey{}, "tendermint/proto/PrivateKey") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marbar3778 I'm getting some errors with the encoding because it doesn't recognize the new priv/pubkey. Can you take a look when you have time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will have to define your own proto type. we decided to only support ed25519 in tendermints proto key types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a pr to show how this could be done simply: #5997
if this works i can work on completing that PR
@@ -1,7 +1,7 @@ | |||
syntax = "proto3"; | |||
package tendermint.proto.crypto; | |||
package tendermint.proto.crypto.keys; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you dont need this file as you will have to define your own proto type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite have the proto
experience to comment in detail, but the basic structure here looks solid to me, particularly the oneof
types corresponding to abstractions (e.g. client types) in IBC, that's exactly what we want.
Per discussion with @fedekunze this is out-of-date but updating it is blocked on the Tendermint protobuf changes upstream (for ICS 7 client migration to protobuf). |
replaced by #6254 |
Description
Switches IBC module from amino to Protobuf encoding
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)