Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Twap: Add twapkeeper wiring #2193

Merged
merged 2 commits into from
Jul 22, 2022
Merged

Twap: Add twapkeeper wiring #2193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
155638c
Twap: Add twapkeeper wiring
ValarDragon Jul 22, 2022
3aac1b2
Add changelog entry
ValarDragon Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

* [#2016](https://github.com/osmosis-labs/osmosis/pull/2016) Add fixed 10000 gas cost for each Balancer swap
* [$2147](https://github.com/osmosis-labs/osmosis/pull/2147) Set MaxAgeNumBlocks in v11 Upgrade Handler to two weeks.

### Breaking Changes


Expand All @@ -54,6 +51,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#1825](https://github.com/osmosis-labs/osmosis/pull/1825) Fixes Interchain Accounts (host side) by adding it to AppModuleBasics
* [#1699](https://github.com/osmosis-labs/osmosis/pull/1699) Fixes bug in sig fig rounding on spot price queries for small values
* [#1994](https://github.com/osmosis-labs/osmosis/pull/1994) Removed bech32ibc module
* [#2016](https://github.com/osmosis-labs/osmosis/pull/2016) Add fixed 10000 gas cost for each Balancer swap
* [#2147](https://github.com/osmosis-labs/osmosis/pull/2147) Set MaxAgeNumBlocks in v11 Upgrade Handler to two weeks.
* [#2193](https://github.com/osmosis-labs/osmosis/pull/2193) Add TwapKeeper to the Osmosis app

#### Golang API breaks

Expand All @@ -66,7 +66,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#1630](https://github.com/osmosis-labs/osmosis/pull/1630) Delete the v043_temp module, now that we're on an updated SDK version.
* [#1667](https://github.com/osmosis-labs/osmosis/pull/1673) Move wasm-bindings code out of app package into its own root level package.
* [#2013](https://github.com/osmosis-labs/osmosis/pull/2013) Make `SetParams`, `SetPool`, `SetTotalLiquidity`, and `SetDenomLiquidity` GAMM APIs private
*[#1857](https://github.com/osmosis-labs/osmosis/pull/1857) x/mint rename GetLastHalvenEpochNum to GetLastReductionEpochNum
* [#1857](https://github.com/osmosis-labs/osmosis/pull/1857) x/mint rename GetLastHalvenEpochNum to GetLastReductionEpochNum

### Features

Expand Down
18 changes: 18 additions & 0 deletions app/apptesting/gamm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,21 @@ func (s *KeeperTestHelper) PrepareBalancerPoolWithPoolAsset(assets []balancer.Po
s.NoError(err)
return poolId
}

func (s *KeeperTestHelper) RunBasicSwap(poolId uint64) {
denoms, err := s.App.GAMMKeeper.GetPoolDenoms(s.Ctx, poolId)
s.Require().NoError(err)

swapIn := sdk.NewCoins(sdk.NewCoin(denoms[0], sdk.NewInt(1000)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be useful to have these as parameters so that the caller can customize them?

I find that frequently in GAMM tests, we have to hard code some parameters because such test helpers are not customizable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could let it take MsgSwapExactAmountIn for full control over how the caller wants the swap to behave?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this one was to not have to think about anything, I think a second helper that does that would be helpful

s.FundAcc(s.TestAccs[0], swapIn)

msg := gammtypes.MsgSwapExactAmountIn{
Sender: string(s.TestAccs[0]),
Routes: []gammtypes.SwapAmountInRoute{{PoolId: poolId, TokenOutDenom: denoms[1]}},
TokenIn: swapIn[0],
TokenOutMinAmount: sdk.ZeroInt(),
}
// TODO: switch to message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's create an issue for this please

_, err = s.App.GAMMKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], poolId, msg.TokenIn, denoms[1], msg.TokenOutMinAmount)
s.Require().NoError(err)
}
10 changes: 10 additions & 0 deletions app/apptesting/test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,16 @@ func (s *KeeperTestHelper) CreateTestContext() sdk.Context {
return sdk.NewContext(ms, tmtypes.Header{}, false, logger)
}

// CreateTestContext creates a test context.
func (s *KeeperTestHelper) Commit() {
oldHeight := s.Ctx.BlockHeight()
oldHeader := s.Ctx.BlockHeader()
s.App.Commit()
newHeader := tmtypes.Header{Height: oldHeight + 1, ChainID: oldHeader.ChainID, Time: time.Now().UTC()}
s.App.BeginBlock(abci.RequestBeginBlock{Header: newHeader})
s.Ctx = s.App.GetBaseApp().NewContext(false, newHeader)
}

// FundAcc funds target address with specified amount.
func (s *KeeperTestHelper) FundAcc(acc sdk.AccAddress, amounts sdk.Coins) {
err := simapp.FundAccount(s.App.BankKeeper, s.Ctx, acc, amounts)
Expand Down
11 changes: 11 additions & 0 deletions app/keepers/keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ import (
epochskeeper "github.com/osmosis-labs/osmosis/v10/x/epochs/keeper"
epochstypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
gammkeeper "github.com/osmosis-labs/osmosis/v10/x/gamm/keeper"
"github.com/osmosis-labs/osmosis/v10/x/gamm/twap"
twaptypes "github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
gammtypes "github.com/osmosis-labs/osmosis/v10/x/gamm/types"
incentiveskeeper "github.com/osmosis-labs/osmosis/v10/x/incentives/keeper"
incentivestypes "github.com/osmosis-labs/osmosis/v10/x/incentives/types"
Expand Down Expand Up @@ -98,6 +100,7 @@ type AppKeepers struct {
TransferKeeper *ibctransferkeeper.Keeper
EvidenceKeeper *evidencekeeper.Keeper
GAMMKeeper *gammkeeper.Keeper
TwapKeeper *twap.Keeper
LockupKeeper *lockupkeeper.Keeper
EpochsKeeper *epochskeeper.Keeper
IncentivesKeeper *incentiveskeeper.Keeper
Expand Down Expand Up @@ -241,6 +244,12 @@ func (appKeepers *AppKeepers) InitNormalKeepers(
appKeepers.AccountKeeper, appKeepers.BankKeeper, appKeepers.DistrKeeper)
appKeepers.GAMMKeeper = &gammKeeper

appKeepers.TwapKeeper = twap.NewKeeper(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking to learn - since TWAP has its own keeper why would we not have a separate module for it? Isn't this inconsistent with the cosmos-sdk conventions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can move it to its own module, the way its written this is really just a folder move

appKeepers.keys[twaptypes.StoreKey],
appKeepers.tkeys[twaptypes.TransientStoreKey],
appKeepers.GetSubspace(twaptypes.ModuleName),
appKeepers.GAMMKeeper)

appKeepers.LockupKeeper = lockupkeeper.NewKeeper(
appCodec,
appKeepers.keys[lockuptypes.StoreKey],
Expand Down Expand Up @@ -454,6 +463,7 @@ func (appKeepers *AppKeepers) SetupHooks() {
gammtypes.NewMultiGammHooks(
// insert gamm hooks receivers here
appKeepers.PoolIncentivesKeeper.Hooks(),
appKeepers.TwapKeeper.GammHooks(),
),
)

Expand Down Expand Up @@ -512,6 +522,7 @@ func KVStoreKeys() []string {
ibctransfertypes.StoreKey,
capabilitytypes.StoreKey,
gammtypes.StoreKey,
twaptypes.StoreKey,
lockuptypes.StoreKey,
incentivestypes.StoreKey,
epochstypes.StoreKey,
Expand Down
4 changes: 3 additions & 1 deletion app/keepers/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"

twaptypes "github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
)

// GenerateKeys generates new keys (KV Store, Transient store, and memory store).
Expand All @@ -13,7 +15,7 @@ func (appKeepers *AppKeepers) GenerateKeys() {
appKeepers.keys = sdk.NewKVStoreKeys(KVStoreKeys()...)

// Define transient store keys
appKeepers.tkeys = sdk.NewTransientStoreKeys(paramstypes.TStoreKey)
appKeepers.tkeys = sdk.NewTransientStoreKeys(paramstypes.TStoreKey, twaptypes.TransientStoreKey)

// MemKeys are for information that is stored only in RAM.
appKeepers.memKeys = sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey)
Expand Down
2 changes: 2 additions & 0 deletions app/keepers/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
_ "github.com/osmosis-labs/osmosis/v10/client/docs/statik"
"github.com/osmosis-labs/osmosis/v10/x/epochs"
"github.com/osmosis-labs/osmosis/v10/x/gamm"
"github.com/osmosis-labs/osmosis/v10/x/gamm/twap"
"github.com/osmosis-labs/osmosis/v10/x/incentives"
"github.com/osmosis-labs/osmosis/v10/x/lockup"
"github.com/osmosis-labs/osmosis/v10/x/mint"
Expand Down Expand Up @@ -74,6 +75,7 @@ var AppModuleBasics = []module.AppModuleBasic{
transfer.AppModuleBasic{},
vesting.AppModuleBasic{},
gamm.AppModuleBasic{},
twap.AppModuleBasic{},
txfees.AppModuleBasic{},
incentives.AppModuleBasic{},
lockup.AppModuleBasic{},
Expand Down
6 changes: 5 additions & 1 deletion app/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import (
"github.com/osmosis-labs/osmosis/v10/x/epochs"
epochstypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
"github.com/osmosis-labs/osmosis/v10/x/gamm"
"github.com/osmosis-labs/osmosis/v10/x/gamm/twap"
twaptypes "github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
gammtypes "github.com/osmosis-labs/osmosis/v10/x/gamm/types"
"github.com/osmosis-labs/osmosis/v10/x/incentives"
incentivestypes "github.com/osmosis-labs/osmosis/v10/x/incentives/types"
Expand Down Expand Up @@ -121,6 +123,7 @@ func appModules(
params.NewAppModule(*app.ParamsKeeper),
app.TransferModule,
gamm.NewAppModule(appCodec, *app.GAMMKeeper, app.AccountKeeper, app.BankKeeper),
twap.NewAppModule(*app.TwapKeeper),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something cool is that with the structure of keeper and module.go being defined in the same file, no other keepers need to be passed in.

Also AppCodec isn't needed in modules that don't serialize interfaces

txfees.NewAppModule(appCodec, *app.TxFeesKeeper),
incentives.NewAppModule(appCodec, *app.IncentivesKeeper, app.AccountKeeper, app.BankKeeper, app.EpochsKeeper),
lockup.NewAppModule(appCodec, *app.LockupKeeper, app.AccountKeeper, app.BankKeeper),
Expand Down Expand Up @@ -165,7 +168,7 @@ func orderBeginBlockers(allModuleNames []string) []string {
// OrderEndBlockers returns EndBlockers (crisis, govtypes, staking) with no relative order.
func OrderEndBlockers(allModuleNames []string) []string {
ord := partialord.NewPartialOrdering(allModuleNames)
// only Osmosis modules with endblock code are: crisis, govtypes, staking
// only Osmosis modules with endblock code are: twap, crisis, govtypes, staking
// we don't care about the relative ordering between them.
return ord.TotalOrdering()
}
Expand All @@ -190,6 +193,7 @@ func OrderInitGenesis(allModuleNames []string) []string {
ibchost.ModuleName,
icatypes.ModuleName,
gammtypes.ModuleName,
twaptypes.ModuleName,
txfeestypes.ModuleName,
genutiltypes.ModuleName,
evidencetypes.ModuleName,
Expand Down
6 changes: 5 additions & 1 deletion app/upgrades/v11/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v11

import (
"github.com/osmosis-labs/osmosis/v10/app/upgrades"
twaptypes "github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"

store "github.com/cosmos/cosmos-sdk/store/types"
)
Expand All @@ -12,5 +13,8 @@ const UpgradeName = "v11"
var Upgrade = upgrades.Upgrade{
UpgradeName: UpgradeName,
CreateUpgradeHandler: CreateUpgradeHandler,
StoreUpgrades: store.StoreUpgrades{},
StoreUpgrades: store.StoreUpgrades{
Added: []string{twaptypes.StoreKey},
Deleted: []string{}, // double check bech32ibc
},
}
7 changes: 7 additions & 0 deletions x/gamm/keeper/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/osmosis-labs/osmosis/v10/osmoutils"
"github.com/osmosis-labs/osmosis/v10/x/gamm/pool-models/balancer"
"github.com/osmosis-labs/osmosis/v10/x/gamm/pool-models/stableswap"
"github.com/osmosis-labs/osmosis/v10/x/gamm/types"
Expand Down Expand Up @@ -197,6 +198,12 @@ func (k Keeper) DeletePool(ctx sdk.Context, poolId uint64) error {
// return nil
// }

func (k Keeper) GetPoolDenoms(ctx sdk.Context, poolId uint64) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, let's make a test for this please with a success and an error case

pool, err := k.GetPoolAndPoke(ctx, poolId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err check

denoms := osmoutils.CoinsDenoms(pool.GetTotalPoolLiquidity(ctx))
return denoms, err
}

// setNextPoolNumber sets next pool number.
func (k Keeper) setNextPoolNumber(ctx sdk.Context, poolNumber uint64) {
store := ctx.KVStore(k.storeKey)
Expand Down
13 changes: 0 additions & 13 deletions x/gamm/twap/abci.go
View file
Open in desktop

This file was deleted.

31 changes: 27 additions & 4 deletions x/gamm/twap/hook_listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,52 @@
package twap

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

epochtypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
"github.com/osmosis-labs/osmosis/v10/x/gamm/types"
)

var _ types.GammHooks = &gammhook{}
var _ epochtypes.EpochHooks = &epochhook{}

type epochhook struct {
k Keeper
}

func (hook *epochhook) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
if epochIdentifier == hook.k.PruneEpochIdentifier(ctx) {
fmt.Println("restore logic in subsequent PR")
// hook.k.pruneOldTwaps(ctx)
}
}

func (hook *epochhook) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {}

type gammhook struct {
k twapkeeper
k Keeper
}

func (k Keeper) GammHooks() types.GammHooks {
return &gammhook{k}
}

// AfterPoolCreated is called after CreatePool
func (hook *gammhook) AfterPoolCreated(ctx sdk.Context, sender sdk.AccAddress, poolId uint64) {
// TODO: Log pool creation to begin creating TWAPs for it
// err := hook.k.afterCreatePool(ctx, poolId)
// // Will halt pool creation
// if err != nil {
// panic(err)
// }
}

func (hook *gammhook) AfterSwap(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, input sdk.Coins, output sdk.Coins) {
// Log that this pool had a potential spot price change
hook.k.trackChangedPool(ctx, poolId)
}

func (hook *gammhook) AfterJoinPool(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, enterCoins sdk.Coins, shareOutAmount sdk.Int) {
// Log that this pool had a potential spot price change
hook.k.trackChangedPool(ctx, poolId)
}

Expand Down
27 changes: 23 additions & 4 deletions x/gamm/twap/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
package twap

import sdk "github.com/cosmos/cosmos-sdk/types"
import (
sdk "github.com/cosmos/cosmos-sdk/types"

type twapkeeper struct {
// storeKey sdk.StoreKey
transientKey sdk.TransientStoreKey
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"

"github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
)

type Keeper struct {
storeKey sdk.StoreKey
transientKey *sdk.TransientStoreKey

paramSpace paramtypes.Subspace

ammkeeper types.AmmInterface
}

func NewKeeper(storeKey sdk.StoreKey, transientKey *sdk.TransientStoreKey, paramSpace paramtypes.Subspace, ammKeeper types.AmmInterface) *Keeper {
return &Keeper{storeKey: storeKey, transientKey: transientKey, paramSpace: paramSpace, ammkeeper: ammKeeper}
}

// TODO: make this read from a parameter, or hardcode it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make an issue please

func (k *Keeper) PruneEpochIdentifier(ctx sdk.Context) string {
return "daily"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be "day" and not "daily" according to the epochs parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh woops, yeah it should

}
Loading