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

feat: cosmwasmpool core logic and tests #5354

Merged
merged 7 commits into from
May 31, 2023
Merged

feat: cosmwasmpool core logic and tests #5354

merged 7 commits into from
May 31, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented May 30, 2023

Closes: #XXX

What is the purpose of the change

This is the core logic and full integration of x/cosmwasmpool into the rest of the system.

It is tested with the version of transmuter contract as of main branch here: https://github.com/osmosis-labs/transmuter

Spec: https://github.com/osmosis-labs/osmosis/tree/main/x/cosmwasmpool

The remaining steps are:

Testing and Verifying

This change added tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@p0mvn p0mvn added the V:state/breaking State machine breaking PR label May 30, 2023
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:x/poolmanager labels May 30, 2023
@p0mvn p0mvn requested review from nicolaslara and iboss-ptk May 30, 2023 04:03
@p0mvn p0mvn marked this pull request as ready for review May 30, 2023 04:04
// represents a pool that was stored in the keeper.
// - An error if unmarshalling fails for any of the values fetched from the store.
// In this case, the slice of PoolI interfaces will be nil.
func (k Keeper) GetPools(ctx sdk.Context) ([]poolmanagertypes.PoolI, error) {
Copy link
Contributor

@nicolaslara nicolaslara May 31, 2023

Choose a reason for hiding this comment

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

who can call this? I'm concerned about performance as this is a linear operation. Right now this is probably limited by the whitelist, but if we open it up it may be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is currently exposed as a query to frontend. For every pool module, we have a similar query. x/poolmanager exposes an AllPools query that gets pools from every module. This is not an issue.

However, we should avoid exposing this to stargate whitelist

Copy link
Contributor

@nicolaslara nicolaslara left a comment

Choose a reason for hiding this comment

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

Added a few nits, but other than that, this looks good to me

@github-actions github-actions bot added C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. labels May 31, 2023
@p0mvn
Copy link
Member Author

p0mvn commented May 31, 2023

Addressed all comments. Going to merge once CI passes. Thanks @nicolaslara

@p0mvn p0mvn merged commit 4206fd9 into main May 31, 2023
@p0mvn p0mvn deleted the roman/cw-pools-core branch May 31, 2023 20:02
pysel pushed a commit that referenced this pull request Jun 6, 2023
* feat: cosmwasmpool core logic and tests

* changelog

* updates

* updates

* rename cast helpers

* rename

* remove error check
@github-actions github-actions bot mentioned this pull request Feb 1, 2024
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants