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

x/params deprecated, but users still need to rely on x/param types #4806

Closed
3 tasks
roy-dydx opened this issue Oct 3, 2023 · 2 comments · Fixed by #4811
Closed
3 tasks

x/params deprecated, but users still need to rely on x/param types #4806

roy-dydx opened this issue Oct 3, 2023 · 2 comments · Fixed by #4811
Assignees
Labels
type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@roy-dydx
Copy link

roy-dydx commented Oct 3, 2023

Summary of Bug

Keeper constructors still take paramtypes.Subspace as a parameter (e.g. https://github.com/cosmos/ibc-go/blob/59e42c6/modules/core/keeper/keeper.go#L43). This requires users to include x/params to pass in an initialized Subspace.

Instead, these constructors should take in an interface like cosmos-sdk modules do (e.g. https://pkg.go.dev/github.com/cosmos/cosmos-sdk@v0.47.5/x/auth#NewAppModule). This allows users to pass in nil and not have to include x/param types.

Expected Behaviour

See above.

Version

59e42c6

Steps to Reproduce

N/A


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega
Copy link
Contributor

@roy-dydx thanks for opening the issue. I believe we can remove the paramtypes.Subspace parameter from keeper constructor functions in the next major release (please correct if I'm wrong, @colin-axner or @damiannolan). Once the migration to self-manager params is run as part of the upgrade to ibc-go v8.0.0, then we can remove the legacy subspace. For that reason, since we are going to remove it probably in the next 6 months, does it cause you a lot of trouble if we just leave it as is? Is the downside of importing x/params reasonable for you for the time being?

What do you think, @colin-axner or @damiannolan?

@damiannolan
Copy link
Member

Doing an expected interface is a fairly low lift. I could make a PR today.

Given that x/params is the same go mod dependency tho it doesn't really make much of a difference, we're not really reducing the dep graph here and as @crodriguezvega says it will be removed in the next release.

I do see it being a code smell tho for new chains to have to bring in legacy imports which are existing for migration purposes only. I guess that is the issue for @roy-dydx? Happy to help clean it up.

tldr; I'll make a PR

@damiannolan damiannolan self-assigned this Oct 4, 2023
@damiannolan damiannolan added the type: code hygiene Clean up code but without changing functionality or interfaces label Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Status: Done 🥳
Development

Successfully merging a pull request may close this issue.

3 participants