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

[CL]: Deprecate duplicated queries from gamm module #3905

Merged
merged 8 commits into from
Jan 6, 2023

Conversation

mattverse
Copy link
Member

Closes: #3883

What is the purpose of the change

This PR deprecates three of the following queries from the gamm module since they have been moved to the swap rotuer module.

  • NumPools
  • EstimateSwapExactAmountIn
  • EstimateSwapExactAmountOut

Protos remained undeleted but marked deprecated using the deprecation option in proto3.

This PR also removes the three queries from the stargate whitelist list.

Brief Changelog

  • Deprecates duplicated queries between the gamm module and the swap router module.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jan 3, 2023
@github-actions github-actions bot added the C:CLI label Jan 3, 2023
@mattverse mattverse added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Jan 3, 2023
@mattverse mattverse changed the title Deprecate duplicated queries from gamm module [CL]: Deprecate duplicated queries from gamm module Jan 3, 2023
@mattverse mattverse requested review from p0mvn and czarcas7ic January 3, 2023 11:06
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once comments are addressed and CI passes

CHANGELOG.md Outdated Show resolved Hide resolved
wasmbinding/stargate_whitelist.go Outdated Show resolved Hide resolved
mattverse and others added 2 commits January 4, 2023 15:14
Co-authored-by: Roman <roman@osmosis.team>
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI passes

tests/e2e/configurer/chain/commands.go Outdated Show resolved Hide resolved
tests/e2e/configurer/chain/commands.go Outdated Show resolved Hide resolved
tests/e2e/configurer/chain/commands.go Outdated Show resolved Hide resolved
Comment on lines 15 to 16
lockuptypes "github.com/osmosis-labs/osmosis/v13/x/lockup/types"
swaprouterqueryproto "github.com/osmosis-labs/osmosis/v13/x/swaprouter/client/queryproto"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lockuptypes "github.com/osmosis-labs/osmosis/v13/x/lockup/types"
swaprouterqueryproto "github.com/osmosis-labs/osmosis/v13/x/swaprouter/client/queryproto"
gammtypes "github.com/osmosis-labs/osmosis/v13/x/gamm/types"
lockuptypes "github.com/osmosis-labs/osmosis/v13/x/lockup/types"

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you still need to remove the three queries from the stargate whitelist

After than and Roman's comments this LGTM

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading on of the other PRs, it looks like we need to keep these on the stargate whitelist. Probably just need to add a linter staticcheck

@mattverse mattverse merged commit ce32907 into main Jan 6, 2023
@mattverse mattverse deleted the mattverse/cl-remove-query-redundancy branch January 6, 2023 08:34
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

refactor(x/gamm): mark queries deprecated
3 participants