Change the name of the balancer proto service from Msg -> BalancerMsg #1208

ValarDragon · 2022-04-06T04:35:34Z

This previously was causing problems for typescript proto generation,
because we have Balancer's proto Msg service and gamm's proto Msg service
existing at the same proto import path.

They existed at the same import path, because due to interface serialization
depending on the amino route (which depended on the proto import path),
changing the import path requires a state migration thats a bit cumbersome to
do.

Closes: #1185

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

This previously was causing problems for typescript proto generation, because we have Balancer's proto Msg service and gamm's proto Msg service existing at the same proto import path. They existed at the same import path, because due to interface serialization depending on the amino route (which depended on the proto import path), changing the import path requires a state migration thats a bit cumbersome to do.

ValarDragon · 2022-04-06T04:36:16Z

proto/osmosis/txfees/v1beta1/query.proto

+    option (google.api.http).get =
+        "/osmosis/txfees/v1beta1/spot_price_by_denom";


done by proto formatting automatically

ValarDragon · 2022-04-06T04:37:33Z

x/gamm/pool-models/balancer/tx.pb.go

-	ServiceName: "osmosis.gamm.v1beta1.Msg",
-	HandlerType: (*MsgServer)(nil),
+var _BalancerMsg_serviceDesc = grpc.ServiceDesc{
+	ServiceName: "osmosis.gamm.v1beta1.BalancerMsg",


Is this service name at all important?

an alternative approach to fixing this problem entirely, is to migrate the balancer pools and not deal with this duplicate import situation

why is still it creating BalancerMsg under v1beta1? I was assuming it should be created as something like osmosis.gamm.pool-models.BalacerMsg

Wdym by migrating balancer pools?

oh thats a good point, we could make txs under the correct package, but just leave the pool proto definition under the legacy wrong one

I think your proposal is a better approach

mattverse

Funny I just came across the same problem while I was working on #1209

Anyways, is x/gamm/pool-models/balancer/tx.pb.go's ServiceName the main concern we have and the main reason we can't push foward on simply chagning to BalancerMsg?

alexanderbez · 2022-04-06T14:30:45Z

proto/osmosis/gamm/pool-models/balancer/tx.proto

@@ -6,7 +6,7 @@ import "osmosis/gamm/pool-models/balancer/balancerPool.proto";

 option go_package = "github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer";

-service Msg {
+service BalancerMsg {


Can you explain how/where this service is used? Does this actually process on-chain messages?

It should process on-chain messages at least, I have not confirmed on a testnet. The service name as far as I can tell is just for generated protobuf structs, we have methods to register the proto service, linking the go struct to the proto message

Sorry if this is obvious, but why are they not all under a single service?

bufbuild/buf#99 (comment)

ValarDragon · 2022-05-03T21:38:00Z

Going to close this as stale for now. Lets update with the fix matt pointed out

@ValarDragon

Closes: [#1185](#1185) ## What is the purpose of the change With proto Msg service defined twice(both in Balancer proto and gamm package proto), this is currently causing problems in typescript proto generation. As @ValarDragon mentioned in #1208, > They existed at the same import path, because due to interface serialization depending on the amino route (which depended on the proto import path), changing the import path requires a state migration thats a bit cumbersome to do. This PR solves this by making txs under the correct package, but leaving balacner pool proto definition under the legacy wrong one, `package osmosis.gamm.v1beta1`. Solving the problem this way allows us to solve without having to need state breaking changes or additional migration for existing pools. In addition to solving the exisitng problem of duplicated Msg in one package, this PR also changes stableswap pool proto package to the correct package. ## Brief Changelog - Change proto package for balancer msg ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? maybe - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes - How is the feature or change documented? not applicable

@ValarDragon

Closes: [osmosis-labs#1185](osmosis-labs#1185) With proto Msg service defined twice(both in Balancer proto and gamm package proto), this is currently causing problems in typescript proto generation. As @ValarDragon mentioned in osmosis-labs#1208, > They existed at the same import path, because due to interface serialization depending on the amino route (which depended on the proto import path), changing the import path requires a state migration thats a bit cumbersome to do. This PR solves this by making txs under the correct package, but leaving balacner pool proto definition under the legacy wrong one, `package osmosis.gamm.v1beta1`. Solving the problem this way allows us to solve without having to need state breaking changes or additional migration for existing pools. In addition to solving the exisitng problem of duplicated Msg in one package, this PR also changes stableswap pool proto package to the correct package. - Change proto package for balancer msg - Does this pull request introduce a new feature or user-facing behavior changes? maybe - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes - How is the feature or change documented? not applicable

## What is the purpose of the change This fix is necessary in order to support ORM. The legacy problem with proto package for balancer has been a problem for a long time, which has been fixed and addressed over multiple PRs(#1208, #1469). The latest decision that has been made upon this legacy package issue was to change the `tx.proto`(https://github.com/osmosis-labs/osmosis/blob/main/proto/osmosis/gamm/pool-models/balancer/tx.proto) to the correct package while leaving `balacerPool.proto` under the legacy wrong package so that we wouldn't have to deal with migration logic. Even though such decision has fixed duplicate package issue that has been a blocker for typescript proto generation, it is now an issue with orm support, as protoc does not allow one package path to have different packages within the protofiles(ex. protoc-gen-go-grpc: Go package "github.com/osmosis-labs/osmosis/api/osmosis/gamm/pool-models/balancer" has inconsistent names gammv1beta1 (osmosis/gamm/pool-models/balancer/balancerPool.proto) and balancerv1beta1 (osmosis/gamm/pool-models/balancer/tx.proto)). There would be multiple ways to fix this problem, open to other ways of fixing this as well. The way this PR solves it is physically creating a new folder for the `tx.proto` file under the `proto/gamm/pool-models/balancer` folder to avoid the problem stated above. --- As for the superfluid module, we have had inconsistency with the packages as well proto package for `gov.proto` set to `osmosis.superfluid.v1beta`, while the other proto packages have the proto package of `osmosis.superfluid`. The suggested fix in this PR is creating a new folder under `proto/superfluid` to fix the duplicate proto package path issue. ## Brief Changelog - Change proto package for `proto/gamm/pool-models/balancer/tx.proto` and `proto/superfluid/gov.proto` ## Testing and Verifying Additional testing needed that this fix does not break unknown unknowns in the state side. ## 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`?no - How is the feature or change documented? (not applicable / specification (`x/<module>/spec/`) / [Osmosis docs repo](https://github.com/osmosis-labs/docs) / not documented)

ValarDragon requested review from alexanderbez and mattverse April 6, 2022 04:35

ValarDragon added the A:backport/v7.x Do not use. backport patches to v7.x branch label Apr 6, 2022

ValarDragon commented Apr 6, 2022

View reviewed changes

mattverse reviewed Apr 6, 2022

View reviewed changes

alexanderbez approved these changes Apr 6, 2022

View reviewed changes

ValarDragon mentioned this pull request May 3, 2022

[x/gamm][Misc improvements] Msg defined twice in different proto files #1185

Closed

ValarDragon closed this May 3, 2022

mattverse mentioned this pull request May 11, 2022

Change proto service for Balancer Msg #1469

Merged

mattverse mentioned this pull request Jun 5, 2022

Fix proto services for balancer and superfluid module #1672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the name of the balancer proto service from Msg -> BalancerMsg #1208

Change the name of the balancer proto service from Msg -> BalancerMsg #1208

ValarDragon commented Apr 6, 2022

ValarDragon Apr 6, 2022

ValarDragon Apr 6, 2022

mattverse Apr 6, 2022

ValarDragon Apr 6, 2022

ValarDragon Apr 6, 2022

mattverse left a comment

alexanderbez Apr 6, 2022

ValarDragon Apr 6, 2022

alexanderbez Apr 6, 2022 •

edited

Loading

mattverse Apr 8, 2022

ValarDragon commented May 3, 2022

		option (google.api.http).get =
		"/osmosis/txfees/v1beta1/spot_price_by_denom";

Change the name of the balancer proto service from Msg -> BalancerMsg #1208

Change the name of the balancer proto service from Msg -> BalancerMsg #1208

Conversation

ValarDragon commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented May 3, 2022

alexanderbez Apr 6, 2022 •

edited

Loading