-
Notifications
You must be signed in to change notification settings - Fork 373
Avoid generic parameter in CosmosMsg<C>
#2249
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
Comments
What are the use cases of the generic param in func DefaultEncoders(unpacker codectypes.AnyUnpacker, portSource types.ICS20TransferPortSource) MessageEncoders {
return MessageEncoders{
Bank: EncodeBankMsg,
Custom: NoCustomMsg,
Distribution: EncodeDistributionMsg,
IBC: EncodeIBCMsg(portSource),
IBC2: EncodeIBCv2Msg,
Staking: EncodeStakingMsg,
Any: EncodeAnyMsg(unpacker),
Wasm: EncodeWasmMsg,
Gov: EncodeGovMsg,
}
} where func NoCustomMsg(_ sdk.AccAddress, _ json.RawMessage) ([]sdk.Msg, error) {
return nil, errorsmod.Wrap(types.ErrUnknownMsg, "custom variant not supported")
} So in other words, if a given blockchain developers does not configure otherwise, smart contracts should not return custom types. Some blockchains may use the custom msg by implementing their own encoder. The problem is that Rust's type system cannot prevent mistakes related to mismatch between the chain's custom type and the one defined in the contracts. My proposal is to use impl CosmosMsg {
pub fn new_custom<T: Into<Binary>>(msg: T) -> Self {
Self::Custom(msg.into())
}
} PoC: #2475 |
This approach is reasonable, but it is pretty much what |
I think the benefits that
With the |
We still need to support the custom message feature, but having this as a generic is quite annoying for users because it has to be carried through the whole code.
In our meeting, switching to dynamic dispatch with erased_serde / typetag was proposed.
We should make sure that downstream users are not affected by the fact that this makes
CosmosMsg
no longerSend + Sync
and that the custom message type is not necessarilySized
anymore.This could probably be looked at for something like MultiTest.
The text was updated successfully, but these errors were encountered: