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

Move all tx stuff into x/tx go.mod #14600

Closed
4 tasks
amaury1093 opened this issue Jan 12, 2023 · 10 comments
Closed
4 tasks

Move all tx stuff into x/tx go.mod #14600

amaury1093 opened this issue Jan 12, 2023 · 10 comments
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 12, 2023

Summary

Move all tx stuff into x/tx go.mod

Problem Definition

We currently have tx-related logic and types inside:

This is confusing.

Proposal

During our last SDK call, we decided to concentrate all these above into a new x/tx go mod.

@amaury1093 amaury1093 added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jan 12, 2023
@amaury1093 amaury1093 self-assigned this Jan 12, 2023
@amaury1093
Copy link
Contributor Author

amaury1093 commented Jan 12, 2023

One problem I envision is: a lot of tx stuff depends on cosmos/cosmos-sdk/types, e.g. sdk.Context. Do we want to:

  1. put everything in x/tx already now, but this new go module will depend on the root sdk
  2. cherry-pick stuff into x/tx as we go, so that it doesn't depend on the root sdk, and keep this issue as a long-term goal as we clean the repo up.

Maybe 2 is a cleaner approach.

@alexanderbez
Copy link
Contributor

We did have consensus on x/tx, right?

@julienrbrt
Copy link
Member

julienrbrt commented Jan 13, 2023

A small point about the docs: they will need to be moved as well and possibly improved to explain the package as a whole (that will do much more):

Another possible thing to think about is the package name. Given that we go for the whole path for packages living under x/ (as of x/nft and x/circuit), we may want to do the same for tx for consistency (instead of using cosmossdk.io)

@github-project-automation github-project-automation bot moved this to 📝 Todo in Cosmos-SDK Jan 16, 2023
@tac0turtle tac0turtle moved this from 📝 Todo to 💪 In Progress in Cosmos-SDK Jan 16, 2023
@amaury1093
Copy link
Contributor Author

amaury1093 commented Jan 19, 2023

So I took at look at what can be put into x/tx today... not much:

In conclusion, this issue is probably an clean-up Epic in itself.

The alternative is to do 1 in #14600 (comment), i.e. have a cyclic go mod dependency between root sdk and x/tx, like we do for groups.

@alexanderbez
Copy link
Contributor

#10368 will allow us to cross off many of the sub-points in the first point, right?

@amaury1093
Copy link
Contributor Author

#10368 will allow us to cross off many of the sub-points in the first point, right?

Yeah I think so. But did we decide on a solution for #10368? Also, I'm not 100% sure if it's dependent on ADR-054 or if they are orthogonal.

@amaury1093 amaury1093 moved this from 💪 In Progress to 📝 Todo in Cosmos-SDK Jan 25, 2023
@aaronc
Copy link
Member

aaronc commented Feb 27, 2023

I would modify the original proposal slightly to propose that client related tx stuff (that depends on x/tx) goes in client/v2/tx to separate state machine and client stuff a bit more.

@tac0turtle
Copy link
Member

lots of this was done by @kocubinski, can we close this isse?

@aaronc
Copy link
Member

aaronc commented May 10, 2023

We still don't have ante handlers or client support migrated

@tac0turtle
Copy link
Member

now i believe this is closed

@github-project-automation github-project-automation bot moved this from 📝 Todo to 👏 Done in Cosmos-SDK May 31, 2023
@tac0turtle tac0turtle removed this from Cosmos-SDK Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

No branches or pull requests

5 participants