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

Add stableswap to pooltype, and pooltype query to stargate whitelist #3356

Merged
merged 3 commits into from
Nov 14, 2022
Merged

Add stableswap to pooltype, and pooltype query to stargate whitelist #3356

merged 3 commits into from
Nov 14, 2022

Conversation

ValarDragon
Copy link
Member

What is the purpose of the change

Resolves an issue pointed out by @jonator

Brief Changelog

  • Add stableswap support for pooltype query
  • Make pooltype query callable from cosmwasm
  • Fix bug in stableswap msg validate basic, when no scaling factors provided

Testing and Verifying

  • Test has been added

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? N/A

@ValarDragon ValarDragon added V:state/breaking State machine breaking PR A:backport/v13.x backport patches to v13.x branch labels Nov 12, 2022
@ValarDragon ValarDragon requested review from a team and AlpinYukseloglu November 12, 2022 06:12
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:x/gamm Changes, features and bugs related to the gamm module. labels Nov 12, 2022
@jonator
Copy link
Member

jonator commented Nov 12, 2022

@ValarDragon I don't think an individual pool query for type is a good performance move here (prob good to have in general, though). IMO we should include the type, and include nullable scaling factors in the existing pools query and pool model

@ValarDragon
Copy link
Member Author

That definitely doesn't make sense for the general query. Its bad design to have fields that have no sensical interpretation for a given type configuration.

What is instead available right now is interpreting the result of the pool query as a protobuf oneof between Balancer pool and Stableswap pool that can then be type casted. (This is a poor man's version of a sum type)

I'd prefer for the single query, we handle the pool enum complexity for it.

@jonator
Copy link
Member

jonator commented Nov 12, 2022

OK. I didn't know protobuf could handle a union/sum type like that. Yeah then that would be ideal to have a union of each of the types of pools. Then the pools (plural) query could contain a list of that union type.

mattverse
mattverse approved these changes Nov 14, 2022
View reviewed changes
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.

LGTM!

app/apptesting/gamm.go
@@ -81,6 +87,21 @@ func (s *KeeperTestHelper) PrepareBalancerPool() uint64 {
return poolId
}

func (s *KeeperTestHelper) PrepareBasicStableswapPool() uint64 {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@ValarDragon ValarDragon merged commit 66ec9c2 into main Nov 14, 2022
@ValarDragon ValarDragon deleted the dev/add_stableswap_to_pooltype branch November 14, 2022 06:17
@mergify mergify bot mentioned this pull request Nov 14, 2022
Merged
mergify bot pushed a commit that referenced this pull request Nov 14, 2022
@ValarDragon @mergify
Add stableswap to pooltype, and pooltype query to stargate whitelist (#…
e326041 
…3356)

* Add stableswap to pooltype, and pooltype query to stargate whitelist

* Add test / make test scaffolding for stableswap. Fix validate basic bug

* Changelog entry

(cherry picked from commit 66ec9c2)
ValarDragon added a commit that referenced this pull request Nov 14, 2022
@mergify @ValarDragon
Add stableswap to pooltype, and pooltype query to stargate whitelist (#…
e35527b 
…3356) (#3366)

* Add stableswap to pooltype, and pooltype query to stargate whitelist

* Add test / make test scaffolding for stableswap. Fix validate basic bug

* Changelog entry

(cherry picked from commit 66ec9c2)

Co-authored-by: Dev Ojha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattverse mattverse mattverse approved these changes

@AlpinYukseloglu AlpinYukseloglu Awaiting requested review from AlpinYukseloglu AlpinYukseloglu is a code owner

Assignees
No one assigned
Labels
A:backport/v13.x backport patches to v13.x branch C:app-wiring Changes to the app folder C:x/gamm Changes, features and bugs related to the gamm module. V:state/breaking State machine breaking PR
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ValarDragon @jonator @mattverse