Skip to content

Commit

Permalink
Review: Types Package, model package holistic review (#5266)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
3 people authored and pysel committed Jun 6, 2023
1 parent 056cf42 commit a1b1b7f
Show file tree
Hide file tree
Showing 15 changed files with 279 additions and 66 deletions.
17 changes: 13 additions & 4 deletions x/concentrated-liquidity/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 0 additions & 2 deletions x/concentrated-liquidity/model/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion x/concentrated-liquidity/model/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
110 changes: 108 additions & 2 deletions x/concentrated-liquidity/model/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions x/concentrated-liquidity/position.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion x/concentrated-liquidity/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)}
}

Expand Down
13 changes: 1 addition & 12 deletions x/concentrated-liquidity/types/cl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
File renamed without changes.
6 changes: 5 additions & 1 deletion x/concentrated-liquidity/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions x/concentrated-liquidity/types/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion x/concentrated-liquidity/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
12 changes: 11 additions & 1 deletion x/concentrated-liquidity/types/gov.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
}

Expand Down
40 changes: 40 additions & 0 deletions x/concentrated-liquidity/types/gov_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Loading

0 comments on commit a1b1b7f

Please sign in to comment.