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/gamm: Add check for correct input asset denoms to CalcJoinPoolShares #1931

Merged
merged 6 commits into from
Jul 5, 2022

Conversation

AlpinYukseloglu
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu commented Jul 1, 2022

Closes: #1906

What is the purpose of the change

Our current implementation of CalcJoinPoolShares checks to make sure the number of denoms sent in for an all-asset join is correct, but has no check for whether those assets are actually the correct denoms. This makes it so that entirely invalid inputs (i.e. correct number of input tokens but incorrect denoms) successfully make it through the entire set of all-asset join calculations until they are incidentally caught in the single-asset join section due to a check in updateIntermediaryPoolAssetsLiquidity.

} else if tokensIn.Len() != p.NumAssets() {
return sdk.ZeroInt(), sdk.NewCoins(), errors.New("balancer pool only supports LP'ing with one asset or all assets in pool")
}

This PR adds an additional check to the input tokens that makes sure they are of the correct denoms so that the function never assumes invalid inputs are correct for calculating all-asset joins.

Brief Changelog

  • Adds a check similar to DenomsSubsetOf that makes sure that the input denoms are part of the pool being joined (Note: we cannot use DenomsSubsetOf here because we have a denom->PoolAsset map instead of a set of sdk.Coins to represent pool liquidity)

Testing and Verifying

This change is already covered by existing GAMM tests.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
  • How is the feature or change documented? (not applicable)

@AlpinYukseloglu AlpinYukseloglu added T:bug 🐛 Something isn't working C:x/gamm Changes, features and bugs related to the gamm module. labels Jul 1, 2022
@AlpinYukseloglu AlpinYukseloglu requested review from a team, ValarDragon and p0mvn July 1, 2022 02:27
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

I do think that the existing code base does have checks within the methods of CalcJoinPoolShares already, but Im generally agreeing on this extra explicit sanity check!

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM

  1. Do we have a test case already testing this path? If not, we should definitely add one.
  2. We should get a changelog entry in.

@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Jul 1, 2022
@AlpinYukseloglu
Copy link
Contributor Author

LGTM

  1. Do we have a test case already testing this path? If not, we should definitely add one.

I believe this test should cover it but happy to add another one with more assets/mixed assets if you think it would be useful:

name: "newLiquidity has a coin that poolAssets don't",

  1. We should get a changelog entry in.

Added in recent commit!

@p0mvn
Copy link
Member

p0mvn commented Jul 4, 2022

LGTM

  1. Do we have a test case already testing this path? If not, we should definitely add one.

I believe this test should cover it but happy to add another one with more assets/mixed assets if you think it would be useful:

name: "newLiquidity has a coin that poolAssets don't",

  1. We should get a changelog entry in.

Added in recent commit!

Just making sure we don't miss anything - was that commit pushed? Not seeing a new test

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

@AlpinYukseloglu
Copy link
Contributor Author

Just making sure we don't miss anything - was that commit pushed? Not seeing a new test

This test was already there so it's not a part of this PR. The link I included was directly from main

@alexanderbez alexanderbez merged commit 49d632f into main Jul 5, 2022
@alexanderbez alexanderbez deleted the input-check-calcjoinpoolshares branch July 5, 2022 18:07
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid T:bug 🐛 Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

x/gamm: Add check for correct input asset denoms to CalcJoinPoolShares
4 participants