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

chore: twap mutation fixes part 1 #2468

Merged
merged 20 commits into from
Aug 23, 2022
Merged

chore: twap mutation fixes part 1 #2468

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ba4857c
mutation test fixes twap
czarcas7ic Aug 19, 2022
b49296e
specify error in test
czarcas7ic Aug 19, 2022
6557bf9
more fixes
czarcas7ic Aug 19, 2022
bce4e6a
pool fix
czarcas7ic Aug 19, 2022
cdcad39
alphabetical order for denoms
czarcas7ic Aug 19, 2022
e584239
fix store test
czarcas7ic Aug 19, 2022
8bfb48b
add test for LexicographicalOrderDenoms
czarcas7ic Aug 20, 2022
f541e4d
separate join pool test
czarcas7ic Aug 20, 2022
9b4d849
move NewTwapRecord to main keeper
czarcas7ic Aug 20, 2022
a32310d
Merge branch 'main' into adam/twap-mutation-1
czarcas7ic Aug 20, 2022
b12c5b4
merge main fixes
czarcas7ic Aug 20, 2022
ed90689
fix lexicographic order of test
czarcas7ic Aug 20, 2022
fc343c8
Apply suggestions from code review
czarcas7ic Aug 21, 2022
5f92191
changes from code review
czarcas7ic Aug 21, 2022
96bf8c1
code review
czarcas7ic Aug 21, 2022
1da6e24
address order comment
czarcas7ic Aug 22, 2022
24eb6ee
rev tests should now pass
czarcas7ic Aug 22, 2022
3a461ae
document api guarantee
czarcas7ic Aug 22, 2022
1b82828
lint
czarcas7ic Aug 22, 2022
f8347a5
Update spec
ValarDragon Aug 23, 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
22 changes: 22 additions & 0 deletions app/apptesting/gamm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,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
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
_, _, err = s.App.GAMMKeeper.JoinPoolNoSwap(s.Ctx, s.TestAccs[0], poolId, msg.ShareOutAmount, msg.TokenInMaxs)
s.Require().NoError(err)
}
4 changes: 0 additions & 4 deletions x/twap/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: This gets checked/modified in getMostRecentRecord which is called in the return statement which is why I removed it here.

}
25 changes: 16 additions & 9 deletions x/twap/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package twap_test

import (
"fmt"
"time"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -27,16 +28,18 @@ func (s *TestSuite) TestGetBeginBlockAccumulatorRecord() {
poolId uint64
quoteDenom string
baseDenom string
expError bool
expError error
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: changed this to specific error type since I realized things were passing with an error, but not the right error we desired. Might want to eventually have an errors type file in twap so we can specify specific error codes instead of manually specifying expected errors here

Copy link
Member

@ValarDragon ValarDragon Aug 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, we should make error types. (Lets not do that move this PR, other parallel PRs will conflict that change that logic) We don't need a code though, golang errors.Is will suffice for our usecase here

}{
"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 {
Expand All @@ -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)
})
Expand Down
6 changes: 3 additions & 3 deletions x/twap/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import (

// TODO: Consider switching this everywhere
var (
denom0 = "token/B"
denom1 = "token/A"
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))
baseTime = time.Unix(1257894000, 0).UTC()
Expand All @@ -41,7 +41,7 @@ func (s *TestSuite) SetupTest() {
// sets up a new two asset pool, with spot price 1
func (s *TestSuite) setupDefaultPool() (poolId uint64, denomA, denomB string) {
poolId = s.PrepareUni2PoolWithAssets(defaultUniV2Coins[0], defaultUniV2Coins[1])
denomA, denomB = defaultUniV2Coins[1].Denom, defaultUniV2Coins[0].Denom
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: This is where a majority of the issue was coming from, we were setting denomA as if it were lexicographically smaller when in reality it was the larger.

denomA, denomB = defaultUniV2Coins[0].Denom, defaultUniV2Coins[1].Denom
return
}

Expand Down
7 changes: 4 additions & 3 deletions x/twap/listeners_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ func (s *TestSuite) TestSwapTriggeringTrackPoolId() {
s.Require().Equal([]uint64{poolId}, s.twapkeeper.GetChangedPools(s.Ctx))
}

// TestSwapAndEndBlockTriggeringSave tests that if we:
// TestJoinSwapAndEndBlockTriggeringSave tests that if we:
// * create a pool in block 1
// * swap in block 2
// * join that pool and swap in block 2
// then after block 2 end block, we have saved records for the pool,
// for both block 1 & 2, with distinct spot prices in their records, and accumulators incremented.
// TODO: Abstract this to be more table driven, and test more pool / block setups.
func (s *TestSuite) TestSwapAndEndBlockTriggeringSave() {
func (s *TestSuite) TestJoinSwapAndEndBlockTriggeringSave() {
s.Ctx = s.Ctx.WithBlockTime(baseTime)
poolId := s.PrepareUni2PoolWithAssets(defaultUniV2Coins[0], defaultUniV2Coins[1])
expectedHistoricalTwap, err := types.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denom0, denom1)
Expand All @@ -50,6 +50,7 @@ func (s *TestSuite) TestSwapAndEndBlockTriggeringSave() {
s.Commit() // clear transient store
// Now on a clean state after a create pool
s.Require().Equal(baseTime.Add(time.Second), s.Ctx.BlockTime())
s.RunBasicJoin(poolId)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: This is needed to test the JoinPool twap hook (mutation tests caught this!)

Copy link
Member

Choose a reason for hiding this comment

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

Err, this should be its own test. We need to test on a clean instance, a join in a block will trigger the end tracking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

s.RunBasicSwap(poolId)

// accumulators are default right here
Expand Down
10 changes: 6 additions & 4 deletions x/twap/logic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,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 {
Expand All @@ -118,8 +119,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 {
Expand Down
16 changes: 8 additions & 8 deletions x/twap/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)},
},
}

Expand Down
17 changes: 14 additions & 3 deletions x/twap/types/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ 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)
func NewTwapRecord(k AmmInterface, ctx sdk.Context, poolId uint64, denom0, denom1 string) (TwapRecord, error) {
denom0, denom1, err := LexicographicalOrderDenoms(denom0, denom1)
if err != nil {
return TwapRecord{}, err
}
sp0 := MustGetSpotPrice(k, ctx, poolId, denom0, denom1)
sp1 := MustGetSpotPrice(k, ctx, poolId, denom1, denom0)
Expand Down Expand Up @@ -75,3 +76,13 @@ 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))
}

func LexicographicalOrderDenoms(denom0, denom1 string) (string, string, error) {
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
p0mvn marked this conversation as resolved.
Show resolved Hide resolved
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
}