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.

Sign up for GitHub

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

Jump to bottom

x/gamm: Add check to ensure JoinPoolNoSwap cannot be called with arbitrary denoms #1930

Merged
merged 6 commits into from
Jul 1, 2022
Merged

x/gamm: Add check to ensure JoinPoolNoSwap cannot be called with arbitrary denoms #1930

merged 6 commits into from
Jul 1, 2022

Conversation

AlpinYukseloglu
Copy link
Contributor

Closes: #1929

What is the purpose of the change

Our current implementation of JoinPoolNoSwap only checks for whether the input set of tokens includes the tokens in the pool, but does not have any check that prevents someone from putting in an arbitrary number of arbitrary tokens. Since this is a no-swap/all-asset join, there are no other immediate checks so an invalid input can make it directly to the state-changing JoinPool function.

Brief Changelog

  • Add && tokenInMaxs.DenomSubsetOf(neededLpLiquidity) to x/gamm/keeper/pool_service.go/L203 so that input tokens for a no-swap pool join don’t include denoms that aren’t in the pool:

Testing and Verifying

This change is already covered by existing tests in our GAMM module

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 ValarDragon and a team July 1, 2022 01:02
p0mvn
p0mvn reviewed Jul 1, 2022
View reviewed changes
x/gamm/keeper/pool_service.go Outdated Show resolved Hide resolved
x/gamm/keeper/pool_service.go Outdated Show resolved Hide resolved
@ValarDragon
Copy link
Member

LGTM once a test case is added

@AlpinYukseloglu
Copy link
Contributor Author

LGTM once a test case is added

Done!

@ValarDragon
Copy link
Member

Awesome! Can you add a changelog entry under bug fix, and then merge this?

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

AlpinYukseloglu commented Jul 1, 2022 •
edited
Loading

Awesome! Can you add a changelog entry under bug fix, and then merge this?

Ready to merge! (not authorized to merge myself)

@ValarDragon
Copy link
Member

Err can you fix your markdown editor locally, to not overwrite all the bulletpoints 😅

I'll commit to this branch to get this changelog change through

@ValarDragon ValarDragon merged commit 01dee57 into main Jul 1, 2022
@ValarDragon ValarDragon deleted the no-swap-denom-check branch July 1, 2022 03:57
@github-actions github-actions bot mentioned this pull request May 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0mvn p0mvn p0mvn approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees
No one assigned
Labels
C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. T:bug 🐛 Something isn't working
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

x/gamm: Add check to ensure JoinPoolNoSwap cannot be called with arbitrary denoms
3 participants
@AlpinYukseloglu @ValarDragon @p0mvn