osmosis-labs/osmosis

The AMM Laboratory

osmosis's Issues

Add multihop routes to SwapMsgs

If people want to do a trade between two assets that aren’t in the same pool, we need to construct a multihop swap.

To do this, we should switch the PoolId in a SwapMsg to into a Route which is an ordered list of PoolIds.

So for example:

type MsgSwapExactAmountIn struct {
	Sender            string
	PoolId            uint64
	TokenIn           types.Coin
	TokenOutDenom     string
	TokenOutMinAmount github_com_cosmos_cosmos_sdk_types.Int
	MaxSpotPrice      github_com_cosmos_cosmos_sdk_types.Dec
}

would become

type MsgSwapExactAmountIn struct {
	Sender            string
	Route             []PoolId (int64)
	TokenIn           types.Coin
	TokenOutDenom     string
	TokenOutMinAmount github_com_cosmos_cosmos_sdk_types.Int
	MaxSpotPrice      github_com_cosmos_cosmos_sdk_types.Dec
}

Make pools messages specify a JSON instead of many error-prone CLI args

As title. We should allow pool's to be specified by a JSON representation, not many distinct CLI arguments. (Especially as there's really no way to 'undo' a pool creation)

This is similar to whats done in the SDK for governance proposals, we can likely lift a lot of the logic from there.

Remove StakingKeeper from Mint module

The mint module doesn't depend on staking anymore, so this dependency can be removed.

Unexpose DeleteLockRefByKey from Lockup Module

Needed in order to keep proper security model for LockupKeeper.

See discussion here:
#22 (comment)

Reference:
https://stackoverflow.com/questions/24622388/how-to-test-a-unexported-private-function-in-go-golang

Add Perpetual Pots to Incentives Module

Improve help command examples for CLI to show required flags

Currently just says osmosisd tx lockup lock-tokens [flags]

Should have examples of required flags

Rename ReductionFactorForEvent to just be ReductionFactor

I think the ForEvent suffix is not helpful here, as this parameter lives within x/mint, in which case there is only one such factor.

Remove `Lock` params from `MsgCreatePool`

Since we have no pool-specific governance at launch, it doesn't make sense to have pools get created with lock's (which is already disabled).

We should remove this field from MsgCreatePool, and then have pools default to have locks off.

Simulation: Make x/gamm spot price checks into invariants

When we add support for x/gamm simulation, we should move the spot price checks from

osmosis/x/gamm/keeper/swap_test.go

