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

[stableswap]: Add max pool asset check to ValidateBasic() #2860

Closed
Tracked by #1451
AlpinYukseloglu opened this issue Sep 26, 2022 · 4 comments · Fixed by #2866
Closed
Tracked by #1451

[stableswap]: Add max pool asset check to ValidateBasic() #2860

AlpinYukseloglu opened this issue Sep 26, 2022 · 4 comments · Fixed by #2866
Assignees

Comments

@AlpinYukseloglu
Copy link
Contributor

Background

Even with scaling factors and increased decimal precision, we still need to have a cap on the number of assets we can support in multi-asset pools.

This number should be somewhere between 4 and 8 assets per multi-asset pool and can be enforced by adding checks at pool creation. The simplest check we can add is in ValidateBasic(), although we should enforce this at lower levels as well as a guardrail.

Suggested Design

  • Expand the input asset length check in ValidateBasic() to include a maximum (currently only checks that there are min 2 assets)
  • Add any other relevant checks to CreatePool etc.

Acceptance Criteria

  • All new and existing tests pass
@hieuvubk
Copy link
Contributor

@AlpinYukseloglu As I read in the description, we don't allow pool with 3 assets right?

@AlpinYukseloglu
Copy link
Contributor Author

I don't see why we wouldn't? Anything between 2 and the cap we set is fair game, although I think we should hold off on this until we finish bounding the rest of stableswap inputs and come to a conclusion on a reasonable maxNumAssets

@AlpinYukseloglu AlpinYukseloglu self-assigned this Sep 26, 2022
@hieuvubk
Copy link
Contributor

hieuvubk commented Sep 26, 2022

Alright I got a little misunderstanding here. We don't have a conclusion about the specific cap yet, right?

@AlpinYukseloglu
Copy link
Contributor Author

Yes we haven't finalized it yet! It will come down to a number of factors tied to current open issues/PRs (not a major decision just one I'd prefer not to spend too much time reverting/changing)

@mergify mergify bot closed this as completed in #2866 Sep 27, 2022
mergify bot pushed a commit that referenced this issue Sep 27, 2022
Closes: #2860 
## What is the purpose of the change

> Add max pool asset check to ValidateBasic() 
> Add tests to check the new condition
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants