-
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
Proto Tx with Any #7276
Proto Tx with Any #7276
Conversation
… into marie/protobuf-pubkeys
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.
Just a small comment, otherwise lgtm.
RE: #7281 QueryTxByHash now works off this PR from the command line. For legacy Amino Rest, I get
|
thanks @zmanian for testing, I'll fix this before merging the PR. |
|
||
if t.AuthInfo != nil { | ||
return t.AuthInfo.UnpackInterfaces(unpacker) | ||
} |
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.
Shouldn't we return error if both Body == nil
or AuthInfo==nil
?
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.
UnpackInterfaces just unpacks all Any
s in the struct's nested fields. If there are no such fields, then it should do nothing, I believe it's better not to error here.
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.
When a Body
can be empty? Is this expected?
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.
Yeah I see what you mean. The Body
of a Tx will never be empty indeed.
My idea is, UnpackInterfaces
should not care about the logic of the struct, it should not "know" that Body
cannot be nil, it should just unpack recursively, when possible.
Thanks for the review, esp for the 6line->1line change! |
types/tx/types.go
Outdated
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
// This is used when we want to marshal a pubkey using Amino. | ||
// ref: https://github.com/cosmos/cosmos-sdk/issues/7281 | ||
cdc.RegisterInterface((*isModeInfo_Sum)(nil), nil) | ||
cdc.RegisterConcrete(ModeInfo_Single_{}, | ||
"cosmos.tx.v1beta1.ModeInfo_Single", nil) | ||
cdc.RegisterConcrete(ModeInfo_Multi_{}, | ||
"cosmos.tx.v1beta1.ModeInfo_Multi", nil) | ||
} |
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.
In the REST output, here's what I get:
// --snip--
"auth_info": {
"signer_infos": [
{
"public_key": {
"type": "tendermint/PubKeySecp256k1",
"value": "A3xJ+aIUmryjhUU2ZftMbQPRsYB57AtAFQug4kUSu5sm"
},
"mode_info": {
"Sum": {
"type": "cosmos.tx.v1beta1.ModeInfo_Single",
"value": {
"single": {
"mode": 127
}
}
}
}
}
],
// --snip--
it's breaking from what the REST endpoint was previously showing, but is that okay?
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.
this API breakage seems okay to me and difficult to avoid.
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.
After talking with @aaronc, we decided to convert the proto Tx to a StdTx before displaying it as JSON in this endpoint. This means that the output won't be as above, but similar to what was there before.
I believe this is ready to be merged. Putting automerge on! |
* 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 * 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 * Use anil's suggestion * Add norace back * Check nil * Address comments * FIx lint * Add tests for solomachine unpack interfaces * Fix query tx by hash * Better name in amino register * Display StdTx instead of proto Tx * Remove useless check Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: blushi <[email protected]> Co-authored-by: Alexander Bezobchuk <[email protected]> Co-authored-by: colin axnér <[email protected]>
Description
Replace usage of public keys from
PublicKey
(theoneof
) toAny
, everywhere except in BaseAccount (this will be done in a separate PR, as it's more involved).ref: #6886 (comment)
depends on:
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/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes