Add TxEncoder to client TxBuilder

[arturalbov]

Summary

Hello,
In BaseApp we have possibility to set transaction decoder in order to convert bytes to StdTx.

...
txDecoder   sdk.TxDecoder        // unmarshal []byte into sdk.Tx
...

On the other hand in client we're using TxBuilder that have strict logic of tx marshaling:

bldr.Codec.MarshalBinaryLengthPrefixed(auth.NewStdTx(msg.Msgs, msg.Fee, []auth.StdSignature{sig}, msg.Memo))

Problem Definition

The problem is when I need some custom decoder on the node side I have to extend TxBuilder or rewrite some of its methods. That results in a lot of redundant code.

Proposal

I propose to add field named TxEncoder into TxBuilder of type type TxDecoder func(tx Tx) ([]byte, Error) which will called everywhere we need to get transaction bytes. And move old behavior to "default" encoder.

I could make a PR for you.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

Im confused as to why a TxBuilder needs to "unbuild/unpack" a tx? Its sole intended purpose was to be used for easily building txs (including serializing them) which later get broadcasted via client/utils.

[arturalbov]

@alexanderbez TxBuilder don't need to "unbuild/unpack" tx. My issue is about custom encoding/serializing tx in TxBuilder.
If I, for example, using some custom TxDecoder on node side, then your CLI that uses TxBuilder with strictly defined serialization wouldn't work with it.

[alexanderbez]

I see. Yes, in that case I think we may be able to remove the codec entirely and provide a TxEncoder function where the default is: cdc.MarshalBinaryLengthPrefixed(stdTx auth.StdTx)

[arturalbov]

Cool, I can implement this and open a PR.

[arturalbov]

@alexanderbez I've got a proposal. What if add TxEncode to global config (types.Config) or somehow make it globally available for client commands?
Another way is to pass TxEncoder in commands creation functions. For example, func GetCmdEditValidator(cdc *codec.Codec) *cobra.Command will look like func GetCmdEditValidator(cdc *codec.Codec, txEncoder TxEncoder) *cobra.Command
I'll explain why I need it:
After I've started work on PR I realized that adding TxEncoder to TxBuilder sadly doesn't solve my problem. There are a lot of useful CLI commands from cosmos-sdk ( stake, gov, dist, slashing modules) that generating transactions. TxBuilder initialized there and so I it'll be impossible to use custom TxEncoder in this commands.

[alexanderbez]

I think we'll have to with something like option (2) you mentioned. Reason being is maybe we want different types of encoders...I can't imagine such a case, but still. (2) is also more flexible.

[arturalbov]

So pass TxEncoder to every GetCmd* method?

[alexanderbez]

@arturalbov let me go through the CLI flows and see what solutions we can come up with.

[alexanderbez]

Ok, so I think the path of least resistance here and also somewhat clean, is the following:

• TxBuilder has no need for a codec directly. It should have a WithTxEncoder method instead that sets the tx encoder.
• Remove WithCodec on the TxBuilder.
• Update all CLI and REST implementations that use a TxBuilder to call WithTxEncoder passing in a decoder set in the Config as you suggested. However, if no such config is defined, it should fall back to the default (the way it currently works -- just put that into a function).
  • Ultimately, this will be a function that you can call: GetTxEncoder which checks if the config was set. If so, return that, otherwise, return the default.

How does this sound?

[arturalbov]

Sounds great! I'll implement this