Skip to content
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

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

Open
kocubinski opened this issue May 20, 2024 · 6 comments
Open

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

kocubinski opened this issue May 20, 2024 · 6 comments
Assignees
Labels
C:x/tx S:needs rfc Needs an RFC to inform community over those changes S:zondax Squad: Zondax

Comments

@kocubinski
Copy link
Member

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)

Msg = gogoproto.Message

These two packages can be simplified and collapsed into one simple implementation in x/tx. An initial exploration of this was done here in #20424 and integrated in #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.

@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label May 20, 2024
@tac0turtle tac0turtle added C:x/tx and removed needs-triage Issue that needs to be triaged labels May 21, 2024
@noelukwa
Copy link
Contributor

@tac0turtle i would like to take on this

@tac0turtle
Copy link
Member

yes, we would love the help ❤️

@kocubinski
Copy link
Member Author

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

@JulianToledano
Copy link
Contributor

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 julienrbrt added the S:needs rfc Needs an RFC to inform community over those changes label Aug 5, 2024
@educlerici-zondax educlerici-zondax added the S:zondax Squad: Zondax label Aug 26, 2024
@julienrbrt
Copy link
Member

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

@JulianToledano
Copy link
Contributor

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

I have this on hold to be honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/tx S:needs rfc Needs an RFC to inform community over those changes S:zondax Squad: Zondax
Projects
Status: 📋 Backlog
Development

No branches or pull requests

6 participants