Review: Types Package, model package holistic review (#5266)

* WIP

* WIP

* Remove unnecessary seperator usage

* Update Readme

* Remove unnecessary fmt

* Update x/concentrated-liquidity/types/keys.go

Co-authored-by: Roman <[email protected]>

* Update x/concentrated-liquidity/types/codec.go

Co-authored-by: Roman <[email protected]>

* Fix merge conflict

* Fix merge conflict

* Alpin and adams comments

* Update x/concentrated-liquidity/README.md

Co-authored-by: Adam Tucker <[email protected]>

* Update x/concentrated-liquidity/model/pool_test.go

Co-authored-by: Adam Tucker <[email protected]>

---------

Co-authored-by: Roman <[email protected]>
Co-authored-by: Adam Tucker <[email protected]>

x/concentrated-liquidity/README.md

@@ -1541,13 +1541,22 @@ This listener executes after a swap in a concentrated liquidity pool.
 
 At the time of this writing, it is only utilized by the `x/twap` module.
 
-## State
 
-- global (per-pool)
+### State entries and KV store management
+The following are the state entries (key and value pairs) stored for the concentrated liquidity module. 
 
-- per-tick
+- structs
+  - TickPrefix + pool ID + tickIndex ➝ Tick Info struct
+  - PoolPrefix + pool id ➝ pool struct
+  - IncentivePrefix | pool id | min uptime index | denom | addr ➝ Incentive Record body struct
+- links
+  - positionToLockPrefix | position id ➝ lock id
+  - lockToPositionPrefix | lock id ➝ position id
+  - PositionPrefix | addr bytes | pool id | position id ➝ boolean
+  - PoolPositionPrefix | pool id | position id ➝ boolean
+
+Note that for storing ticks, we use 9 bytes instead of directly using uint64, first byte being reserved for the Negative / Positive prefix, and the remaining 8 bytes being reserved for the tick itself, which is of uint64. Although we directly store signed integers as values, we use the first byte to indicate and re-arrange tick indexes from negative to positive.
 
-- per-position
 
 ## Terminology
 

x/concentrated-liquidity/model/codec.go

@@ -38,8 +38,6 @@ func RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
 	msgservice.RegisterMsgServiceDesc(registry, &_MsgCreator_serviceDesc)
 }
 
-// TODO: re-enable this when CL state-breakage PR is merged.
-// return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))
 var (
 	amino     = codec.NewLegacyAmino()
 	ModuleCdc = codec.NewAminoCodec(amino)

x/concentrated-liquidity/model/pool.go

@@ -273,7 +273,8 @@ func (p Pool) IsCurrentTickInRange(lowerTick, upperTick int64) bool {
 }
 
 // ApplySwap state of pool after swap.
-// It specifically overwrites the pool's liquidity, curr tick and the curr sqrt price
+// It specifically overwrites the pool's liquidity, curr tick and the curr sqrt price.
+// Note that this method is mutative.
 func (p *Pool) ApplySwap(newLiquidity sdk.Dec, newCurrentTick int64, newCurrentSqrtPrice sdk.Dec) error {
 	// Check if the new liquidity provided is not negative.
 	if newLiquidity.IsNegative() {

x/concentrated-liquidity/model/pool_test.go

@@ -8,6 +8,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/suite"
 
+	"github.com/osmosis-labs/osmosis/osmoutils/osmoassert"
 	"github.com/osmosis-labs/osmosis/v15/app/apptesting"
 	clmath "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/math"
 	"github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/model"
@@ -41,6 +42,99 @@ func TestConcentratedPoolTestSuite(t *testing.T) {
 	suite.Run(t, new(ConcentratedPoolTestSuite))
 }
 
+// TestGetAddress tests the GetAddress method of pool
+func (s *ConcentratedPoolTestSuite) TestGetAddress() {
+
+	tests := []struct {
+		name          string
+		expectedPanic bool
+	}{
+		{
+			name: "Happy path",
+		},
+		{
+			name:          "Unhappy path: wrong bech32 encoded address",
+			expectedPanic: true,
+		},
+	}
+
+	for _, tc := range tests {
+		s.Run(tc.name, func() {
+			// Init suite for each test.
+			s.Setup()
+
+			address := s.TestAccs[0].String()
+
+			// if the test case is expected to panic, we use wrong bech32 encoded address instead
+			if tc.expectedPanic {
+				address = "osmo15l7yueqf3tx4cvpt6njvj7zxmvuhkwyrr509e9"
+			}
+			mock_pool := model.Pool{
+				Id:      1,
+				Address: address,
+			}
+
+			// Check that the returned address is backward compatible
+			osmoassert.ConditionalPanic(s.T(), tc.expectedPanic, func() {
+				addr := mock_pool.GetAddress()
+				s.Require().Equal(addr, s.TestAccs[0])
+			})
+		})
+	}
+}
+
+// TestGetIncentivesAddress tests the GetIncentivesAddress method of pool
+func (s *ConcentratedPoolTestSuite) TestGetIncentivesAddress() {
+
+	tests := []struct {
+		name          string
+		expectedPanic bool
+	}{
+		{
+			name: "Happy path",
+		},
+		{
+			name:          "Unhappy path: wrong bech32 encoded address",
+			expectedPanic: true,
+		},
+	}
+
+	for _, tc := range tests {
+		s.Run(tc.name, func() {
+			// Init suite for each test.
+			s.Setup()
+
+			// Create a concentrated liquidity pool struct instance
+			address := s.TestAccs[0].String()
+
+			// if the test case is expected to panic, we use wrong bech32 encoded address instead
+			if tc.expectedPanic {
+				address = "osmo15l7yueqf3tx4cvpt6njvj7zxmvuhkwyrr509e9"
+			}
+			mock_pool := model.Pool{
+				Id:                1,
+				IncentivesAddress: address,
+			}
+
+			// Check that the returned address is backward compatible
+			osmoassert.ConditionalPanic(s.T(), tc.expectedPanic, func() {
+				addr := mock_pool.GetIncentivesAddress()
+				s.Require().Equal(addr, s.TestAccs[0])
+			})
+		})
+	}
+}
+
+// TestString tests if String method of the pool correctly json marshals the pool object
+func (s *ConcentratedPoolTestSuite) TestString() {
+	s.Setup()
+
+	pool, err := model.NewConcentratedLiquidityPool(1, "foo", "bar", DefaultTickSpacing, DefaultSpreadFactor)
+	s.Require().NoError(err)
+	poolString := pool.String()
+	s.Require().Equal(poolString, "{\"address\":\"osmo19e2mf7cywkv7zaug6nk5f87d07fxrdgrladvymh2gwv5crvm3vnsuewhh7\",\"incentives_address\":\"osmo156gncm3w2hdvuxxaejue8nejxgdgsrvdf7jftntuhxnaarhxcuas4ywjxf\",\"spread_rewards_address\":\"osmo10t3u6ze74jn7et6rluuxyf9vr2arykewmhcx67svg6heuu0gte2syfudcv\",\"id\":1,\"current_tick_liquidity\":\"0.000000000000000000\",\"token0\":\"foo\",\"token1\":\"bar\",\"current_sqrt_price\":\"0.000000000000000000\",\"tick_spacing\":1,\"exponent_at_price_one\":-6,\"spread_factor\":\"0.010000000000000000\",\"last_liquidity_update\":\"0001-01-01T00:00:00Z\"}")
+}
+
 // TestSpotPrice tests the SpotPrice method of the ConcentratedPoolTestSuite.
 func (s *ConcentratedPoolTestSuite) TestSpotPrice() {
 	type param struct {
@@ -172,6 +266,18 @@ func (s *ConcentratedPoolTestSuite) TestIsCurrentTickInRange() {
 			DefaultCurrTick,
 			false,
 		},
+		{
+			"only lower tick is equal to the pool tick",
+			DefaultCurrTick,
+			DefaultCurrTick + 3,
+			true,
+		},
+		{
+			"only upper tick is equal to the pool tick",
+			DefaultCurrTick - 3,
+			DefaultCurrTick,
+			false,
+		},
 		{
 			"lower tick is greater then pool tick",
 			DefaultCurrTick + 1,
@@ -250,7 +356,7 @@ func (s *ConcentratedPoolTestSuite) TestApplySwap() {
 			expectErr:        types.SqrtPriceNegativeError{ProvidedSqrtPrice: negativeOne},
 		},
 		{
-			name:             "upper tick too big",
+			name:             "upper tick is greater than max tick",
 			currentLiquidity: DefaultLiquidityAmt,
 			currentTick:      1,
 			currentSqrtPrice: DefaultCurrSqrtPrice,
@@ -264,7 +370,7 @@ func (s *ConcentratedPoolTestSuite) TestApplySwap() {
 			},
 		},
 		{
-			name:             "lower tick too small",
+			name:             "lower tick is smaller than min tick",
 			currentLiquidity: DefaultLiquidityAmt,
 			currentTick:      1,
 			currentSqrtPrice: DefaultCurrSqrtPrice,

x/concentrated-liquidity/position.go

@@ -754,7 +754,7 @@ func (k Keeper) positionHasActiveUnderlyingLockAndUpdate(ctx sdk.Context, positi
 // - there is no full range liquidity in the pool.
 func (k Keeper) GetFullRangeLiquidityInPool(ctx sdk.Context, poolId uint64) (sdk.Dec, error) {
 	store := ctx.KVStore(k.storeKey)
-	poolIdLiquidityKey := types.KeyPoolIdForLiquidity(poolId)
+	poolIdLiquidityKey := types.KeyFullRangeLiquidityPrefix(poolId)
 	currentTotalFullRangeLiquidity, err := osmoutils.GetDec(store, poolIdLiquidityKey)
 	if err != nil {
 		return sdk.Dec{}, err
@@ -766,7 +766,7 @@ func (k Keeper) GetFullRangeLiquidityInPool(ctx sdk.Context, poolId uint64) (sdk
 func (k Keeper) updateFullRangeLiquidityInPool(ctx sdk.Context, poolId uint64, liquidity sdk.Dec) error {
 	store := ctx.KVStore(k.storeKey)
 	// Get previous total liquidity.
-	poolIdLiquidityKey := types.KeyPoolIdForLiquidity(poolId)
+	poolIdLiquidityKey := types.KeyFullRangeLiquidityPrefix(poolId)
 	currentTotalFullRangeLiquidityDecProto := sdk.DecProto{}
 	found, err := osmoutils.Get(store, poolIdLiquidityKey, &currentTotalFullRangeLiquidityDecProto)
 	if err != nil {

x/concentrated-liquidity/store.go

@@ -92,7 +92,7 @@ func ParseFullTickFromBytes(key, value []byte) (tick genesis.FullTick, err error
 		return genesis.FullTick{}, types.ValueNotFoundForKeyError{Key: key}
 	}
 
-	if len(key) != types.TickKeyLengthBytes {
+	if len(key) != types.KeyTickLengthBytes {
 		return genesis.FullTick{}, types.InvalidTickKeyByteLengthError{Length: len(key)}
 	}
 

x/concentrated-liquidity/types/cl.go

@@ -4,18 +4,7 @@ import (
 	fmt "fmt"
 )
 
-func OrderInitialPoolDenoms(denom0, denom1 string) (string, string, error) {
-	if denom0 == denom1 {
-		return "", "", fmt.Errorf("cannot have the same asset in a single pool")
-	}
-	if denom0 > denom1 {
-		denom1, denom0 = denom0, denom1
-	}
-
-	return denom0, denom1, nil
-}
-
-// GetConcentratedLockupDenomFromPoolId returns the concentrated lockup denom for a given pool.
+// GetConcentratedLockupDenomFromPoolId returns the concentrated lockup denom for a given pool id.
 func GetConcentratedLockupDenomFromPoolId(poolId uint64) string {
 	return fmt.Sprintf("%s/%d", ConcentratedLiquidityTokenPrefix, poolId)
 }

x/concentrated-liquidity/types/codec.go

@@ -11,13 +11,17 @@ import (
 
 func RegisterCodec(cdc *codec.LegacyAmino) {
 	cdc.RegisterInterface((*ConcentratedPoolExtension)(nil), nil)
+
+	// msgs
 	cdc.RegisterConcrete(&MsgCreatePosition{}, "osmosis/cl-create-position", nil)
 	cdc.RegisterConcrete(&MsgAddToPosition{}, "osmosis/cl-add-to-position", nil)
 	cdc.RegisterConcrete(&MsgWithdrawPosition{}, "osmosis/cl-withdraw-position", nil)
 	cdc.RegisterConcrete(&MsgCollectSpreadRewards{}, "osmosis/cl-col-sp-rewards", nil)
 	cdc.RegisterConcrete(&MsgCollectIncentives{}, "osmosis/cl-collect-incentives", nil)
 	cdc.RegisterConcrete(&MsgFungifyChargedPositions{}, "osmosis/cl-fungify-charged-positions", nil)
-	cdc.RegisterConcrete(&CreateConcentratedLiquidityPoolsProposal{}, "osmosis/create-concentrated-liquidity-pools-proposal", nil)
+
+	// gov proposals
+	cdc.RegisterConcrete(&CreateConcentratedLiquidityPoolsProposal{}, "osmosis/create-cl-pools-proposal", nil)
 	cdc.RegisterConcrete(&TickSpacingDecreaseProposal{}, "osmosis/cl-tick-spacing-dec-prop", nil)
 }
 

x/concentrated-liquidity/types/constants.go

@@ -28,8 +28,8 @@ var (
 		sdk.MustNewDecFromStr("0.001"),  // 0.1%
 		sdk.MustNewDecFromStr("0.002"),  // 0.2%
 		sdk.MustNewDecFromStr("0.003"),  // 0.3%
-		sdk.MustNewDecFromStr("0.005"),
-	} // 0.5%
+		sdk.MustNewDecFromStr("0.005"),  // 0.5%
+	}
 	BaseGasFeeForNewIncentive     = 10_000
 	DefaultBalancerSharesDiscount = sdk.MustNewDecFromStr("0.05")
 	// By default, we only authorize one nanosecond (one block) uptime as an option

x/concentrated-liquidity/types/errors.go

@@ -468,7 +468,7 @@ type InvalidTickKeyByteLengthError struct {
 }
 
 func (e InvalidTickKeyByteLengthError) Error() string {
-	return fmt.Sprintf("expected tick store key to be of length (%d), was (%d)", TickKeyLengthBytes, e.Length)
+	return fmt.Sprintf("expected tick store key to be of length (%d), was (%d)", KeyTickLengthBytes, e.Length)
 }
 
 type InsufficientPoolBalanceError struct {

x/concentrated-liquidity/types/gov.go

@@ -15,7 +15,7 @@ const (
 
 func init() {
 	govtypes.RegisterProposalType(ProposalTypeCreateConcentratedLiquidityPool)
-	govtypes.RegisterProposalTypeCodec(&CreateConcentratedLiquidityPoolsProposal{}, "osmosis/CreateConcentratedLiquidityPoolProposal")
+	govtypes.RegisterProposalTypeCodec(&CreateConcentratedLiquidityPoolsProposal{}, "osmosis/CreateCLPoolsProposal")
 	govtypes.RegisterProposalType(ProposalTypeTickSpacingDecrease)
 	govtypes.RegisterProposalTypeCodec(&TickSpacingDecreaseProposal{}, "osmosis/TickSpacingDecreaseProposal")
 }
@@ -126,6 +126,16 @@ func (p *TickSpacingDecreaseProposal) ValidateBasic() error {
 	if len(p.PoolIdToTickSpacingRecords) == 0 {
 		return fmt.Errorf("empty proposal records")
 	}
+
+	for _, poolIdToTickSpacingRecord := range p.PoolIdToTickSpacingRecords {
+		if poolIdToTickSpacingRecord.PoolId <= uint64(0) {
+			return fmt.Errorf("Pool Id cannot be negative")
+		}
+
+		if poolIdToTickSpacingRecord.NewTickSpacing <= uint64(0) {
+			return fmt.Errorf("tick spacing must be positive")
+		}
+	}
 	return nil
 }
 

x/concentrated-liquidity/types/gov_test.go

@@ -68,3 +68,43 @@ func TestCreateConcentratedLiquidityPoolsProposalMarshalUnmarshal(t *testing.T)
 		require.Equal(t, *test.proposal, decoded)
 	}
 }
+
+func TestTickSpacingDecreaseProposalMarshalUnmarshal(t *testing.T) {
+	tests := []struct {
+		proposal *types.TickSpacingDecreaseProposal
+	}{
+		{ // empty title
+			proposal: &types.TickSpacingDecreaseProposal{
+				Title:       "",
+				Description: "proposal to update migration records",
+			},
+		},
+		{ // empty description
+			proposal: &types.TickSpacingDecreaseProposal{
+				Title:       "title",
+				Description: "",
+			},
+		},
+		{ // happy path
+			proposal: &types.TickSpacingDecreaseProposal{
+				Title:       "title",
+				Description: "proposal to update migration records",
+				PoolIdToTickSpacingRecords: []types.PoolIdToTickSpacingRecord{
+					{
+						PoolId:         1,
+						NewTickSpacing: uint64(1),
+					},
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		bz, err := proto.Marshal(test.proposal)
+		require.NoError(t, err)
+		decoded := types.TickSpacingDecreaseProposal{}
+		err = proto.Unmarshal(bz, &decoded)
+		require.NoError(t, err)
+		require.Equal(t, *test.proposal, decoded)
+	}
+}