diff --git a/app/apptesting/gamm.go b/app/apptesting/gamm.go index 13ff202e4d7..90e07b1f7e2 100644 --- a/app/apptesting/gamm.go +++ b/app/apptesting/gamm.go @@ -111,3 +111,25 @@ func (s *KeeperTestHelper) RunBasicSwap(poolId uint64) { _, err = s.App.GAMMKeeper.SwapExactAmountIn(s.Ctx, s.TestAccs[0], poolId, msg.TokenIn, denoms[1], msg.TokenOutMinAmount) s.Require().NoError(err) } + +func (s *KeeperTestHelper) RunBasicJoin(poolId uint64) { + pool, _ := s.App.GAMMKeeper.GetPoolAndPoke(s.Ctx, poolId) + denoms, err := s.App.GAMMKeeper.GetPoolDenoms(s.Ctx, poolId) + s.Require().NoError(err) + + tokenIn := sdk.NewCoins(sdk.NewCoin(denoms[0], sdk.NewInt(1000)), sdk.NewCoin(denoms[1], sdk.NewInt(1000))) + s.FundAcc(s.TestAccs[0], tokenIn) + + minShareOutAmt, _, err := pool.CalcJoinPoolShares(s.Ctx, tokenIn, pool.GetSwapFee(s.Ctx)) + s.Require().NoError(err) + + msg := gammtypes.MsgJoinPool{ + Sender: string(s.TestAccs[0]), + PoolId: poolId, + ShareOutAmount: minShareOutAmt, + TokenInMaxs: tokenIn, + } + // TODO: switch to message + _, _, err = s.App.GAMMKeeper.JoinPoolNoSwap(s.Ctx, s.TestAccs[0], poolId, msg.ShareOutAmount, msg.TokenInMaxs) + s.Require().NoError(err) +} diff --git a/x/twap/README.md b/x/twap/README.md index dfd38cc5859..d7c5da6a7e1 100644 --- a/x/twap/README.md +++ b/x/twap/README.md @@ -84,6 +84,7 @@ We maintain TWAP accumulation records for every AMM pool on Osmosis. Because Osmosis supports multi-asset pools, a complicating factor is that we have to store a record for every asset pair in the pool. For every pool, at a given point in time, we make one twap record entry per unique pair of denoms in the pool. If a pool has `k` denoms, the number of unique pairs is `k * (k - 1) / 2`. +All public API's for the module will sort the input denoms to the canonical representation, so the caller does not need to worry about this. (The canonical representation is the denoms in lexicographical order) Each twap record stores [(source)](https://github.com/osmosis-labs/osmosis/tree/main/proto/osmosis/gamm/twap): * last spot price of base asset A in terms of quote asset B diff --git a/x/twap/api.go b/x/twap/api.go index 09cb71eddd3..8e5d8b3d5e2 100644 --- a/x/twap/api.go +++ b/x/twap/api.go @@ -83,9 +83,5 @@ func (k Keeper) GetArithmeticTwapToNow( // these swaps have had no time to be arbitraged back. // This accumulator can be stored, to compute wider ranged twaps. func (k Keeper) GetBeginBlockAccumulatorRecord(ctx sdk.Context, poolId uint64, asset0Denom string, asset1Denom string) (types.TwapRecord, error) { - // correct ordering of args for db - if asset1Denom > asset0Denom { - asset0Denom, asset1Denom = asset1Denom, asset0Denom - } return k.getMostRecentRecord(ctx, poolId, asset0Denom, asset1Denom) } diff --git a/x/twap/api_test.go b/x/twap/api_test.go index 660b8ea8e01..e818551e097 100644 --- a/x/twap/api_test.go +++ b/x/twap/api_test.go @@ -1,6 +1,7 @@ package twap_test import ( + "fmt" "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -27,16 +28,18 @@ func (s *TestSuite) TestGetBeginBlockAccumulatorRecord() { poolId uint64 quoteDenom string baseDenom string - expError bool + expError error }{ - "no record (wrong pool ID)": {initStartRecord, initStartRecord, baseTime, 4, denomA, denomB, true}, - "default record": {initStartRecord, initStartRecord, baseTime, 1, denomA, denomB, false}, - "one second later record": {initStartRecord, recordWithUpdatedAccum(initStartRecord, OneSec, OneSec), tPlusOne, 1, denomA, denomB, false}, - "idempotent overwrite": {initStartRecord, initStartRecord, baseTime, 1, denomA, denomB, false}, - "idempotent overwrite2": {initStartRecord, recordWithUpdatedAccum(initStartRecord, OneSec, OneSec), tPlusOne, 1, denomA, denomB, false}, + "no record (wrong pool ID)": {initStartRecord, initStartRecord, baseTime, 4, denomA, denomB, fmt.Errorf("twap not found")}, + "default record": {initStartRecord, initStartRecord, baseTime, 1, denomA, denomB, nil}, + "default record but same denom": {initStartRecord, initStartRecord, baseTime, 1, denomA, denomA, fmt.Errorf("both assets cannot be of the same denom: assetA: %s, assetB: %s", denomA, denomA)}, + "default record wrong order (should get reordered)": {initStartRecord, initStartRecord, baseTime, 1, denomB, denomA, nil}, + "one second later record": {initStartRecord, recordWithUpdatedAccum(initStartRecord, OneSec, OneSec), tPlusOne, 1, denomA, denomB, nil}, + "idempotent overwrite": {initStartRecord, initStartRecord, baseTime, 1, denomA, denomB, nil}, + "idempotent overwrite2": {initStartRecord, recordWithUpdatedAccum(initStartRecord, OneSec, OneSec), tPlusOne, 1, denomA, denomB, nil}, "diff spot price": {zeroAccumTenPoint1Record, recordWithUpdatedAccum(zeroAccumTenPoint1Record, OneSec.MulInt64(10), OneSec.QuoInt64(10)), - tPlusOne, 1, denomA, denomB, false}, + tPlusOne, 1, denomA, denomB, nil}, // TODO: Overflow } for name, tc := range tests { @@ -49,10 +52,14 @@ func (s *TestSuite) TestGetBeginBlockAccumulatorRecord() { actualRecord, err := s.twapkeeper.GetBeginBlockAccumulatorRecord(s.Ctx, tc.poolId, tc.baseDenom, tc.quoteDenom) - if tc.expError { - s.Require().Error(err) + if tc.expError != nil { + s.Require().Equal(tc.expError, err) return } + + // ensure denom order was corrected + s.Require().True(actualRecord.Asset0Denom < actualRecord.Asset1Denom) + s.Require().NoError(err) s.Require().Equal(tc.expRecord, actualRecord) }) diff --git a/x/twap/keeper_test.go b/x/twap/keeper_test.go index 61c870cf37b..372a0cfcaa5 100644 --- a/x/twap/keeper_test.go +++ b/x/twap/keeper_test.go @@ -16,14 +16,14 @@ import ( // TODO: Consider switching this everywhere var ( - denom0 = "token/B" - denom1 = "token/A" - denom2 = "token/C" - defaultUniV2Coins = sdk.NewCoins(sdk.NewInt64Coin(denom0, 1_000_000_000), sdk.NewInt64Coin(denom1, 1_000_000_000)) - defaultThreeAssetCoins = sdk.NewCoins(sdk.NewInt64Coin(denom0, 1_000_000_000), sdk.NewInt64Coin(denom1, 1_000_000_000), sdk.NewInt64Coin(denom2, 1_000_000_000)) - baseTime = time.Unix(1257894000, 0).UTC() - tPlusOne = baseTime.Add(time.Second) - basePoolId uint64 = 1 + denom0 = "token/A" + denom1 = "token/B" + denom2 = "token/C" + defaultUniV2Coins = sdk.NewCoins(sdk.NewInt64Coin(denom0, 1_000_000_000), sdk.NewInt64Coin(denom1, 1_000_000_000)) + defaultThreeAssetCoins = sdk.NewCoins(sdk.NewInt64Coin(denom0, 1_000_000_000), sdk.NewInt64Coin(denom1, 1_000_000_000), sdk.NewInt64Coin(denom2, 1_000_000_000)) + baseTime = time.Unix(1257894000, 0).UTC() + tPlusOne = baseTime.Add(time.Second) + basePoolId uint64 = 1 ) type TestSuite struct { @@ -277,7 +277,7 @@ func (suite *TestSuite) TestTWAPExportGenesis() { // sets up a new two asset pool, with spot price 1 func (s *TestSuite) setupDefaultPool() (poolId uint64, denomA, denomB string) { poolId = s.PrepareBalancerPoolWithCoins(defaultUniV2Coins[0], defaultUniV2Coins[1]) - denomA, denomB = defaultUniV2Coins[1].Denom, defaultUniV2Coins[0].Denom + denomA, denomB = defaultUniV2Coins[0].Denom, defaultUniV2Coins[1].Denom return } diff --git a/x/twap/listeners_test.go b/x/twap/listeners_test.go index dc59794b721..e42e4cf6abd 100644 --- a/x/twap/listeners_test.go +++ b/x/twap/listeners_test.go @@ -6,6 +6,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/osmosis-labs/osmosis/v11/osmoutils" + "github.com/osmosis-labs/osmosis/v11/x/twap" "github.com/osmosis-labs/osmosis/v11/x/twap/types" ) @@ -49,7 +50,7 @@ func (s *TestSuite) TestAfterPoolCreatedHook() { denomPairs0, denomPairs1 := types.GetAllUniqueDenomPairs(denoms) expectedRecords := []types.TwapRecord{} for i := 0; i < len(denomPairs0); i++ { - expectedRecord, err := types.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denomPairs0[i], denomPairs1[i]) + expectedRecord, err := twap.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denomPairs0[i], denomPairs1[i]) s.Require().NoError(err) expectedRecords = append(expectedRecords, expectedRecord) } @@ -86,6 +87,15 @@ func (s *TestSuite) TestSwapTriggeringTrackPoolId() { s.Require().Equal([]uint64{poolId}, s.twapkeeper.GetChangedPools(s.Ctx)) } +// Tests that after a swap, we are triggering internal tracking logic for a pool. +func (s *TestSuite) TestJoinTriggeringTrackPoolId() { + poolId := s.PrepareBalancerPoolWithCoins(defaultUniV2Coins...) + s.BeginNewBlock(false) + s.RunBasicJoin(poolId) + + s.Require().Equal([]uint64{poolId}, s.twapkeeper.GetChangedPools(s.Ctx)) +} + // TestSwapAndEndBlockTriggeringSave tests that if we: // * create a pool in block 1 // * swap in block 2 @@ -96,7 +106,7 @@ func (s *TestSuite) TestSwapAndEndBlockTriggeringSave() { s.Ctx = s.Ctx.WithBlockTime(baseTime) poolId := s.PrepareBalancerPoolWithCoins(defaultUniV2Coins...) - expectedHistoricalTwap, err := types.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denom0, denom1) + expectedHistoricalTwap, err := twap.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denom0, denom1) s.Require().NoError(err) s.EndBlock() @@ -106,7 +116,7 @@ func (s *TestSuite) TestSwapAndEndBlockTriggeringSave() { s.RunBasicSwap(poolId) // accumulators are default right here - expectedLatestTwapUpToAccum, err := types.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denom0, denom1) + expectedLatestTwapUpToAccum, err := twap.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denom0, denom1) s.Require().NoError(err) // ensure different spot prices s.Require().NotEqual(expectedHistoricalTwap.P0LastSpotPrice, expectedLatestTwapUpToAccum.P0LastSpotPrice) diff --git a/x/twap/logic.go b/x/twap/logic.go index eba0b0e4cbc..7d955a1142e 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -11,12 +11,32 @@ import ( // TODO: configure recordHistoryKeepPeriod via parameter. const recordHistoryKeepPeriod = 48 * time.Hour +func NewTwapRecord(k types.AmmInterface, ctx sdk.Context, poolId uint64, denom0, denom1 string) (types.TwapRecord, error) { + denom0, denom1, err := types.LexicographicalOrderDenoms(denom0, denom1) + if err != nil { + return types.TwapRecord{}, err + } + sp0 := types.MustGetSpotPrice(k, ctx, poolId, denom0, denom1) + sp1 := types.MustGetSpotPrice(k, ctx, poolId, denom1, denom0) + return types.TwapRecord{ + PoolId: poolId, + Asset0Denom: denom0, + Asset1Denom: denom1, + Height: ctx.BlockHeight(), + Time: ctx.BlockTime(), + P0LastSpotPrice: sp0, + P1LastSpotPrice: sp1, + P0ArithmeticTwapAccumulator: sdk.ZeroDec(), + P1ArithmeticTwapAccumulator: sdk.ZeroDec(), + }, nil +} + // afterCreatePool creates new twap records of all the unique pairs of denoms within a pool. func (k Keeper) afterCreatePool(ctx sdk.Context, poolId uint64) error { denoms, err := k.ammkeeper.GetPoolDenoms(ctx, poolId) denomPairs0, denomPairs1 := types.GetAllUniqueDenomPairs(denoms) for i := 0; i < len(denomPairs0); i++ { - record, err := types.NewTwapRecord(k.ammkeeper, ctx, poolId, denomPairs0[i], denomPairs1[i]) + record, err := NewTwapRecord(k.ammkeeper, ctx, poolId, denomPairs0[i], denomPairs1[i]) // err should be impossible given GetAllUniqueDenomPairs guarantees if err != nil { return err @@ -107,8 +127,9 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t // This is achieved by getting the record `r` that is at, or immediately preceding in state time `t`. // To be clear: the record r s.t. `t - r.Time` is minimized AND `t >= r.Time` func (k Keeper) getInterpolatedRecord(ctx sdk.Context, poolId uint64, t time.Time, assetA, assetB string) (types.TwapRecord, error) { - if !(assetA > assetB) { - assetA, assetB = assetB, assetA + assetA, assetB, err := types.LexicographicalOrderDenoms(assetA, assetB) + if err != nil { + return types.TwapRecord{}, err } record, err := k.getRecordAtOrBeforeTime(ctx, poolId, t, assetA, assetB) if err != nil { @@ -119,8 +140,9 @@ func (k Keeper) getInterpolatedRecord(ctx sdk.Context, poolId uint64, t time.Tim } func (k Keeper) getMostRecentRecord(ctx sdk.Context, poolId uint64, assetA, assetB string) (types.TwapRecord, error) { - if !(assetA > assetB) { - assetA, assetB = assetB, assetA + assetA, assetB, err := types.LexicographicalOrderDenoms(assetA, assetB) + if err != nil { + return types.TwapRecord{}, err } record, err := k.getMostRecentRecordStoreRepresentation(ctx, poolId, assetA, assetB) if err != nil { diff --git a/x/twap/store.go b/x/twap/store.go index ece01cf5f40..abcd654978d 100644 --- a/x/twap/store.go +++ b/x/twap/store.go @@ -78,6 +78,10 @@ func (k Keeper) deleteHistoricalRecord(ctx sdk.Context, twap types.TwapRecord) { // interpolated to the current block time, or after events in this block. // Neither of which apply to the record this returns. func (k Keeper) getMostRecentRecordStoreRepresentation(ctx sdk.Context, poolId uint64, asset0Denom string, asset1Denom string) (types.TwapRecord, error) { + asset0Denom, asset1Denom, err := types.LexicographicalOrderDenoms(asset0Denom, asset1Denom) + if err != nil { + return types.TwapRecord{}, err + } store := ctx.KVStore(k.storeKey) key := types.FormatMostRecentTWAPKey(poolId, asset0Denom, asset1Denom) bz := store.Get(key) @@ -121,6 +125,10 @@ func (k Keeper) storeNewRecord(ctx sdk.Context, twap types.TwapRecord) { // * there is no record for the asset pair (asset0, asset1) in particular // - e.g. asset not in pool, or provided in wrong order. func (k Keeper) getRecordAtOrBeforeTime(ctx sdk.Context, poolId uint64, t time.Time, asset0Denom string, asset1Denom string) (types.TwapRecord, error) { + asset0Denom, asset1Denom, err := types.LexicographicalOrderDenoms(asset0Denom, asset1Denom) + if err != nil { + return types.TwapRecord{}, err + } store := ctx.KVStore(k.storeKey) // We make an iteration from time=t + 1ns, to time=0 for this pool. // Note that we cannot get any time entries from t + 1ns, as the key would be `prefix|t+1ns` diff --git a/x/twap/store_test.go b/x/twap/store_test.go index cdae2ebc2d7..0602bf0a5b7 100644 --- a/x/twap/store_test.go +++ b/x/twap/store_test.go @@ -82,24 +82,24 @@ func (s *TestSuite) TestGetAllMostRecentRecordsForPool() { "set multi-asset pool record": { recordsToSet: []types.TwapRecord{ newEmptyPriceRecord(1, baseTime, denom0, denom1), - newEmptyPriceRecord(1, baseTime, denom2, denom0), - newEmptyPriceRecord(1, baseTime, denom2, denom1)}, + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2)}, poolId: 1, expectedRecords: []types.TwapRecord{ newEmptyPriceRecord(1, baseTime, denom0, denom1), - newEmptyPriceRecord(1, baseTime, denom2, denom1), - newEmptyPriceRecord(1, baseTime, denom2, denom0)}, + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2)}, }, "set multi-asset pool record - reverse order": { recordsToSet: []types.TwapRecord{ - newEmptyPriceRecord(1, baseTime, denom2, denom0), - newEmptyPriceRecord(1, baseTime, denom2, denom1), + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2), newEmptyPriceRecord(1, baseTime, denom0, denom1)}, poolId: 1, expectedRecords: []types.TwapRecord{ newEmptyPriceRecord(1, baseTime, denom0, denom1), - newEmptyPriceRecord(1, baseTime, denom2, denom1), - newEmptyPriceRecord(1, baseTime, denom2, denom0)}, + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2)}, }, } @@ -143,7 +143,7 @@ func (s *TestSuite) TestGetRecordAtOrBeforeTime() { }{ "no entries": {[]types.TwapRecord{}, defaultInputAt(baseTime), baseRecord, true}, "get at latest (exact)": {[]types.TwapRecord{baseRecord}, defaultInputAt(baseTime), baseRecord, false}, - "rev at latest (exact)": {[]types.TwapRecord{baseRecord}, defaultRevInputAt(baseTime), baseRecord, true}, + "rev at latest (exact)": {[]types.TwapRecord{baseRecord}, defaultRevInputAt(baseTime), baseRecord, false}, "get latest (exact) w/ past entries": { []types.TwapRecord{tMin1Record, baseRecord}, defaultInputAt(baseTime), baseRecord, false}, @@ -152,7 +152,7 @@ func (s *TestSuite) TestGetRecordAtOrBeforeTime() { "get sandwitched entry (exact)": { []types.TwapRecord{tMin1Record, baseRecord, tPlus1Record}, defaultInputAt(baseTime), baseRecord, false}, "rev sandwitched entry (exact)": { - []types.TwapRecord{tMin1Record, baseRecord, tPlus1Record}, defaultRevInputAt(baseTime), baseRecord, true}, + []types.TwapRecord{tMin1Record, baseRecord, tPlus1Record}, defaultRevInputAt(baseTime), baseRecord, false}, "get future": {[]types.TwapRecord{baseRecord}, defaultInputAt(tPlus1), baseRecord, false}, "get future w/ past entries": {[]types.TwapRecord{tMin1Record, baseRecord}, defaultInputAt(tPlus1), baseRecord, false}, diff --git a/x/twap/types/utils.go b/x/twap/types/utils.go index 01b73bbe66e..692d95fcc9a 100644 --- a/x/twap/types/utils.go +++ b/x/twap/types/utils.go @@ -10,25 +10,6 @@ import ( "github.com/osmosis-labs/osmosis/v11/osmoutils" ) -func NewTwapRecord(k AmmInterface, ctx sdk.Context, poolId uint64, denom0 string, denom1 string) (TwapRecord, error) { - if !(denom0 > denom1) { - return TwapRecord{}, fmt.Errorf("precondition denom0 > denom1 not satisfied. denom0 %s | denom1 %s", denom0, denom1) - } - sp0 := MustGetSpotPrice(k, ctx, poolId, denom0, denom1) - sp1 := MustGetSpotPrice(k, ctx, poolId, denom1, denom0) - return TwapRecord{ - PoolId: poolId, - Asset0Denom: denom0, - Asset1Denom: denom1, - Height: ctx.BlockHeight(), - Time: ctx.BlockTime(), - P0LastSpotPrice: sp0, - P1LastSpotPrice: sp1, - P0ArithmeticTwapAccumulator: sdk.ZeroDec(), - P1ArithmeticTwapAccumulator: sdk.ZeroDec(), - }, nil -} - // mustGetSpotPrice returns the spot price for the given pool id, and denom0 in terms of denom1. // Panics if the pool state is misconfigured, which will halt any tx that interacts with this. func MustGetSpotPrice(k AmmInterface, ctx sdk.Context, poolId uint64, baseAssetDenom string, quoteAssetDenom string) sdk.Dec { @@ -75,3 +56,16 @@ func SpotPriceTimesDuration(sp sdk.Dec, timeDelta time.Duration) sdk.Dec { func AccumDiffDivDuration(accumDiff sdk.Dec, timeDelta time.Duration) sdk.Dec { return accumDiff.QuoInt64(int64(timeDelta)) } + +// LexicographicalOrderDenoms takes two denoms and returns them to be in lexicographically ascending order. +// In other words, the first returned denom string will be the lexicographically smaller of the two denoms. +// If the denoms are equal, an error will be returned. +func LexicographicalOrderDenoms(denom0, denom1 string) (string, string, error) { + if denom0 == denom1 { + return "", "", fmt.Errorf("both assets cannot be of the same denom: assetA: %s, assetB: %s", denom0, denom1) + } + if denom0 > denom1 { + denom0, denom1 = denom1, denom0 + } + return denom0, denom1, nil +} diff --git a/x/twap/types/utils_test.go b/x/twap/types/utils_test.go index ef5abca3a6d..bd9e99a4471 100644 --- a/x/twap/types/utils_test.go +++ b/x/twap/types/utils_test.go @@ -32,3 +32,31 @@ func TestGetAllUniqueDenomPairs(t *testing.T) { }) } } + +func TestLexicographicalOrderDenoms(t *testing.T) { + tests := map[string]struct { + firstDenom string + secondDenom string + expectedDenomA string + expectedDenomB string + expectErr bool + }{ + "basic": {"A", "B", "A", "B", false}, + "basicRev": {"B", "A", "A", "B", false}, + "realDenoms": {"uosmo", "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2", + "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2", "uosmo", false}, + "sameDenom": {"A", "A", "", "", true}, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + // system under test + denomA, denomB, err := LexicographicalOrderDenoms(tt.firstDenom, tt.secondDenom) + if tt.expectErr { + require.Error(t, err) + } else { + require.Equal(t, denomA, tt.expectedDenomA) + require.Equal(t, denomB, tt.expectedDenomB) + } + }) + } +}