chore: twap mutation fixes part 1 #2468

czarcas7ic · 2022-08-19T21:02:52Z

Part Of: #2426

What is the purpose of the change

This PR brings the mutation test score for the twap module from ~33% to ~46%

PRing this now to prevent a large diff for the next / upcoming mutation fixes

Brief Changelog

Adds RunBasicJoin as a GAMM keeper test helper
Corrects lexicographical order of twap denoms
- getMostRecentRecord switches order
- also checks that the denoms are not equal
Removes ordering from GetBeginBlockAccumulatorRecord (this is done in getMostRecentRecord which this function calls/returns. Better to have the logic at the higher level)
Improves TestGetBeginBlockAccumulatorRecord

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? not applicable

x/twap/types/utils.go

czarcas7ic · 2022-08-19T23:51:44Z

x/twap/api.go

-	// correct ordering of args for db
-	if asset1Denom > asset0Denom {
-		asset0Denom, asset1Denom = asset1Denom, asset0Denom
-	}
 	return k.getMostRecentRecord(ctx, poolId, asset0Denom, asset1Denom)


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

czarcas7ic · 2022-08-19T23:53:16Z

x/twap/api_test.go

@@ -27,16 +28,18 @@ func (s *TestSuite) TestGetBeginBlockAccumulatorRecord() {
 		poolId     uint64
 		quoteDenom string
 		baseDenom  string
-		expError   bool
+		expError   error


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

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

czarcas7ic · 2022-08-19T23:54:56Z

x/twap/keeper_test.go

@@ -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


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.

czarcas7ic · 2022-08-19T23:55:32Z

x/twap/listeners_test.go

@@ -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)


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

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.

ValarDragon

Great job! Using the right order + this helper really cleans things up!

One change needed for the RunBasicJoin test, then I think its good to merge :)

x/twap/listeners_test.go

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

app/apptesting/gamm.go

ValarDragon · 2022-08-22T11:21:08Z

x/twap/listeners_test.go

 			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])
