refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) #3560

p0mvn · 2022-11-29T02:24:26Z

Closes: #XXX

What is the purpose of the change

We would like to drop concentrated-liquidity-main branch to avoid branch synching overhead. As part of that, we are going to be incrementally merging the current concentrated liquidity feature state to main.

All this logic has already been given a round of reviews since we treated concentrated-liqudiity-main branch as main with appropriate review processes.

Note that the large diff stems from regenerating mocks. The actual change is under 250 LOC. The state break stems from proto-registering the new interface.

The main theme of this PR is the following:

implements CFMMPoolI defining an interface that all cfmm/gamm pools must implement
using CFMMPoolI throughout gamm. Using swaproutertypes.PoolI everywhere else
regenerating test mocks to be aligned with the new interfaces

Secondary changes:

Implementing GetPoolType() on CreatePoolMsg
implementing and testing convertToCFMMPool
Switching to using some identical constants and events that have been moved to `swaprouter

For the latest state of the feature that we are merging, see: https://github.com/osmosis-labs/osmosis/tree/concentrated-liquidity-main

See spec: https://github.com/osmosis-labs/osmosis/pull/3052/files

p0mvn · 2022-11-29T02:43:48Z

x/gamm/keeper/pool.go

+// convertToCFMMPool converts PoolI to CFMMPoolI by casting the input.
+// Returns the pool of the CFMMPoolI or error if the given pool does not implement
+// CFMMPoolI.
+// nolint: unused


Note: removing this is tracked in #3556

czarcas7ic

LGTM, approving and then can merge on @ValarDragon approval due to state break as discussed previously 🏄🏻‍♂️

czarcas7ic · 2022-11-30T01:44:53Z

x/gamm/keeper/pool.go

+func (k Keeper) MarshalPool(pool swaproutertypes.PoolI) ([]byte, error) {
 	return k.cdc.MarshalInterface(pool)
 }

-func (k Keeper) UnmarshalPool(bz []byte) (types.PoolI, error) {
-	var acc types.PoolI
+func (k Keeper) UnmarshalPool(bz []byte) (types.CFMMPoolI, error) {
+	var acc types.CFMMPoolI


Just to clarify, is it intended to Marshal from PoolI but Unmarshal into CFMMPoolI?

Yeah. The reason is convenience. When we create a pool it comes in to gamm as PoolI from swaprouter. As a result, it is cleaner to marshal it directly without having to cast it to CFMMPoolI.

On the other hand, when we get a pool, it is usually to perform some operations within the gamm module. Most of these calculations require CFMMPoolI methods.

Its unclear to me that it would be safe to Marshal to CFMMPoolI, I think we may have to always marshal and unmarshal from PoolI. (Which is what is happening here)

At least we would need to double check the serialized bytecode. IIRC, the proto import path + struct name does get serialized into @type

There should be a way for us to check this though, by serializing the interface to JSON

I'm going to look into this.

Please see test: d7d388d

Just noticed that you specified serializing specifically to JSON. Why would JSON matter? Would the test above address your concern?

JSON was suggested just for debugging

czarcas7ic · 2022-11-30T01:49:36Z

x/gamm/keeper/share.go

 )

-func (k Keeper) applyJoinPoolStateChange(ctx sdk.Context, pool types.PoolI, joiner sdk.AccAddress, numShares sdk.Int, joinCoins sdk.Coins) error {
+func (k Keeper) applyJoinPoolStateChange(ctx sdk.Context, pool types.CFMMPoolI, joiner sdk.AccAddress, numShares sdk.Int, joinCoins sdk.Coins) error {


why does this specifically call for CFMMPoolI but the others use PoolI?

There was no reason for this being CFMMPoolI. So I updated it to PoolI which has fewer methods.

ValarDragon · 2022-11-30T03:21:27Z

x/gamm/pool-models/balancer/codec.go

+		"osmosis.swaprouter.v1beta1.PoolI",
+		(*swaproutertypes.PoolI)(nil),
+		&Pool{},
+	)
+	registry.RegisterInterface(
+		"osmosis.gamm.v1beta1.CFMMPoolI",
+		(*types.CFMMPoolI)(nil),


imo we need to keep the old names here, otherwise we would need a relatively complex state migration.

Alternative is we make state migration code that takes the old type and casts it to a new type. This seems like a complex integrator break, that doesn't seem worth it imo

Something to keep in mind on this matter - we have an e2e test that pre-creates pools before the upgrade, runs some swaps against them, performs the upgrade, and runs some swaps again.

If the e2e test is passing, do you think still that the migrations would be necessary?

Either way, I will try to understand whether this is unsafe by looking into RegisterInterface(...) logic

Reverted to old name to avoid breaking clients:

834bc85

ValarDragon · 2022-12-01T16:10:28Z

x/gamm/types/codec.go

+		"osmosis.gamm.v1beta1.CFMMPoolI",
+		(*CFMMPoolI)(nil),


Wait we should keep this as "osmosis.gamm.v1beta1.PoolI"

Now im even more confused, we had one entry at time of this comment, now were back at two entries.

I thought this PR was jsut renaming PoolI -> CFMMPoolI. If so, then there should just be one interface registered, with the interface route being the old one.

x/gamm/types/codec.go

ValarDragon · 2022-12-02T04:21:42Z

x/gamm/keeper/pool.go

 )

-func (k Keeper) MarshalPool(pool types.PoolI) ([]byte, error) {
+func (k Keeper) MarshalPool(pool swaproutertypes.PoolI) ([]byte, error) {
 	return k.cdc.MarshalInterface(pool)


err does this work without casting to CFMMPoolI ?

ValarDragon

Reviewed synchronously! LGTM, nice job

Theres a test confirming these are serialization equivalent (so the interface serialization didn't include this), we just can't have another interface, that uses this as a constituent interface.

I think for backwards consistency, we should still make the pairs in all 3 spots:
"osmosis.gamm.v1beta1.PoolI" -> CFMMPoolI

"osmosis.swaprouter.v1beta1.PoolI" -> swaprouter.PoolI

p0mvn · 2022-12-05T19:01:20Z

Paths updated as discussed so merging this to continue progressing with the swaprouter merge

github-actions bot added C:app-wiring Changes to the app folder C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid C:x/txfees labels Nov 29, 2022

p0mvn changed the title ~~feat(CL): create CFMMPoolI interface, refactor PoolI~~ refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) Nov 29, 2022

p0mvn added the V:state/breaking State machine breaking PR label Nov 29, 2022

p0mvn commented Nov 29, 2022

View reviewed changes

p0mvn marked this pull request as ready for review November 29, 2022 02:45

p0mvn requested review from a team, czarcas7ic and ValarDragon November 29, 2022 02:45

czarcas7ic approved these changes Nov 30, 2022

View reviewed changes

ValarDragon reviewed Nov 30, 2022

View reviewed changes

p0mvn and others added 7 commits November 30, 2022 15:49

feat(CL): create CFMMPoolI interface, refactor PoolI

89a39b0

use gammKeeper in tests

e543ffb

fix interface proto path

feba479

restore test

c661d62

lint

5f71fc1

update applyJoinPoolStateChange to take PoolI

97f1e12

test marshaling

c5000a2

p0mvn force-pushed the roman/cl-pool-refactor branch from d7d388d to 9f01762 Compare November 30, 2022 20:49

revert pool model proto names

834bc85

p0mvn force-pushed the roman/cl-pool-refactor branch from 9f01762 to 834bc85 Compare November 30, 2022 20:51

p0mvn requested a review from ValarDragon November 30, 2022 20:51

ValarDragon reviewed Dec 1, 2022

View reviewed changes

p0mvn commented Dec 1, 2022

View reviewed changes

x/gamm/types/codec.go Outdated Show resolved Hide resolved

Update x/gamm/types/codec.go

feba4b6

ValarDragon reviewed Dec 2, 2022

View reviewed changes

ValarDragon approved these changes Dec 2, 2022

View reviewed changes

p0mvn added 3 commits December 2, 2022 12:26

update paths for backward compatibility

df0ee16

Merge branch 'main' into roman/cl-pool-refactor

9b2bf60

Merge branch 'main' into roman/cl-pool-refactor

9eb3f02

p0mvn merged commit fa9293f into main Dec 5, 2022

p0mvn deleted the roman/cl-pool-refactor branch December 5, 2022 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) #3560

refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) #3560

p0mvn commented Nov 29, 2022 •

edited

Loading

p0mvn Nov 29, 2022

czarcas7ic left a comment

czarcas7ic Nov 30, 2022

p0mvn Nov 30, 2022

ValarDragon Nov 30, 2022 •

edited

Loading

p0mvn Nov 30, 2022

p0mvn Nov 30, 2022

p0mvn Nov 30, 2022 •

edited

Loading

ValarDragon Dec 2, 2022

czarcas7ic Nov 30, 2022

p0mvn Nov 30, 2022

ValarDragon Nov 30, 2022

ValarDragon Nov 30, 2022

p0mvn Nov 30, 2022

p0mvn Nov 30, 2022

p0mvn Nov 30, 2022 •

edited

Loading

ValarDragon Dec 1, 2022 •

edited

Loading

p0mvn Dec 1, 2022

ValarDragon Dec 2, 2022

ValarDragon Dec 2, 2022

ValarDragon left a comment

p0mvn commented Dec 5, 2022

refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) #3560

refactor(CL/gamm): create CFMMPoolI interface, refactor PoolI (merge 4 - state breaking) #3560

Conversation

p0mvn commented Nov 29, 2022 • edited Loading

What is the purpose of the change

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Nov 30, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon Dec 1, 2022 • edited Loading

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

ValarDragon left a comment

Choose a reason for hiding this comment

p0mvn commented Dec 5, 2022

p0mvn commented Nov 29, 2022 •

edited

Loading

ValarDragon Nov 30, 2022 •

edited

Loading

p0mvn Nov 30, 2022 •

edited

Loading

p0mvn Nov 30, 2022 •

edited

Loading

ValarDragon Dec 1, 2022 •

edited

Loading