refactor(sims): rm usage of GetSignBytes from NewOperationMsg #16036
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package simulation | ||
|
|
||
| import ( | ||
| // NewOperationMsg (in this package) requires all the types to be imported for the amionJSON marshal to properly | ||
| // handle any types, and any types are required to support gogo types being generated, since they are not | ||
| // proto reflect capable. | ||
| _ "cosmossdk.io/api/cosmos/app/runtime/v1alpha1" | ||
| _ "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
| _ "cosmossdk.io/api/cosmos/auth/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/auth/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/authz/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/authz/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/autocli/v1" | ||
| _ "cosmossdk.io/api/cosmos/bank/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/bank/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/abci/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/node/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/query/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/reflection/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/reflection/v2alpha1" | ||
| _ "cosmossdk.io/api/cosmos/base/tendermint/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/base/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/consensus/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/consensus/v1" | ||
| _ "cosmossdk.io/api/cosmos/crisis/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/crisis/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/crypto/ed25519" | ||
| _ "cosmossdk.io/api/cosmos/crypto/hd/v1" | ||
| _ "cosmossdk.io/api/cosmos/crypto/multisig" | ||
| _ "cosmossdk.io/api/cosmos/crypto/multisig/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/crypto/secp256k1" | ||
| _ "cosmossdk.io/api/cosmos/crypto/secp256r1" | ||
| _ "cosmossdk.io/api/cosmos/distribution/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/distribution/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/evidence/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/evidence/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/feegrant/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/feegrant/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/genutil/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/genutil/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/gov/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/gov/v1" | ||
| _ "cosmossdk.io/api/cosmos/gov/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/group/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/group/v1" | ||
| _ "cosmossdk.io/api/cosmos/mint/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/mint/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/msg/textual/v1" | ||
| _ "cosmossdk.io/api/cosmos/msg/v1" | ||
| _ "cosmossdk.io/api/cosmos/nft/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/nft/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/orm/module/v1alpha1" | ||
| _ "cosmossdk.io/api/cosmos/orm/query/v1alpha1" | ||
| _ "cosmossdk.io/api/cosmos/orm/v1" | ||
| _ "cosmossdk.io/api/cosmos/orm/v1alpha1" | ||
| _ "cosmossdk.io/api/cosmos/params/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/params/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/query/v1" | ||
| _ "cosmossdk.io/api/cosmos/reflection/v1" | ||
| _ "cosmossdk.io/api/cosmos/slashing/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/slashing/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/staking/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/staking/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/store/snapshots/v1" | ||
| _ "cosmossdk.io/api/cosmos/store/streaming/abci" | ||
| _ "cosmossdk.io/api/cosmos/store/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/tx/config/v1" | ||
| _ "cosmossdk.io/api/cosmos/tx/signing/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/tx/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/upgrade/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/upgrade/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/vesting/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/vesting/v1beta1" | ||
| ) | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,11 +5,14 @@ import ( | |||||
| "math/rand" | ||||||
| "time" | ||||||
|
|
||||||
| "google.golang.org/protobuf/types/known/anypb" | ||||||
|
|
||||||
| "cosmossdk.io/x/tx/signing/aminojson" | ||||||
| "github.com/cosmos/cosmos-sdk/baseapp" | ||||||
| "github.com/cosmos/cosmos-sdk/codec" | ||||||
| "github.com/cosmos/cosmos-sdk/codec/types" | ||||||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
| "github.com/cosmos/cosmos-sdk/types/kv" | ||||||
| ) | ||||||
|
|
||||||
| // Deprecated: Use WeightedProposalMsg instead. | ||||||
|
|
@@ -95,11 +98,20 @@ func NewOperationMsg(msg sdk.Msg, ok bool, comment string, cdc *codec.ProtoCodec | |||||
| moduleName = msgType | ||||||
| } | ||||||
|
|
||||||
| anyMsg, err := types.NewAnyWithValue(msg) | ||||||
| if err != nil { | ||||||
| panic(err) | ||||||
| } | ||||||
| encoder := aminojson.NewAminoJSON() | ||||||
|
There was a problem hiding this comment.
Suggested change
The constructor name should match the type name There was a problem hiding this comment. Agreed. This should be a separate PR since it necessitates an x/tx release, please review when you get a chance. |
||||||
| msgBytes, err := encoder.Marshal(&anypb.Any{ | ||||||
| TypeUrl: anyMsg.TypeUrl, | ||||||
| Value: anyMsg.Value, | ||||||
| }) | ||||||
| if err != nil { | ||||||
| panic(err) | ||||||
| } | ||||||
|
|
||||||
| return NewOperationMsgBasic(moduleName, msgType, comment, ok, msgBytes) | ||||||
| } | ||||||
|
|
||||||
| // NoOpMsg - create a no-operation message | ||||||
|
|
||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
None of this should be necessary. This is only needed because the amino JSON encoder isn't properly setup to take a custom file resolver and use dynamicpb when there are is no concrete type. It should be possible to pass in
proto.HybridResolveras an option toaminojson.NewEncoderand be done, without all these importsThere 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'm not following your suggestion. The usage in question in the aminojson package requires a
protoregistry.MessageTypeResolver, not aProtoFileResolverwhich is what aphoto.HybridResolveris.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've moved the type resolution step up into this sims package to prevent passing an any to aminojson.Encoder, it would need an exclusion with types without amino.name. But I still need a typeResolver to get a protoreflect enabled msg for aminojson.Encoder. Any suggestions?
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.
Use dyncamicpb + the file resolver if the typeresolver doesn't have the type like I was saying above. Also the encoder should be parameterized by both file resolver and type resolver. See what autocli does
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.
Can we tackle this in a separate PR? I don't think any change is required here for sims to remove this last usage.
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 guess so but addressing this issue with aminojson takes priority in my opinion. We said that it would work dynamically without depending on the API module. This PR has uncovered that this is not true. So we need to address that
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.
We can merge both, I don't see them as blocking, but all right.