Lines 82 to 94 in b6cc74c

	if test.expectPass {
	spotPriceBefore, err := keeper.CalculateSpotPrice(suite.ctx, poolId, test.param.tokenIn.Denom, test.param.tokenOutDenom)
	suite.NoError(err, "test: %v", test.name)
	tokenOutAmount, _, err := keeper.SwapExactAmountIn(suite.ctx, acc1, poolId, test.param.tokenIn, test.param.tokenOutDenom, test.param.tokenOutMinAmount)
	suite.NoError(err, "test: %v", test.name)
	spotPriceAfter, err := keeper.CalculateSpotPrice(suite.ctx, poolId, test.param.tokenIn.Denom, test.param.tokenOutDenom)
	suite.NoError(err, "test: %v", test.name)
	// Ratio of the oken out should be between the before spot price and after spot price.
	sp := test.param.tokenIn.Amount.ToDec().Quo(tokenOutAmount.ToDec())
	suite.True(sp.GT(spotPriceBefore) && sp.LT(spotPriceAfter), "test: %v", test.name)

into an invariant.

Update to cosmos-sdk v0.41.0

See: https://github.com/cosmos/cosmos-sdk/releases/tag/v0.41.0

Make sure norace tests are run in CI

The CLI and GRPC tests should be run using the norace build flag in CI. Need to make sure -tag=norace is passed in to the CI's test commands so it runs these tests and doesn't skip them.

See: cosmos/cosmos-sdk#7305

Make slippage bounds on gamm CLI be optional instead of required

Create lock-up mechanism for LP tokens

By default, when providing liquidity, liquidity providers receive back liquid LP tokens. We want a way to allow liquidity providers to lock their LP tokens for a minimum locktime (i.e. 1 month, 3 months, etc). This lock-ups will help guarantee long term liquidity, which will prevent Vampire Attacks.

These lockups should be easily queryable by other modules, that want to grant certain powers to long-term LPs.

Examples can include:

1. Distribution of liquidity mining rewards
2. Granting of governance power over pool parameters

Rename packages from c-osmosis to osmosis-labs

The repo's go.mod declares this as github.com/c-osmosis/osmosis. We should switch this this to github.com/osmosis-labs/osmosis.

CI: Add a markdown linter

We should add a markdown linter to CI, ala https://github.com/tendermint/spec/blob/master/.github/workflows/linter.yml

Query proto types don't satisfy protoreflect.ProtoMessage

When you run make all twice, it gives me a bunch of errors about the Query proto types not satisfying protoreflect.ProtoMessage (missing ProtoReflect method)

# github.com/c-osmosis/osmosis/x/gamm/types
x/gamm/types/query.pb.gw.go:56:2: cannot use msg (type *QueryPoolResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:82:2: cannot use msg (type *QueryPoolResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:97:44: cannot use &protoReq (type *QueryPoolsRequest) as type protoreflect.ProtoMessage in argument to "github.com/grpc-ecosystem/grpc-gateway/v2/runtime".PopulateQueryParameters:
	*QueryPoolsRequest does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:102:2: cannot use msg (type *QueryPoolsResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolsResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:113:44: cannot use &protoReq (type *QueryPoolsRequest) as type protoreflect.ProtoMessage in argument to "github.com/grpc-ecosystem/grpc-gateway/v2/runtime".PopulateQueryParameters:
	*QueryPoolsRequest does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:118:2: cannot use msg (type *QueryPoolsResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolsResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:144:2: cannot use msg (type *QueryPoolParamsResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolParamsResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:170:2: cannot use msg (type *QueryPoolParamsResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryPoolParamsResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:196:2: cannot use msg (type *QueryTotalShareResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryTotalShareResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:222:2: cannot use msg (type *QueryTotalShareResponse) as type protoreflect.ProtoMessage in return argument:
	*QueryTotalShareResponse does not implement protoreflect.ProtoMessage (missing ProtoReflect method)
x/gamm/types/query.pb.gw.go:222:2: too many errors
make: *** [osmosis] Error 2

Remove SwapFee Governor

This feature was removed from v0 launch roadmap. Should be removed from the current master codebase.

Increase initial shares to 100 * 10^18

Currently, the initial shares given to the first LP of a pool is 100 * 10^6. I wonder if this might not have enough precision. Especially, because usually the initial creator of a pool might be a massive whale (such as the project team) to kickstart the pool and add an extremely high amount of liquidity.

Balancer uses 100 * 10^18 as the initial shares amount.
https://github.com/balancer-labs/balancer-core/blob/master/contracts/BConst.sol#L33

Maybe we should use something similar?

Add a field to pools to allow specifying governer

After #60, we have no way for a pool to specify who its future governor will be, once we build support for it.

Lets add a string for "Future owner of a pool". This can be an address, or a This LP Token, lockup time pair. We then need to build a "validate owner string" function.

Then next upgrade when we implement pool-specific governance, we automatically port the governor specification from this string.

Originally posted by @ValarDragon in #60 (comment)

Make create-pool.json fail if any unrecognized JSON field was included

Otherwise pool creators could silently fail, and potentially waste funds

Add Minimum Commission Rate

We should add a minimum commission rate that validators are allowed to set to avoid a race to the bottom for validators.

This will have to be done in https://github.com/osmosis-labs/cosmos-sdk

Change c-osmosis/osmosis everywhere in the code to osmosis-labs/osmosis

Add Liquidity Bootstrapping Pool

See https://docs.balancer.finance/guides/crp-tutorial/liquidity-bootstrapping

NOT a issue

But found you ❤

Check to confirm that PoolAccount addresses are ADR 028 compliant

If they aren't, we should either make them ADR 028 compliant, or remove the addresses for now.

Register basedenom and denoms

See example from crypto.com chain

https://github.com/crypto-org-chain/chain-main/blob/master/config/config.go#L20

GetRewardEst iterates all over the locked tokens

GetRewardsEst(which is called in RewardsEst grpc query) calls FilteredLocksDistributionEst, which iterates all over the locked tokens(https://github.com/osmosis-labs/osmosis/blob/main/x/incentives/keeper/pot.go#L164). This causes issues on performance side, as it need to recalculate the reward every time when the query happens.

The suggested solution is limit the lockup period to the epoch, and make the lockup module updates the total amount when there is lock related event happens. The incentive module will read the cached state when queried.

ref @Thunnini

Switch Addresses in proto files from bytes to string

In some of the modules' proto files, addresses are defined by bytes to be gogoproto casted to sdk.AccAddress

Example:

message MsgCreatePot {
  bool is_perpetual = 1; // flag to show if it's perpetual or multi-epoch
                         // distribution incentives by third party
  bytes owner = 2 [ (gogoproto.casttype) =
                        "github.com/cosmos/cosmos-sdk/types.AccAddress" ];

...

These should be switched to strings as this is the standard in the cosmos sdk now.

Perpetual pot for infinite rewards distribution per epoch

We need a way to keep a perpetual pot. Such a pot should distribute it's full balance every epoch, but the pot shouldn't disappear afterwards. This is what OSMO rewards will probably go into.

Like the weekly minted amount should go into a perpetual pot that distributes for 1 epoch. But new coins will be sent to this same pot every week until governance decides to vote to change it.

Setup CI Process

We should have a CI process to run tests, simulations, etc.

Use F1 based reward distribution for v1

Background

We initially delayed the idea of using an F1-based reward distribution mechanism for Osmosis due to concerns on the timeline, but I do think we can adopt the F1-based module without significantly delaying the launch schedule. I believe there are minimal benefits of an epoch-based system over the F1-based system, and only opens up spam attack risk vectors as well as instability in the block times due to the iterations required.

The x/farm module provides a much more sustainable and manageable architecture, and I've made progress on the implementation that I believe it will create no delays for launch. (Refer to the pool-yield branch)

Below is an outline of how the current architecture functions.

Farm Module

• Other modules create a farm
• Farm module doesn't custody the shares, but the rewards are managed(and custodied) by the farm module
• Farm module is created by an external module, but external accounts can provide additional incentives to farms that are already created
• Users can request the farm module for unclaimed rewards

Pool-Yield (to be changed to Pool Incentive) Module

• x/gamm afterPoolCreate hook created.
• Pool-yield module sets the lockableDurations at genesis (i.e. 1 week / 2 week / 4 week)
• Pool-yield module creates a farm (according to the number of lockableDurations) when the x/gamm afterPoolCreate is run
• lockup module
  • onTokenLocked - When the hook runs this, the pool-yield module checks if the locked token is the pool share token. If it is a pool share token, then it will check if the duration matches the lockableDurations. If both conditions are true, it will get the farm and add the farm share according to the locked poolShare amount. (Because the farm module doesn't custody the farm's share, the lockup module custodies the shares.)
  • onTokenUnlocked - This is the opposite process of onTokenLocked.
• External accounts can query the x/pool-yield module by specifying the poolId and lockableDurations and find the farmId.
• x/farm module supports external incentives, but the incentives must be added on a regular periodical basis for the duration that the additional incentives is active (i.e. reward added once per day).
• AddPoolIncentives proposal can be used to add records. The records consist of farm Id and weight. When the record is added, reward is allocated according to the record's farm ID. The reward amount equals to weight/ totalWeight.
  • To consider the potential the amount of farms/lockable duration increases, the pool-yield module uses an unnormalized weight since normalizing becomes fairly inconvenient when the number of farm becomes large.
• RemovePoolIncentives proposal is the opposite of the above proposal
• Because feeCollector manages the entire minted inflation in the same module account as the previous block's transaction fees are handled, and the distribution module take the entire remaining amount that's left in the feeCollector module account, the pool-yield module's begin block must be run between the mint module and the distr module's begin block.
• The pool-yield takes the mintedDenom according to the allocationRatio from the feeCollector module account then distributes the amount to farms according to the logic described above, then the distribution module uses the remaining amount.
• Issue (TODO):
  • Each module has a specific set of order that it takes its allocated amount in
  • The allocation taken by each designation (i.e. pool incentives, developer DAO, etc) is relative to the remaining amount
  • Therefore, setting the parameter is not very logical:
    • i.e. 100 OSMO minted, 50% liquidity incentives, 20% developer dao)
      1. liquidity incentives take the 50%
      2. Developer dao parameter must be set to 0.4(40%) because the liquidity incentives already took half, and only half is remaining (20% of total, but relatively 40% of the remaining minted amount)
    • feeCollector account manages the transaction fee and the inflation minted amount in the same account, and can't know which % is the transaction fee. Therefore, the transaction fees are distributed in the same allocationRatio as the minted inflation amount.
  • If the mint module can specify the amount that is to be allocated to pool-yield/developer-dao, then this should be able to be resolved.
    • ACTION ITEM → merge new mint module implementation to Osmosis-Labs repository
  • Create updatePoolIncentives proposal type.
  • Need to create more CLI and gRPC queries
  • DeveloperDAO (should be easy lift)

Discussion

• It'd be good for the lockup module to send tokens that have expired lockup at each block, rather than having each user to manually unlock their tokens. (Because the amount of lock tx that can be included in a block is limited, and the amount of unlock depends on the amount of lock, this alleviates concerns about spam)
  • Needed because the pool-yield module needs to remove expired lockup LP shares out of actively incentivized farms. and if this isn't done automatically, the pool-yield module needs to take care of this and having the lockup module handle this would simplify the design significantly while also making it easier for users.

Rename denormalized weight in x/gamm

Not sure whats the better name to be using, but it definitely shouldn't be denormalized. Denormal numbers are something very different in relevant contexts =) https://en.wikipedia.org/wiki/Denormal_number (Essentially an edge case of things with floats)

Refactor general math functions to be in our own osmo-math library

I think we will have a growing number of math operations that we will need high precision & fast implementations of. I think we should move such functions & related types into its own osmo-math package in the root directory. For now, Pow with fractional exponent would go in there.

Switch bech32 prefix to osmo instead of cosmos

Applying the `x/farm` logic to `x/gamm` module

I was hoping to apply the x/farm module to the x/gamm module as well. Currently, the x/gamm fee distribution mechanism accumulates the swap fees then gives the fee to the user upon claiming in proportion to the shares owned, so technically an LP who deposited later has some advantages if other users don’t claim their portion of the rewards as often. Therefore, I believe the F1 fee distribution mechanism is more reasonable so we should apply to the x/gamm module as well. Also, since there is no way to distribute the exit fee and the F1 distribution module will allow this as well.

However, this brings up another issue since each LP tokens have a different amount of fee reward accumulated, so unlocked LP tokens are technically not fungible with each other unless there is additional logic to stake the shares. Perhaps staking the share token upon minting by default could solve this. But the bigger issue that I see is that when the pool’s share tokens are staked to the pool, the lockup module isn’t able to be used. One option is to change the lockup module’s architecture, as going with the current implementation will force users to choose whether to receive the pool rewards or the Osmo farming rewards.

Pros:

• Fee distribution is more equitable
• Exit fee distribution can happen

Cons:

• Requires a stake/unstake feature to be added to the pool shares
• Need to come up with a different mechanism to distribute the exit fee
• Not compatible with the current lockup module

Split up Lock parameter into LockLiquidityProviding and MinimumLiquidity

We want at least two types of pool locking for our Balancer pool type. (Following adapted from @sunnya97 )

• Lockable Liquidity Providing, because you might want a private pool where only one person can be the LP (i.e. for a LBP perhaps).
• Lockable Trading, because you might want to have constraints on things like “minimum liquidity” in order for trading to be possible. This would give slippage bounds.

Currently we only have one Lock boolean, which locks just anyone from adding or removing liquidity.

I believe lockable liquidity providing is captured by a permission for "can anyone add liquidity", with some ability to specify exceptions to this rule.

Lockable trading probably should be up to the AMM to decide conditions of. Namely, a natural one is "trading is locked until liquidity of asset A in the pool is > x".

We should revisit enabling capabilities for this after minimum viable osmosis.

Chain-wide epochs

Currently, a number of modules use epochs for different purposes (i.e. incentives uses it to determine when to distribute coins from pots, mint uses it to determine how often to create newly issued coins, etc). In the future, we may want more stuff to use epochs as well (i.e. epoched staking, distribution).

All these different epochs could get quite annoying I think, especially if they are different lengths or offset from each other.

Should we combine all of these epochs into one epochs module, that others can import? This new module can have a hook called EpochEnd(ctx, epochNum), that these other modules can connect to.

This will allow a single global epoch system that I think will make things easier for end users to understand.

Is there any reason why some of these modules will need a bespoke epoch length?

Rename Record to PoolAssetType

Record in the pools currently refers to an (asset_denom, weight) pair. I believe the name record is fairly misleading for this.

I suggest we rename Record to PoolAssetType. (Or perhaps a better name if anyone can think of one)

Upgrade to Cosmos SDK v0.40.0

Upgrade to final release of Stargate

Finish adding events for x/gamm

Currently, only the TypeEvtUpdateSwapFee and the TypeEvtPoolCreated events are being emitted. Need to emit events for the other x/gamm actions as well

Add consistency tests for marshalling to/from all of our proto types

We should add tests for marshalling to/from all of our proto types, both just as a consistency test, and because alot of our types code coverage just looks abysmal without it 😬. (its at 5% in x/gamm/types)

gamm CLI query still uses name records instead of pool-assets

Optimize `powApprox`

Currently powApprox under gamm/keeper/math.go is using a lot of resources. BenchmarkPow() test will measure the performance of the function. I’m ran 10 small pows on my MBP(2018/2.3GHz 8 Core i9/16GB RAM) and this was the result I got:

go test -bench . -benchmem -cpu 1,2,4,8,16

goos: darwin
goarch: amd64
pkg: github.com/c-osmosis/osmosis/x/gamm/keeper
BenchmarkPow                  25          51252602 ns/op        30098473 B/op     898416 allocs/op
BenchmarkPow-2                26          46804606 ns/op        30099396 B/op     898423 allocs/op
BenchmarkPow-4                24          46385548 ns/op        30100596 B/op     898430 allocs/op
BenchmarkPow-8                24          45494231 ns/op        30102812 B/op     898441 allocs/op
BenchmarkPow-16               25          45395677 ns/op        30106263 B/op     898449 allocs/op
PASS

Because multithreading is not used, the number of CPU cores shouldn’t matter, but it does require a lot of memory space and allocation.

Currently, the function is used to find a rational number exponent of a rational number. However, this seems to require too many loops so speed and memory becomes an issue.

The current implementation uses an expanded recursion function to find the binomial series so using multithreading will be difficult.

As for the memory, Cosmos-SDK’s sdk.Dec is immutable so it’s safer to use, but requires memory allocation every single time so it uses a lot of memory. To run some tests on the benefits, I’ve created a temporary mutable dec type under the gamm-math-perf branch.

The following are the results:

goos: darwin
goarch: amd64
pkg: github.com/c-osmosis/osmosis/x/gamm/keeper
BenchmarkPow                  40          29956045 ns/op         3770156 B/op     171524 allocs/op
BenchmarkPow-2                39          28767250 ns/op         3770280 B/op     171525 allocs/op
BenchmarkPow-4                40          28282755 ns/op         3770435 B/op     171526 allocs/op
BenchmarkPow-8                40          28204827 ns/op         3770722 B/op     171527 allocs/op
BenchmarkPow-16               40          28658882 ns/op         3771151 B/op     171528 allocs/op
PASS

It seems like memory allocation and memory use has definitely decreased. So the speed is a bit faster. But using a mutable dec will require extra caution, and because it’s different from Cosmos-SDK’s sdk.Dec each must be managed separately and must be well checked for bugs.

Pros:

• Memory usage, memory allocation decreased. Improved performance in calculation.

Cons:

• Must use a separate mutable dec, and not sdk.Dec -> may have bugs.

Run linter in CI

See make lint for what command we should be running

Switch Incentives Module epochs to be time-based

question about native zone token

what would be the different between staking with the LP token vs the native zone token by itself? Could this same configuration be for the hub itself?

Add claims process

Initial fairdrop coins need to go through claim process of participating in the network

SwapFee used in SpotPrice calculation

@Thunnini why does the spot price calculation include the swapfee? I don't believe this is standard when describing spot price of AMMs.

osmosis/x/gamm/keeper/math.go

Line 23 in a003c11

scale := sdk.OneDec().Quo(sdk.OneDec().Sub(swapFee))

InitialPoolWeights and TargetPoolWeights should not be []PoolAsset

The types for InitialPoolWeights and for TargetPoolWeights in the SmoothWeightChangeParams are both []PoolAsset

type SmoothWeightChangeParams struct {
	StartTime time.Time
	Duration time.Duration
	InitialPoolWeights []PoolAsset
	TargetPoolWeights []PoolAsset
}

But PoolAsset is defined as

type PoolAsset struct {
	Token sdk.Coin
	Weight sdk.Int 
}

But this doesn’t make sense because sdk.Coin includes an amount, but all we want for InitialPoolWeights and TargetPoolWeights is a denom and a weight

I think this comes down to the PoolAsset abstraction just doesn’t make sense

Why are we trying to combine the balance of a pool into the same struct as the weights of the assets? Just to save on storing the denom twice?

A pool’s balances should just be sdk.Coins, and the pool’s weights could be also sdk.Coins, but might make sense to just alias that to a new type called poolWeights (for type safety)

CalculateSpotPrice should be fee-less

Currently we have two functions: CalculateSpotPrice and CalculateSpotPriceSansFee.

I know, this is using the naming used in the Balancer code, but I think this nomenclature of SpotPrice including fees is quite non-standard in industry and we should it correctly. (Even the balancer docs, define spot price as not including fees: https://docs.balancer.finance/core-concepts/protocol/index#spot-price).

I'd recommend switching:

• CalculateSpotPriceSansFee -> CalculateSpotPrice
• CalculateSpotPrice -> CalculateSpotPriceWithFee.