[Refactor]: x/auth/tx and x/tx should converge on gogoproto usage.

kocubinski commented 6 months ago

There is duplicate logic in both x/tx/decode and the x/auth/tx packages. Since the decision to support gogoproto throughout the SDK (as specified in core/transaction below) https://github.com/cosmos/cosmos-sdk/blob/1f06f5bec67e76843ceb00502a814b2a87e81d6b/core/transaction/transaction.go#L8

These two packages can be simplified and collapsed into one simple implementation in x/tx. An initial exploration of this was done here in https://github.com/cosmos/cosmos-sdk/pull/20424 and integrated in https://github.com/cosmos/cosmos-sdk/pull/20428.

The result will be a lot of deleted code and much less mental overhead when reasoning about transactions in the SDK. The signing code in x/tx can and should remain the same, as shown in #20424 reflection over gogotypes as input is possible through the use of dynamic messages.

noelukwa commented 6 months ago

@tac0turtle i would like to take on this

tac0turtle commented 6 months ago

yes, we would love the help ❤️

kocubinski commented 6 months ago

@noelukwa Great, #20424 has some preliminary work for it.

JulianToledano commented 3 months ago

hey guys, I’m having a hard time with this. I don’t see an easy way to avoid a dependency on the SDK in x/tx. If we move tx there, there’s a dependency with coin and multisigs bitarray, so those should be defined in x/tx. Furthermore, there are functions defined over types in tx.proto that make use of codec and pubkey interfaces.

I see two options here:

easiest would be to have the dependency on the sdk. Not a good idea as all modules that use x/tx will end up with an indirect dependency to the SDK.
- Move some definitions:
  1. Move Coin and Bitarray to x/tx (which I find pretty werid) or another module. (Does it make sense for types to be a module??)
  2. Use Pubkey defined in cosmos/crypto or move the definition to core
  3. Move codec to core. I think it will be sufficient to move just InterfaceRegistry to core.

There’s also a call to AccAddressFromBech32 that may need an address.Codec to avoid.

@tac0turtle @kocubinski

julienrbrt commented 2 months ago

Is there an RFC coming? Is it still in progress?

JulianToledano commented 2 months ago

Is there an RFC coming? Is it still in progress?

I have this on hold to be honest.

cosmos / cosmos-sdk

[Refactor]: x/auth/tx and x/tx should converge on gogoproto usage. #20431