refactor(x/gamm, x/swaprouter): multihop and estimate swap move #3764
Conversation
github-actions
bot
added
C:app-wiring
p0mvn
force-pushed
the
roman/swaprouter-multihops
branch
from
7a0d6d0 to
01df0c3
Compare
| @@ -49,6 +30,9 @@ func (k Keeper) swapExactAmountIn( | |||
| if tokenIn.Denom == tokenOutDenom { | |||
| return sdk.Int{}, errors.New("cannot trade same denomination in and out") | |||
| } | |||
| if !pool.IsActive(ctx) { | |||
There was a problem hiding this comment.
TODO: consider moving this to swaprotuer
| // TODO: remove this redundancy after making routes be shared between x/gamm and x/swaprouter. | ||
| swaprouterRoutes := types.ConvertAmountInRoutes(req.Routes) |
There was a problem hiding this comment.
Note: this implies proto-generating routes only in swaprouter and importing them in x/gamm.
According to earlier discussions, this should not cause breakage and will be done in a future PR
There was a problem hiding this comment.
SG if were confident it wont break stuff. Is there an issue or something written to track our testing strategy? (Not PR merge blocking)
There was a problem hiding this comment.
According to our earlier discussions, this should not be state-breaking.
The only concern was breaking telescope. I'm planning to make a follow-up PR and tag Dan Lynch to help with looking into this.
I can also add a test to serialize both old and new routes to make sure that they are valid
| } | ||
|
|
||
| func (k Keeper) MultihopEstimateOutGivenExactAmountIn( | ||
| ctx sdk.Context, | ||
| routes []types.SwapAmountInRoute, | ||
| tokenIn sdk.Coin, | ||
| ) (tokenOutAmount sdk.Int, err error) { | ||
| panic("not implemented") |
There was a problem hiding this comment.
Note: confirmed that for all methods in this file the followin is true:
Copied with minimal changes, the only additional changes are one of:
- new routing logic
- avoid using sdkerrors
| @@ -1,365 +0,0 @@ | |||
| package keeper | |||
There was a problem hiding this comment.
Note: diffed this whole file against x/swaprouter/router.go. The few changes are summarized in x/swaprouter/router.go per method.
| @@ -1,846 +0,0 @@ | |||
| package keeper_test | |||
There was a problem hiding this comment.
Note: confirmed that every test case removed in this file has been ported to x/swaprouter/router_test.go.
The test fixtures might look different due to clean up. However, every test case is still present
| // tokenInDenom string, | ||
| // swapFee sdk.Dec, | ||
| // ) (tokenIn sdk.Coin, err error) | ||
| GetPool(ctx sdk.Context, poolId uint64) (PoolI, error) |
There was a problem hiding this comment.
Note: adding godocs to the rest of these is tracked via: #3556
| } | ||
| } | ||
|
|
||
| // TestCalcInAmtGivenOut only tests that balancer and stableswap pools are type casted correctly while concentratedliquidity pools fail |
There was a problem hiding this comment.
Can we either remove the concentratedliquidity portion from the description, or add a todo to add this to this test as well as the previous one?
There was a problem hiding this comment.
added TODO, including here: #3556
Co-authored-by: Adam Tucker <[email protected]>
| tokenOutAmount = exitCoins.AmountOf(tokenOutDenom) | ||
| for _, coin := range exitCoins { | ||
| if coin.Denom == tokenOutDenom { | ||
| continue | ||
| } | ||
| swapOut, err := k.SwapExactAmountIn(ctx, sender, poolId, coin, tokenOutDenom, sdk.ZeroInt()) | ||
| swapOut, err := k.SwapExactAmountIn(ctx, sender, pool, coin, tokenOutDenom, sdk.ZeroInt(), swapFee) |
There was a problem hiding this comment.
Wait, theres been an important abstraction issue thats changed here wrt swap fee.
I get why its being done here, but am concerned about:
- Whats the public/private guarantee
- Ensuring we keep dynamic swap fees possible.
The latter is still the case, if AMMs can "ignore" the swapFee arg.
Maybe we add a panic for now, if swapFee arg less than .5 ctx.SwapFee() ?
I think its good to merge if we add that safety check, and add to our mental checklist, that modules shouldn't import gamm, only swaprouter.
| @@ -130,7 +105,12 @@ func (k Keeper) swapExactAmountOut( | |||
| "can't get more tokens out than there are tokens in the pool") | |||
| } | |||
|
|
|||
| tokenIn, err := pool.SwapInAmtGivenOut(ctx, sdk.Coins{tokenOut}, tokenInDenom, swapFee) | |||
| cfmmPool, err := convertToCFMMPool(pool) | |||
There was a problem hiding this comment.
nbd, but why's this a function instead of just a typecast?
There was a problem hiding this comment.
This type cast happens frequently so made sense to abstract it away
Closes: #XXX
What is the purpose of the change
This PR moves multihop and estimate swaps keeper methods from x/gamm to x/swaprouter.
No messages or queries are broken.
Old messages and queries relying on the x/gamm keeper methods now directly call the respective methods in
x/swaprouter.Testing and Verifying
All pre-existing tests have been moved to
x/swaprouter. New tests added where applicable.Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no