refactor(sims): rm usage of GetSignBytes from NewOperationMsg #16036
Conversation
kocubinski
force-pushed
the
kocubinski/sims-getsignbytes
branch
from
2a13f01 to
82ca71f
Compare
github-prbot
requested review from
a team,
mark-rushakoff and
julienrbrt
and removed request for
a team
types/simulation/import.go
Outdated
| _ "cosmossdk.io/api/cosmos/upgrade/v1beta1" | ||
| _ "cosmossdk.io/api/cosmos/vesting/module/v1" | ||
| _ "cosmossdk.io/api/cosmos/vesting/v1beta1" | ||
| ) |
There was a problem hiding this comment.
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.HybridResolver as an option to aminojson.NewEncoder and be done, without all these imports
There was a problem hiding this comment.
I'm not following your suggestion. The usage in question in the aminojson package requires a protoregistry.MessageTypeResolver, not a ProtoFileResolver which is what a photo.HybridResolver is.
There was a problem hiding this comment.
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.
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.
the encoder should be parameterized by both file resolver and type resolver. See what autocli does
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.
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.
We can merge both, I don't see them as blocking, but all right.
types/simulation/types.go
Outdated
| if err != nil { | ||
| panic(err) | ||
| } | ||
| encoder := aminojson.NewAminoJSON() |
There was a problem hiding this comment.
| encoder := aminojson.NewAminoJSON() | |
| encoder := aminojson.NewEncoder() |
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.
#16044
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err != nil { | ||
| panic(fmt.Errorf("failed to marshal msg: %w", err)) | ||
| } | ||
| err = proto.Unmarshal(gogoBytes, dynamicMsg) |
Check warning
Code scanning / CodeQL
Useless assignment to local variable
|
implemented in #16048 |
Description
Ref: #16024
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change