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(ProtoRev): Support for CW Pools #5901

Merged
merged 10 commits into from
Jul 28, 2023
Merged

Conversation

davidterpay
Copy link
Contributor

What is the purpose of the change

This PR adds support for CW pools, primarily by adding it as a recognized type when calculating the amount of pool points a route consumes.

Related issue: #5858

Testing and Verifying

  • added unit tests for calculating routes with CW pools

@davidterpay davidterpay requested a review from p0mvn as a code owner July 27, 2023 22:28
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 27, 2023
@davidterpay davidterpay added protorev All things related to x/protorev V:state/breaking State machine breaking PR and removed C:app-wiring Changes to the app folder labels Jul 27, 2023
@davidterpay davidterpay requested a review from NotJeremyLiu July 27, 2023 22:30
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 27, 2023
@davidterpay
Copy link
Contributor Author

@NotJeremyLiu @p0mvn @czarcas7ic added some tasks to #5858 for better CW testing.

@davidterpay davidterpay requested a review from czarcas7ic July 27, 2023 22:45
@@ -20,6 +20,10 @@ var (
StableWeight: 5, // it takes around 5 ms to simulate and execute a stable swap
BalancerWeight: 2, // it takes around 2 ms to simulate and execute a balancer swap
ConcentratedWeight: 2, // it takes around 2 ms to simulate and execute a concentrated swap
Copy link
Contributor

Choose a reason for hiding this comment

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

Since in upgrade we are resetting pool weights to the defaults, can we match the defaults to what's on mainnet today? (BalancerWeight is 1, ConcentratedWeight is 300 -- essentially turning it off)

Copy link
Member

Choose a reason for hiding this comment

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

The CL issue should go away in the upgrade right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! There is an outstanding task (which we will open a PR for in the next few days) that will bound protorev search better for CL pools and allow it to work on mainnet.

Copy link
Contributor Author

@davidterpay davidterpay Jul 28, 2023

Choose a reason for hiding this comment

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

I'll update the pool weights in the update PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ValarDragon Yeah the issue will go away, but I'd like to change the pool points in the PR that makes the issue go away rather than in this PR

Copy link
Contributor

@NotJeremyLiu NotJeremyLiu left a comment

Choose a reason for hiding this comment

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

lgtm, left some small nits

Comment on lines 90 to 91
// The weight of a cosmwasm pool
uint64 cosmwasm_weight = 4 [ (gogoproto.moretags) = "yaml:\"cw_weight\"" ];
Copy link
Member

@ValarDragon ValarDragon Jul 28, 2023

Choose a reason for hiding this comment

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

We may need to make this dynamic based on address in the future?

I guess gas metering may also handle this for us?
(I'm in support of merging this, and discussing in an issue / spec doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea totally agree. Ill add some tasks to the ProtoRev v17 issue tracker.

@ValarDragon
Copy link
Member

Awesome, thanks!

@ValarDragon ValarDragon merged commit 91e56dd into main Jul 28, 2023
@ValarDragon ValarDragon deleted the terpay/support-cw-pools branch July 28, 2023 18:10
VitalyV1337 pushed a commit to VitalyV1337/osmosis-1 that referenced this pull request Jul 31, 2023
* init

* add new pool weights to upgrade handler

* change log

* add todo for genesis testing

* proto ci

* val basic testing

* e2e test

* main update

* nit
@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 protorev All things related to x/protorev V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants