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

[Feature]: x/Auth and x/tx module common types unification/conversion #17530

Closed
Tracked by #18446
bizk opened this issue Aug 24, 2023 · 5 comments
Closed
Tracked by #18446

[Feature]: x/Auth and x/tx module common types unification/conversion #17530

bizk opened this issue Aug 24, 2023 · 5 comments

Comments

@bizk
Copy link
Contributor

bizk commented Aug 24, 2023

Summary

There are common types in x/auth and x/signing modules that represent the same type of information but with different/old types,

This break interoperability between modules, and makes maintainability difficult.

Problem Definition

Example of SignerData type on both modules. The pubkey should follow crypto module type cryptotypes for public keys. But only one of these do, trying to communicate between modules that use one or antoher require to parse one into another.

x/auth/signing/sign_mode_handler.go

type SignerData struct {
	Address string
	ChainID string
	AccountNumber uint64
	Sequence uint64-
	
	PubKey cryptotypes.PubKey
}

x/tx/signing/signer_data.go

type SignerData struct {
	Address string
	ChainID string
	AccountNumber uint64
	Sequence uint64-
	
	PubKey *anypb.Any
}

Proposed Feature

Let's clean up the modules and agree on maintaining the common type in one place

@bizk bizk assigned bizk and unassigned bizk Aug 24, 2023
@bizk bizk changed the title [Feature]: Auth and tx module common types unification/conversion [Feature]: x/Auth and x/tx module common types unification/conversion Aug 24, 2023
@julienrbrt
Copy link
Member

Was it maybe done for simplifying the dependency graph? cc @kocubinski

@tac0turtle
Copy link
Member

yes, also transaction handling shoulnt be part of auth. The types in auth/tx will be removed in the future they are legacy types right now. We should mark them as such to avoid confusion

@kocubinski
Copy link
Member

that's right, in v0.50 signing is handled in x/tx, so the duplicate type in x/auth is deprecated.

@tac0turtle
Copy link
Member

Should we remove the types in auth since they are still in the release branch?

@github-project-automation github-project-automation bot moved this to 👀 To Do in Cosmos-SDK Nov 17, 2023
@tac0turtle tac0turtle moved this from 👀 To Do to 🧑‍🔧 Needs Design in Cosmos-SDK Nov 17, 2023
@tac0turtle tac0turtle moved this from 🧑‍🔧 Needs Design to ✍ In Progress in Cosmos-SDK Feb 19, 2024
@tac0turtle
Copy link
Member

we merged quite a bit

@github-project-automation github-project-automation bot moved this from 🤸‍♂️ In Progress to 🥳 Done in Cosmos-SDK Apr 22, 2024
@tac0turtle tac0turtle removed this from Cosmos-SDK Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants