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

refactor(x/swaprouter, x/gamm): unify swap routes in swaprouter #3880

Merged
merged 10 commits into from
Jan 9, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Dec 29, 2022

Closes: #3808

What is the purpose of the change

This PR removes the swap routes in x/gamm and refactors all legacy x/gamm swap messages and methods to utilize the new swap route structs from the x/swaprouter module.

This is done to reduce code duplication and redundant conversions between the different swap routes.

Brief Changelog

  • removed proto declarations for all x/gamm swap routes but SwapExactAmountInRoute
    • It is kept for the serialization / deserialization test but is to be removed in the future.
  • substituted usage of old x/gamm swap routes with the x/swaprouter swap routes.

Testing and Verifying

This change added tests and can be verified as follows:

func TestSwapRoutes_MarshalUnmarshal(t *testing.T) {

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? yes
  • How is the feature or change documented? not applicable

@github-actions github-actions bot added C:app-wiring Changes to the app folder C:CLI C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module. labels Dec 29, 2022
@p0mvn p0mvn changed the title reafactor(x/swaprouter, x/gamm): unify swap routes in swaprouter refactor(x/swaprouter, x/gamm): unify swap routes in swaprouter Dec 29, 2022
@p0mvn p0mvn added the V:state/breaking State machine breaking PR label Dec 29, 2022
@p0mvn
Copy link
Member Author

p0mvn commented Dec 29, 2022

While this is not state-breaking, I added the label still to be more careful with this PR as it has an important API break.

@p0mvn p0mvn marked this pull request as ready for review December 29, 2022 20:03
proto/osmosis/gamm/v1beta1/tx.proto Outdated Show resolved Hide resolved
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.

Left one question / concerning point about gamm types. Other then that, looks LGTM

x/gamm/types/route_test.go Outdated Show resolved Hide resolved
Comment on lines +286 to 289
repeated osmosis.swaprouter.v1beta1.SwapAmountOutRoute routes = 3 [
(gogoproto.moretags) = "yaml:\"routes\"",
(gogoproto.nullable) = false
];
Copy link
Member

Choose a reason for hiding this comment

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

We should ask dan lynch and front-end how breaking this is.

Its not clear to me that its worth breaking this in the gamm proto, vs just doing a conversion in the proto boilerplate handling code.

Copy link
Member

Choose a reason for hiding this comment

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

(I think it will break their compiled types, unless we can register a way for a type to get aliased or something in codegen)

Copy link

@pyramation pyramation Jan 4, 2023

Choose a reason for hiding this comment

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

as long as the interfaces/API don't change, it doesn't matter where the types live. Telescope will handle it.

from what I can see here it looks the the interface to the type doesn't change, so will be totally ok!

cc @p0mvn

Choose a reason for hiding this comment

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

also, even w/o telescope, from what I can tell (since I believe osmosis-fe uses another tool) — nothing should break. But, we should probably all keep in sync when this update rolls out to update FE libs and repos.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @pyramation . I also spoke with Jon to make sure that there is no unexpected breakage for FE.

We will track this change and remind everyone to update before this is out.

This should be good to go

@ValarDragon
Copy link
Member

ValarDragon commented Jan 9, 2023

Needs changelog entry for breaking changes if you worked with queries via JSON, otherwise LGTM.

  • (or is it JSON serialization compatible?)

(Maybe we can just add a TODO for us to check when we update front-end, may remove it from being marked as breaking, and then it'd just be a misc improvement)

Its totally fine for contracts, and an easy update, so LGTM!

@p0mvn
Copy link
Member Author

p0mvn commented Jan 9, 2023

Seems to be JSON serialization compatible based on added test here: 3a46711

@ValarDragon
Copy link
Member

oh awesome, thats great!

@p0mvn p0mvn merged commit 719ea37 into main Jan 9, 2023
@p0mvn p0mvn deleted the roman/gamm-swaprouter-routes branch January 9, 2023 05:33
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:CLI C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module. V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test(x/swaprouter): add a test to validate that swap routes serialize correctly
5 participants