Transaction Client (CLI & REST) Protobuf Migration

[alexanderbez]

Summary

1. For each module, migrate the existing client transaction CLI and REST handlers to be Protobuf compatible.

• x/bank
• x/crisis
• x/distribution
• x/gov
• x/params
• x/slashing
• x/staking
• x/upgrade
• x/ibc (@fedekunze)

2. Remove cruft/dead/duplicate code/files from x/auth. We should remove all business logic from this module.

• x/auth/types/txbuilder.go
• x/auth/client/tx.go
• x/auth/types/account_retriever.go
• x/auth/client/rest.go

3. Remove GetTxCmd (and GetQueryCmd?) from AppModuleBasic. Because certain modules have messages that are application-level defined, they'll need custom arguments. The module manager can be redesigned to accommodate this, but it is not a hard requirement for completeness. Rather, the application can just call each module's root handlers for now.

• Finish PR Tx CLI proto module interface #5989 - new tx module interface

• implement new NewTxCmd module interface
• make amino StdTx implement ClientTx so that we have a single set of tx commands
• wire up one module in simcli to use the new module interface (x/bank)

• wire up all the other modules in simcli to use the new module interface from (Tx CLI proto module interface #5989) and pass integration tests

4. Setup simcli to use the new proto transaction format (after Proto Any Tx migration #6213 is far enough along). Make sure integration tests work for both proto and amino (Full amino encoding support #6190). Also being taken care of in part by (Implement SIGN_MODE_DIRECT #6216)

ref: #5663

/cc @jackzampolin @aaronc @marbar3778 @alessio

---

For Admin Use

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

[aaronc]

Regarding moving tx related stuff out of x/auth, should we move StdFee, StdSignature, etc. somewhere else?

[alexanderbez]

Yeah I think that makes sense 👍

[aaronc]

Here's my current thinking on how to address tx CLI commands:

1. make tx.AccountRetriever and tx.Generator fields on CLIContext

Reading the doc string for it says "a typical CLI context created in SDK modules for transaction handling and queries". It seems natural that they would fit there. Before tx stuff depended on x/auth but now that this has been refactored out (#5992), tx.AccountRetriever and tx.Generator (or even all of client/tx) can be merged into client/context

2. have CLIContext as the single parameter for the new GetTxCmd and GetQueryCmd on modules, just like RegisterRESTRoutes. This will also handle passing in codec.Marshaler

3. migrate NewCLIContextWithInputAndFrom, etc. to something like CLIContext.InitFromContextAndFrom(*cobra.Cmd)

I've thought about this a bit and I would argue that passing in a CLIContext that can be pre-populated with the required dependencies higher up is the "correct" way to approach something like this.

[alexanderbez]

1. make tx.AccountRetriever and tx.Generator fields on CLIContext

Sure. They're part of the TxFactory now, but I see no reason why we can't move them to the CLIContext type.

... tx.AccountRetriever and tx.Generator (or even all of client/tx) can be merged into client/context

I would advise against this. Just keep the client/tx package as-is.

2. have CLIContext as the single parameter for the new GetTxCmd and GetQueryCmd on modules, just like RegisterRESTRoutes. This will also handle passing in codec.Marshaler

👍

3. migrate NewCLIContextWithInputAndFrom, etc. to something like CLIContext.InitFromContextAndFrom(*cobra.Cmd)

What does this really buy you? It saves you from writing one line and now this depends on Cobra.

[aaronc]

1. make tx.AccountRetriever and tx.Generator fields on CLIContext

Sure. They're part of the TxFactory now, but I see no reason why we can't move them to the CLIContext type.

... tx.AccountRetriever and tx.Generator (or even all of client/tx) can be merged into client/context

I would advise against this. Just keep the client/tx package as-is.

Well in order for them to exist on CLIContext they would need to exist in client/context because client/tx depends on client/context. But if you'd rather the business logic stay in client/tx, that's fine. Either way, what would your rationale be? I think it's important that we organize packages based on some principle of where things are "supposed" to be

3. migrate NewCLIContextWithInputAndFrom, etc. to something like CLIContext.InitFromContextAndFrom(*cobra.Cmd)

What does this really buy you? It saves you from writing one line and now this depends on Cobra.

Fine it could be CLIContext.InitFromInput(io.Reader) if it's important not to depend on cobra. I would argue, however, that the more repetitive boilerplate that is removed from any of these client methods reduces brittleness and potential need for refactoring for both the sdk and its users.

[alexanderbez]

Either way, what would your rationale be?

Because it has nothing to do with a context.

Fine it could be CLIContext.InitFromInput(io.Reader) if it's important not to depend on cobra.

👍

[fedekunze]

I'm going to work on the IBC module tx migration

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[aaronc]

Marking this as done. Any remaining work is happening in #6213.