feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) #3557

p0mvn · 2022-11-28T22:53:40Z

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.

This particular PR does the following:

ports all cli and query logic while disabling tests
ports swaprouter's module package w/o registering types
ports all message server logic
ports all keeper stubs but not the implementations, tests are disabled.
adds a SwapRouterKeeper in app/keepers/keepers.go to let tests build. However, it does not initialize the keeper

There should be no state or API breaks in this PR. Reviewer, please confirm.

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

…no state breaks)

p0mvn · 2022-11-28T22:55:11Z

app/keepers/keepers.go

+	// Note: DO NOT USE. This is not yet initialized
+	SwapRouterKeeper *swaprouter.Keeper


Note: tracked in #3556

p0mvn · 2022-11-28T22:55:33Z

x/swaprouter/client/cli/cli_test.go

+	// TODO: re-enable this once swaprouter is fully merged.
+	t.SkipNow()


Note: tracked in #3556

p0mvn · 2022-11-28T22:57:05Z

x/swaprouter/client/cli/query_test.go

+	// TODO: re-enable this once swaprouter is fully merged.
+	t.SkipNow()


Note: this is tracked in #3556

p0mvn · 2022-11-28T22:58:49Z

x/swaprouter/keeper_test.go

+	// TODO: re-enable this once swaprouter is fully merged.
+	t.SkipNow()


Note: this is tracked in #3556

p0mvn · 2022-11-28T22:59:48Z

x/swaprouter/module/module.go

+	// TODO: uncomment this once simulation is enabled.
+	// swaprouterGen.Params.PoolCreationFee = sdk.NewCoins(swaproutersimulation.PoolCreationFee)


Note: this is tracked in #3556

p0mvn · 2022-11-28T23:00:47Z

x/swaprouter/module/module.go

+	// TODO: uncomment this once simulation is enabled.
+	// return swaproutersimulation.DefaultActions(am.k, am.gammKeeper)
+	return nil


Note: this is tracked in #3556

czarcas7ic

LGTM 👍

czarcas7ic · 2022-11-28T23:18:57Z

x/swaprouter/client/cli/cli_test.go

+	val := s.network.Validators[0]
+
+	info, _, err := val.ClientCtx.Keyring.NewMnemonic("NewSwapExactAmountOut",
+		keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
+	s.Require().NoError(err)
+
+	newAddr := sdk.AccAddress(info.GetPubKey().Address())
+
+	_, err = banktestutil.MsgSendExec(
+		val.ClientCtx,
+		val.Address,
+		newAddr,
+		sdk.NewCoins(sdk.NewInt64Coin(s.cfg.BondDenom, 20000), sdk.NewInt64Coin("node0token", 20000)), fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
+		fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+		osmoutils.DefaultFeeString(s.cfg),
+	)
+	s.Require().NoError(err)


seems like a lot of boilerplate that gets reused. Do you think we should make an issue to fix this in a subsequent PR?

Agreed that this isn't clean. However, auto CLI will fix it soon so I wouldn't spend any time refactoring CLI

czarcas7ic · 2022-11-28T23:42:47Z

x/swaprouter/client/cli/parse.go

+// TODO: move these to exported types within an internal package
+type XCreatePoolInputs createBalancerPoolInputs
+
+type XCreatePoolInputsExceptions struct {
+	XCreatePoolInputs
+	Other *string // Other won't raise an error
+}
+
+type XCreateStableswapPoolInputs createStableswapPoolInputs
+
+type XCreateStableswapPoolInputsExceptions struct {
+	XCreateStableswapPoolInputs
+	Other *string // Other won't raise an error
+}


Can you explain what is going on here / why the prefix?

I don't know. This is a copy from gamm. Seems to be coming from this PR: #3294

Based on the description, it inherits a code smell from the balancer. I wouldn't worry about these CLI issues. The existing cli tests ensure that these work. These code smells are going to be replaced with Auto CLI soon.

czarcas7ic · 2022-11-28T23:49:34Z

x/swaprouter/client/cli/tx.go

+
+			txf := tx.NewFactoryCLI(clientCtx, cmd.Flags()).WithTxConfig(clientCtx.TxConfig).WithAccountRetriever(clientCtx.AccountRetriever)
+
+			txf, msg, err := NewBuildCreateBalancerPoolMsg(clientCtx, txf, cmd.Flags())


How does this work, I thought we use the same CreatePool cli cmd for both balancer and stableswap?

We use the same CreatePool keeper method. This method takes a CreatePoolMsg interface. Each pool has a message implementing this interface.

This is the client side of things where the message is being built. What you're referring to is chain side.

czarcas7ic · 2022-11-28T23:54:49Z

x/swaprouter/keeper.go

+func NewKeeper(storeKey sdk.StoreKey, paramSpace paramtypes.Subspace, gammKeeper types.SwapI, concentratedKeeper types.SwapI, bankKeeper types.BankI, accountKeeper types.AccountI, communityPoolKeeper types.CommunityPoolI) *Keeper {
+	// set KeyTable if it has not already been set
+	if !paramSpace.HasKeyTable() {
+		paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable())
+	}
+
+	routes := map[types.PoolType]types.SwapI{
+		types.Balancer:     gammKeeper,
+		types.StableSwap:   gammKeeper,
+		types.Concentrated: concentratedKeeper,
+	}
+
+	return &Keeper{storeKey: storeKey, paramSpace: paramSpace, gammKeeper: gammKeeper, concentratedKeeper: concentratedKeeper, bankKeeper: bankKeeper, accountKeeper: accountKeeper, communityPoolKeeper: communityPoolKeeper, routes: routes}
+}


We aren't initializing this keeper anywhere yet right? Just ensuring there are in fact no state breaks

Yes, that's correct!

czarcas7ic · 2022-11-28T23:55:39Z

x/swaprouter/keeper_test.go

+		suite.PrepareBalancerPool()
+		return
+	case types.StableSwap:
+		// TODO


Why cant we add SS to this in a state machine compatible way? Im only asking with the assumption that the next PR is the state break, so might as well get everything SM compatible in this PR.

The tests for stableswap haven't been added yet. That's why the stableswap logic hasn't been added here yet, There is an issue to add that later

p0mvn · 2022-11-29T01:44:31Z

Merging this since there are no state breaks and each PR has been reviewed in detail on concentrated-liquidity-main branch

feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - …

72925e8

…no state breaks)

p0mvn changed the title ~~feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - …~~ feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) Nov 28, 2022

github-actions bot added C:app-wiring Changes to the app folder C:CLI C:x/swaprouter labels Nov 28, 2022

p0mvn commented Nov 28, 2022

View reviewed changes

p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Nov 28, 2022

p0mvn marked this pull request as ready for review November 28, 2022 23:05

p0mvn requested review from a team and czarcas7ic November 28, 2022 23:05

czarcas7ic approved these changes Nov 28, 2022

View reviewed changes

p0mvn merged commit a0675d2 into main Nov 29, 2022

p0mvn deleted the roman/cl-swaprouter3 branch November 29, 2022 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) #3557

feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) #3557

p0mvn commented Nov 28, 2022 •

edited

Loading

p0mvn Nov 28, 2022

p0mvn Nov 28, 2022

p0mvn Nov 28, 2022

p0mvn Nov 28, 2022

p0mvn Nov 28, 2022

p0mvn Nov 28, 2022

czarcas7ic left a comment

czarcas7ic Nov 28, 2022

p0mvn Nov 29, 2022

czarcas7ic Nov 28, 2022

p0mvn Nov 29, 2022 •

edited

Loading

czarcas7ic Nov 28, 2022

p0mvn Nov 29, 2022

czarcas7ic Nov 28, 2022

p0mvn Nov 29, 2022

czarcas7ic Nov 28, 2022

p0mvn Nov 29, 2022 •

edited

Loading

p0mvn commented Nov 29, 2022

		// Note: DO NOT USE. This is not yet initialized
		SwapRouterKeeper *swaprouter.Keeper

		// TODO: re-enable this once swaprouter is fully merged.
		t.SkipNow()

		// TODO: uncomment this once simulation is enabled.
		// swaprouterGen.Params.PoolCreationFee = sdk.NewCoins(swaproutersimulation.PoolCreationFee)


		txf := tx.NewFactoryCLI(clientCtx, cmd.Flags()).WithTxConfig(clientCtx.TxConfig).WithAccountRetriever(clientCtx.AccountRetriever)

		txf, msg, err := NewBuildCreateBalancerPoolMsg(clientCtx, txf, cmd.Flags())

feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) #3557

feat(CL): swaprouter cli, queries, module and keeper stubs (part 3 - no state breaks) #3557

Conversation

p0mvn commented Nov 28, 2022 • edited Loading

What is the purpose of the change

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

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

Choose a reason for hiding this comment

p0mvn Nov 29, 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

p0mvn Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn commented Nov 29, 2022

p0mvn commented Nov 28, 2022 •

edited

Loading

p0mvn Nov 29, 2022 •

edited

Loading

p0mvn Nov 29, 2022 •

edited

Loading