Migrate BaseAccount PubKey to use Any #7268
Conversation
…ronc/revert-5682
…ronc/revert-5682 # Conflicts: # testutil/testdata/proto.pb.go
…ronc/revert-5682
…ronc/revert-5682
| return pk | ||
| content, ok := acc.PubKey.GetCachedValue().(crypto.PubKey) | ||
| if !ok { | ||
| return nil |
There was a problem hiding this comment.
why do we return nil here? Shouldn't we deserialize and set cache?
There was a problem hiding this comment.
it's a getter, how would you set the cached value? not sure to get what you mean.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return string(bz), err |
There was a problem hiding this comment.
Marshalers return []byte normally. Why do we convert to string?
There was a problem hiding this comment.
not sure why, removing it doesn't seem to cause any issue but I'd rather get other contributors's opinion on that first as it doesn't really relate to changes from this PR anyway.
There was a problem hiding this comment.
hmm, I'm also not sure why. All MarshalYAML in the codebase return string, so at least it's consistent. Might be a follow-up PR to switch all to bytes, but it's a low-priority cleanup, I added it to #7357.
types/address.go
Outdated
| // This piece of code is to keep backwards-compatibility. | ||
| // For ed25519 keys, our own ed25519 is registered in Amino under a | ||
| // different name than TM's ed25519. But since users are already using | ||
| // TM's ed25519 bech32 encoding, we explicitly say to bech32-encode our own | ||
| // ed25519 the same way as TM's ed25519. | ||
| pkToMarshal := pubkey | ||
| if ed25519Pk, ok := pubkey.(*ed25519.PubKey); ok { | ||
| pkToMarshal = ed25519Pk.AsTmPubKey() | ||
| } |
There was a problem hiding this comment.
Added suggested todo with link to issue.
Co-authored-by: Cory <[email protected]>
Co-authored-by: Cory <[email protected]>
I believe @amaurymartiny added all required follow-ups in #7357. |
In Tendermint we are currently only support ed25519 for this release. This was done in order to reduce the scope of work because we thought the release would be a few months ago. We will rework this part of Tendermint in the future to not have the type hardcoded. As for the going to Tendermint key and sdk key, I am a bit scared of this design but it works for now. The dependency of Tendermint will change quite a bit in the coming releases. |
There was a problem hiding this comment.
I believe this PR can be merged, follow-up work is tracked in #7357
@robert-zaremba if code comments are not clear in your opinion, feel free to push directly to this PR, it's better (as you said yesterday) than to go back and forth here.
@marbar3778 Could you be a little bit more concrete why you are scared? Do you already have an idea of TM's refactor and how it'll impact the SDK having our own ed25519 like in this PR? |
Having to be aware of which key is being used and where. It is easy to miss something. The refactor of Tendermint will mainly hide more of the API so how things are done currently in places where Tendermint is used directly will need to be refactored to not use it. |
|
@amaurymartiny , @marbar3778 -- exactly. This was my whole comment about. We should relay on interfaces and have good tests. We added followup checkpoint to solve this in the future. |
amaury1093
added
the
A:automerge
* WIP on BaseAccount protobuf pub_key * WIP on BaseAccount.PubKey * WIP on BaseAccount pub key * Update PubKey * Update Account * Docs * WIP on protobuf keys * Use Type() and Bytes() in sr25519 pub key Equals * Add tests * Add few more tests * Update other pub/priv key types Equals * Fix PrivKey's Sign method * Rename variables in tests * Fix infinite recursive calls * Use tm ed25519 keys * Add Sign and VerifySignature tests * Remove ed25519 and sr25519 references * proto linting * Add proto crypto file * Implement some of the new multisig proto type methods * Add tests for MultisigThresholdPubKey * Add tests for pubkey pb/amino conversion functions * Move crypto types.go and register new proto pubkeys * Add missing pointer ref * Address review comments * panic in MultisigThresholdPubKey VerifySignature * Fix compile errors * Remove pk conversion in ante handler * Use internal crypto.PubKey in multisig * Add tests for MultisigThresholdPubKey VerifyMultisignature * Only keep LegacyAminoMultisigThresholdPubKey and move to proto keys to v1 * Remove conversion functions and introduce internal PubKey type * Override Amino marshaling for proto pubkeys * Merge master * Make proto-gen * Start removal of old PubKeyMultisigThreshold references * Fix tests * Fix solomachine * Fix ante handler tests * Pull latest go-amino * Remove ed25519 * Remove old secp256k1 PubKey and PrivKey * Uncomment test case * Fix linting issues * More linting * Revert tests keys values * Add Amino overrides to proto keys * Add pubkey test * Fix tests * Use threshold isntead of K * Standardize Type * Revert standardize types commit * Fix build * Fix lint * Fix lint * Add comment * Register crypto.PubKey * Add empty key in BuildSimTx * Simplify proto names * Unpack interfaces for signing desc * Fix IBC tests? * Format proto * Use secp256k1 in ibc * Fixed merge issues * Uncomment tests * Update x/ibc/testing/solomachine.go * UnpackInterfaces for solomachine types * Remove old multisig * Add amino marshal for multisig * Fix lint * Correctly register amino * One test left! * Remove old struct * Fix test * Fix test * Unpack into tmcrypto * Remove old threshold pubkey tests * Fix register amino * Fix lint * Use sdk crypto PubKey in multisig UnpackInterfaces * Potential fix? * Register LegacyAminoPubKey * Register our own PubKey * Register tmcrypto PubKey * Register both PubKeys * Register interfaces in test * Refactor fiels * Add comments * Remove old cosmos-sdk/crypto/keys reference * Use anil's suggestion * Add norace back * Use our own ed25519 * Fix pubkey types * Fix network tests * Fix more tests * Make ibc work? * Use TM pubkey in NewValidator * Fix lint * Put interface in tpyes * rerun CI * Better name register * Remove stray code * Add ed25519 tests * Check nil * Correct interface impl assert * rerun CI * Add fix for Bech32 * Address comments * FIx lint * Add tests for solomachine unpack interfaces * Fix query tx by hash * Better name in amino register * Fix lint * Add back ed25519 test (fixes cosmos#7352) * go mod tidy * Fix merge issues * Sort import * Add test for backwards-compatibility * Fix tests * Fix merge issue * Update client/context.go Co-authored-by: Cory <[email protected]> * Update types/address.go Co-authored-by: Cory <[email protected]> * Address feedback * Add comment * Fix BaseAccount SetPubKey and address further comments * Lint * Remove unnecessary use of copy in getPubKeyFromString * Update comment Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Alexander Bezobchuk <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Cory <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
ref: #6886 (comment)
closes: #6886
Also add back ed25519 support to the SDK:
Keyfield is[]byteWorkarounds added:
AsTmPubKey()method on the proto type, to convert our own ed25519 into TM's ed25519. Where TM expects a TM ed25519 (e.g. thetmtypes.NewValidator()function), we need to make sure to pass inpubkey.AsTmPubKey()Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes