[Epic]: Client v2

[tac0turtle]

Summary

Within the Cosmos SDK we have a large client package. This package is useful for commands and for interacting with state machine from Go. The issue has become that there is no separation between state machine and client code. This has caused issues when users try to import the client package, they get the whole sdk with it too, and client has bled into the state machine. This is evident with things like client.Context being using in the server package because it was available.

This Epic is focused on the package client/v2. We have started writing autocli in this package for it to work it without importing the Cosmos SDK it has needed to reimplement things like keyring and other parts that exist in the Cosmos SDK.

This issue will be long lived and updated multiple times in order to best identify the path forward.

Problem Definition

Client code is being in non client code paths. We should separate client to its own package (client/v2) and move all things needed to create a client or interact with the state machine into here. This will allow teams to have a dependable location for all things client related.

Work Breakdown

Client/v2 should only focus on transaction building, broadcasting and decoding.
All the other packages present in client should not be added to client/v2.
The main goal is to remove the usage of client/tx from client/v2

• Introduce tx factory
• Create custom tx builder (not depending on the SDK)
  Extract client/tx to client/v2/internal/tx. Note this is not a simple package moving, we should re-create a notion of a simple tx factory and have a tx encoder and decoder.
• Abstract from client.Context to remove dependency on client (pass directly address codecs and interface registry)

[raynaudoe]

Regarding the TxBuilder interface, I find that a minimal interface for the client/v2 that also has underlying support for protobufV2 would be something like this (quite similar to the v1 TBH):

client/v2/tx/builder.go:

type TxBuilder interface {
	GetTx() TxV2

	SetMsgs(msgs ...sdk.MsgV2) error
	SetSignatures(signatures ...signingtypes.SignatureV2) error
	SetMemo(memo string)
	SetFeeAmount(amount sdk.Coins)
	SetFeePayer(feePayer sdk.AccAddress)
	SetGasLimit(limit uint64)
	SetTimeoutHeight(height uint64)
	SetFeeGranter(feeGranter sdk.AccAddress)
	SetAuxSignerData(tx.AuxSignerData) error
}

client/v2/tx/types.go:

type TxV2 interface {
	SigVerifiableTxV2

	types.TxV2WithMemo
	types.TxV2WithFee
	types.TxV2WithTimeoutHeight
	types.HasValidateBasic
}

type SigVerifiableTxV2 interface {
	types.TxV2
	GetSigners() ([][]byte, error)
	GetPubKeys() ([]cryptotypes.PubKey, error)
	GetSignaturesV2() ([]signing.SignatureV2, error)
}

type TxV2 interface {
	GetMsgs() ([]protov2.Message, error)
}

I'm not sure about the requirement of making the builder "not depending on the SDK", I guess importing sdk types is not avoidable IMO

[testinginprod]

I'm not sure about the requirement of making the builder "not depending on the SDK", I guess importing sdk types is not avoidable IMO

this should be avoided fully, we have an API folder which is a separately versioned go.mod that contains only protov2 types.

Regarding the API this is meant only to serve signing purposes, it does not need all the abstractions that we have had in the sdk with respect to the TX, eg all the SigVerifiable, WithMemo interfaces should disappear and the builder should only return the concrete tx type, or the signed version of it.

I think I good abstraction is a builder which sets msgs, fee payer, unordered, etc all the possible combos (and provide defaults). Then it accepts a generic Authenticator that returns the signed TX.

This is a good starting point imho 👍

[raynaudoe]

So regarding

Abstract from client.Context to remove dependency on client (pass directly address codecs and interface registry)

How critical is this since the client.Context is in the same package as Factory ?
I can abstract out most of the fields that the Factory constructor extracts from the context but it feels unnecessary.

However the context struct can be improved to avoid having redundant fields or having them scattered, like everything that is related to "parameters" or "configuration" can be moved into a new idea of TxConfig interface that is something like:

(also, SignMode I think it could be embedded inside signing.Context thus avoiding having "signing configurations" divided in different structs)

type TxConfig interface {
	GetTxParams() TxParameters
	GetSignConfig() TxSigningConfig
}

type TxSigningConfig interface {
	SignModeHandler() *txsigning.HandlerMap
	SigningContext() *txsigning.Context
	MarshalSignatureJSON([]signingtypes.SignatureV2) ([]byte, error) // replace SignatureV2 type with API 
	UnmarshalSignatureJSON([]byte) ([]signingtypes.SignatureV2, error)  // replace SignatureV2 type with API 
}

type TxParameters struct {
	timeoutHeight uint64
	chainID       string
	memo          string

	AccountConfig
	SigningConfig
	GasConfig
	FeeConfig
	ExecutionOptions
	ExtensionOptions
}  

a single interface to access all parameters and configs sounds easier for the Factory to use them.

Thoughts?

[raynaudoe]

Now regarding messages structs and ifaces in tx_msg:
for being compliant with

this should be avoided fully, we have an API folder which is a separately versioned go.mod that contains only protov2 types.

I'm thinking of replacing the type types.Msg (which is an alias for protov1 msgs) directly with anypb.Any , so the new TxBuilder interface will be something like:

type TxBuilder interface {
	GetTx() txv1beta1.Tx
	Sign() error
	SetMsgs(msgs ...*anypb.Any) error
	SetMemo(memo string)
	SetFeeAmount(amount txv1beta1.Fee)
	SetFeePayer(feePayer string)
	SetGasLimit(limit uint64)
	SetTimeoutHeight(height uint64)
	SetFeeGranter(feeGranter string)
	SetUnordered(v bool)
	SetAuxSignerData(data txv1beta1.AuxSignerData) error
}

type TxBuilderProvider interface {
	NewTxBuilder() TxBuilder
	WrapTxBuilder(txv1beta1.Tx) (TxBuilder, error)
}

so there will be no usage of /types/tx inside the client pkg

[raynaudoe]

Hey @testinginprod , regarding this comment and the usage of only protov2 in the client:

this should be avoided fully, we have an API folder which is a separately versioned go.mod that contains only protov2 types.

What are your thoughts about adding to the client only protov2 compatibility (given the impact that that might have on users)?

I'm thinking an alternative, something that would keep client/v2 decoupled from sdk.types but keeping the flexibility of using protov1 or protov2 in the modules. The idea is to do something like:

type MsgAdapter interface {
	MsgExternalToInternal(any) *codectypes.Any
	MsgInternalToExternal(*codectypes.Any) any
}

type FeeAdapter interface {
	FeeExternalToInternal(any) txv1beta1.Fee
	FeeInternalToExternal(txv1beta1.Fee) any
}

type TxAdapter interface {
	MsgAdapter
	FeeAdapter
}

maybe it can be done simpler with generics, but the idea is to decouple types from the client module in this case.
Since these interfaces will be placed outside the client module, client/v2 will only know protov2 types and the modules are the responsible of implementing the TxAdapter iface to make use of TxBuilder/TxFactory
What do you think?