-
Notifications
You must be signed in to change notification settings - Fork 607
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
feat: implement pool liquidity query in pool manager, deprecate the one in gamm #4951
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once test cases are ported over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Please restore x/gamm CLI and Matt's comment about tests
@p0mvn @mattverse imported tests and readded cli! |
x/poolmanager/router_test.go
Outdated
// Create default CL pool | ||
pool := s.PrepareConcentratedPool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test with non-cl pool e.g. balancer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added default balancer asset check. let me know if that suffice
x/poolmanager/router_test.go
Outdated
name: "valid with 2 coins", | ||
poolId: 1, | ||
poolLiquidity: defaultCoins, | ||
expectedResult: defaultCoins, | ||
}, | ||
{ | ||
name: "valid with 1 coin", | ||
poolId: 1, | ||
poolLiquidity: sdk.NewCoins(defaultPoolCoinTwo), | ||
expectedResult: sdk.NewCoins(defaultPoolCoinTwo), | ||
}, | ||
{ | ||
// can only happen if someone sends extra tokens to pool | ||
// address. Should not occur in practice. | ||
name: "valid with 3 coins", | ||
poolId: 1, | ||
poolLiquidity: sdk.NewCoins(defaultPoolCoinTwo, defaultPoolCoinOne, nonPoolCool), | ||
expectedResult: defaultCoins, | ||
}, | ||
{ | ||
// this can happen if someone sends random dust to pool address. | ||
name: "only non-pool coin - does not show up in result", | ||
poolId: 1, | ||
poolLiquidity: sdk.NewCoins(nonPoolCool), | ||
expectedResult: sdk.Coins(nil), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these tests are only relevant in the context of concentrated liquidity (looks like this is where they were copied from).
What makes sense to be tested here:
- can route to CL module and get liquidity
- can route to gamm module and get liquidity
- some error cases specific to poolmanager
Please refactor the tests to be relevant for this level of abstraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
e678d3d
to
df04795
Compare
aa3766d
to
23f4952
Compare
Merging since all earlier comments have been addressed |
Closes: #4802
What is the purpose of the change
Implement total pool liquidity for frontends in pool manager. Deprecate the one in gamm and make it route through pool manager.
Brief Changelog
Testing and Verifying
tested using localosmosis with state
output;
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? (yes / no)x/<module>/spec/
) / Osmosis docs repo / not documented)