feat(stableswap): implement and test JoinPoolNoSwap and CalcJoinPoolNoSwapShares abstractions #2916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
C:x/gamm
p0mvn
added
the
V:state/compatible/no_backport
p0mvn
commented
Oct 1, 2022
p0mvn
requested review from
a team,
AlpinYukseloglu,
ValarDragon and
alexanderbez
ValarDragon
reviewed
Oct 4, 2022
| @@ -52,4 +81,6 @@ var ( | |||
| ErrInvalidStableswapScalingFactors = sdkerrors.Register(ModuleName, 62, "length between liquidity and scaling factors mismatch") | |||
| ErrNotScalingFactorGovernor = sdkerrors.Register(ModuleName, 63, "not scaling factor governor") | |||
| ErrInvalidScalingFactors = sdkerrors.Register(ModuleName, 64, "invalid scaling factor") | |||
|
|
|||
| ErrStableSwapNoSwapJoinNeedsMultiAssetsIn = errors.New("no swap join needs multiple assets in, one was given") | |||
There was a problem hiding this comment.
Thanks for not making it an sdkerrors!
ValarDragon
reviewed
Oct 4, 2022
| // N.B. we simulate the calculation by attempting to join a copy of the original pool. | ||
| // The original pool is not modified. | ||
| pCopy := p.Copy() | ||
| numShares, tokensJoined, err = pCopy.joinPoolNoSwapSharesInternal(ctx, tokensIn, swapFee) |
There was a problem hiding this comment.
This isn't right, because JP no swap attempts to swap when all assets are given in non-perfect ratios.
Should use cfmm_common.MaximalExactRatioJoin(p, ctx, tokensIn)
There was a problem hiding this comment.
This is joinPoolNoSwapSharesInternal though that doesn't do that. I implemented a separate method, similar to how we do this in balancer
There was a problem hiding this comment.
osmosis/x/gamm/pool-models/stableswap/amm.go
Line 328 in 99dcf1c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #2376
What is the purpose of the change
Implements
JoinPoolNoSwapandCalcJoinPoolNoSwapSharesBrief Changelog
Test_StableSwap_CalculateAmountOutAndIn_InverseRelationshipwasn't run due toStableSwapTestSuitenot started. Upon properly enabling it, realized that it was breaking. Skipped it for now. To be fixed later.joinPoolSharesInternaljoinPoolSharesInternalto utilize the no swap methodCalcJoinPoolNoSwapSharesJoinPoolto utilizeCalcJoinPoolNoSwapSharesinstead of joining directly becauseCalcJoinPoolNoSwapShareshas extra validation after performing the calculationTesting and Verifying
This change added tests and can be verified as follows:
Run:
TestJoinPoolNoSwapSharesTestCalcJoinPoolNoSwapSharesTestJoinPoolNoSwapSharesInternalDocumentation and Release Note
Unreleasedsection inCHANGELOG.md? no