+			for i := len(denomPairs0); i < 0; i-- {
+				expectedRecord, err := twap.NewTwapRecord(s.App.GAMMKeeper, s.Ctx, poolId, denomPairs0[i], denomPairs1[i])


Why are we iterating in reverse order?

I just realized why I thought this was necessary. GetAllUniqueDenomPairs returns the denom pairs in non lexicographical order and NewTwapRecord auto rearranges them. Then when we get down to GetMostRecentRecordStoreRepresentation, the order of input matters and does not get auto corrected. I am correcting this now

okay, fixed in most recent commit. Will need to write in spec about these guarantees still though

ValarDragon · 2022-08-22T11:23:34Z

x/twap/types/utils.go

-	if !(denom0 > denom1) {
-		return TwapRecord{}, fmt.Errorf("precondition denom0 > denom1 not satisfied. denom0 %s | denom1 %s", denom0, denom1)
-	}


If we remove this check here, I think we need to be clear in the spec / code comments about guarantees we expect. (Which methods expect correct order, which ones will swap order)

ValarDragon · 2022-08-23T00:04:36Z

x/twap/README.md

+All functions that call `LexicographicalOrderDenoms` (such as `getMostRecentRecordStoreRepresentation`, `getRecordAtOrBeforeTime`, and `NewTwapRecord`), both directly and indirectly, will swap the provided denoms to be in ascending lexicographical order. This means that regardless of input, asset0Denom is guaranteed to be lexicographically smaller than asset1Denom.
+If this does not occur, an error will be returned, likely indicating that the two denoms provided are equivalent (which is not allowed).


err, this doesn;t really make sense at the spec level / at least not the API layer: Its an abstraction leak for the caller! It would only apply for the store layer.

czarcas7ic added 2 commits August 19, 2022 15:47

mutation test fixes twap

ba4857c

specify error in test

b49296e

github-actions bot added C:app-wiring Changes to the app folder C:x/twap Changes to the twap module labels Aug 19, 2022

czarcas7ic added 4 commits August 19, 2022 16:53

more fixes

6557bf9

pool fix

bce4e6a

alphabetical order for denoms

cdcad39

fix store test

e584239

p0mvn reviewed Aug 19, 2022

View reviewed changes

x/twap/types/utils.go Show resolved Hide resolved

x/twap/types/utils.go Show resolved Hide resolved

czarcas7ic commented Aug 19, 2022

View reviewed changes

czarcas7ic marked this pull request as ready for review August 19, 2022 23:56

czarcas7ic requested a review from a team August 19, 2022 23:56

add test for LexicographicalOrderDenoms

8bfb48b

ValarDragon approved these changes Aug 20, 2022

View reviewed changes

czarcas7ic added 5 commits August 20, 2022 09:23

separate join pool test

f541e4d

move NewTwapRecord to main keeper

9b4d849

Merge branch 'main' into adam/twap-mutation-1

a32310d

merge main fixes

b12c5b4

fix lexicographic order of test

ed90689

ValarDragon reviewed Aug 21, 2022

View reviewed changes

x/twap/listeners_test.go Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 21, 2022

View reviewed changes

x/twap/listeners_test.go Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 21, 2022

View reviewed changes

x/twap/listeners_test.go Outdated Show resolved Hide resolved

czarcas7ic and others added 3 commits August 21, 2022 15:14

Apply suggestions from code review

fc343c8

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

changes from code review

5f92191

code review

96bf8c1

p0mvn approved these changes Aug 21, 2022

View reviewed changes

app/apptesting/gamm.go Show resolved Hide resolved

ValarDragon reviewed Aug 22, 2022

View reviewed changes

czarcas7ic added 2 commits August 22, 2022 09:21

address order comment

1da6e24

rev tests should now pass

24eb6ee

czarcas7ic self-assigned this Aug 22, 2022

document api guarantee

3a461ae

github-actions bot added the C:docs Improvements or additions to documentation label Aug 22, 2022

lint

1b82828

ValarDragon reviewed Aug 23, 2022

View reviewed changes

Update spec

f8347a5

ValarDragon merged commit 26d23ef into main Aug 23, 2022

ValarDragon deleted the adam/twap-mutation-1 branch August 23, 2022 00:16

ValarDragon mentioned this pull request Aug 25, 2022

Twap: Add Hooks Test, Add Unfinished Hooks, Add Params #2443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: twap mutation fixes part 1 #2468

chore: twap mutation fixes part 1 #2468

czarcas7ic commented Aug 19, 2022 •

edited

Loading

czarcas7ic Aug 19, 2022

czarcas7ic Aug 19, 2022

ValarDragon Aug 20, 2022 •

edited

Loading

czarcas7ic Aug 19, 2022

czarcas7ic Aug 19, 2022

ValarDragon Aug 20, 2022

czarcas7ic Aug 20, 2022

ValarDragon left a comment

ValarDragon Aug 22, 2022

czarcas7ic Aug 22, 2022

czarcas7ic Aug 22, 2022

ValarDragon Aug 22, 2022 •

edited

Loading

czarcas7ic Aug 22, 2022

ValarDragon Aug 23, 2022

		All functions that call `LexicographicalOrderDenoms` (such as `getMostRecentRecordStoreRepresentation`, `getRecordAtOrBeforeTime`, and `NewTwapRecord`), both directly and indirectly, will swap the provided denoms to be in ascending lexicographical order. This means that regardless of input, asset0Denom is guaranteed to be lexicographically smaller than asset1Denom.
		If this does not occur, an error will be returned, likely indicating that the two denoms provided are equivalent (which is not allowed).

chore: twap mutation fixes part 1 #2468

chore: twap mutation fixes part 1 #2468

Conversation

czarcas7ic commented Aug 19, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic commented Aug 19, 2022 •

edited

Loading

ValarDragon Aug 20, 2022 •

edited

Loading

ValarDragon Aug 22, 2022 •

edited

Loading