Fix: Flip existing twapRecords base/quote price denoms #5939

stackman27 · 2023-08-01T06:28:08Z

Closes: #XXX

What is the purpose of the change

since twap depends on spot-price query and spot-price query base/quote denom is switched. We switch that back in TWAP too

Testing and Verifying

added test for upgrade handler

existing twap records

❯ osmosisd q twap arithmetic 1066 uosmo 1690755728 24h  --node=https://rpc.osmosis.zone:443
arithmetic_twap: "0.000000000002056044"
❯ osmosisd q twap arithmetic 1066 ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7 1690755728 24h  --node=https://rpc.osmosis.zone:443
arithmetic_twap: "486397499707.552675212311200837"

should be flipped

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

app/upgrades/v17/upgrades.go

x/twap/store.go

czarcas7ic · 2023-08-02T03:47:31Z

@stackman27 does anything need to change now that #5863 was merged?

czarcas7ic

Will do another pass when these are addressed, thank you!

app/upgrades/v17/upgrades.go

app/upgrades/v17/upgrades_test.go

stackman27 · 2023-08-02T19:20:41Z

@stackman27 does anything need to change now that #5863 was merged?

i dont think so, i compared mainnet twap records with mainnet spot price. So if mainnet spot price is wrong twap should be wrong

for ex: in the call below osmo is base and we are quoting dai incorrectly

it's showing 1osmo ~ 2.056044dai
and 1dai ~ 0.4863974997075527 osmo

❯ osmosisd q twap arithmetic 1066 uosmo 1690755728 24h  --node=https://rpc.osmosis.zone:443
arithmetic_twap: "0.000000000002056044"

app/upgrades/v17/upgrades.go

app/upgrades/v16/upgrades.go

CHANGELOG.md

app/upgrades/v16/upgrades.go

x/concentrated-liquidity/pool.go

app/upgrades/v17/upgrades.go

x/twap/store.go

app/upgrades/v17/upgrades.go

x/twap/store.go

czarcas7ic

This looks good, but we should add the upgrade handler test described in comment before merging. Will approve after that, then will need a second approval from there. Thanks!

czarcas7ic · 2023-08-07T23:23:13Z

app/upgrades/v17/upgrades.go

+func FlipTwapSpotPriceRecords(ctx sdk.Context, poolIds []uint64, keepers *keepers.AppKeepers) error {
+	for _, poolId := range poolIds {
+		// check that this is a cl pool
+		_, err := keepers.ConcentratedLiquidityKeeper.GetConcentratedPoolById(ctx, poolId)


Lets just take in an array of PoolI without the type check. Then in the upgrade handler we just call GetPools and pass in the result of GetPools directly in. No need to

Get pools

Extract pool IDs

Get pool again

fixed! 21a7552

app/upgrades/v17/upgrades.go

czarcas7ic · 2023-08-07T23:30:53Z

app/upgrades/v17/upgrades_test.go

+				clPoolTwapRecordPreUpgrade, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, existingPool.GetId())
+				suite.Require().NoError(err)
+
+				clPoolTwapRecordHistoricalPoolIndex, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, existingPool.GetId())
+				suite.Require().NoError(err)


Its good that we added this here, but we need to add logic in the success cases that checks the new record post upgrade to ensure both records were changed as expected. I would also create more than one historical record. Then, after the upgrade, you can iterate over the historical records to see if the values were swapped, then just compare the current record to ensure that flipped as well.

oh yea sorry i had this test created but forgot to push 78c7cec#diff-8d0951a4357ce3ef3f57168835dcc72078bd30337d8f9882c3b0d1e64a815e77R180-R195

the above test contains >1 historical records. lmk if this suffice, i can write more test too!

added more detailed test here: 4ea33fa

app/upgrades/v17/upgrades.go

x/twap/store.go

x/twap/types/keys.go

x/twap/store.go

p0mvn · 2023-08-08T10:23:32Z

What I'm missing from approval is the answer to this question in the review.

I think that after the earlier suggested refactor was complete, it is not present but would like a confirmation.

p0mvn

This cycle focuses on tests. I think the tests should be expanded prior to merge

app/upgrades/v17/upgrades_test.go

p0mvn

This LGTM after the latest test requests are addressed

app/upgrades/v17/upgrades_test.go

p0mvn

Nice work on working through a long series of feedbacks! LGTM

Have two more minor comments + a general suggestion that test code could benefit from more comments explaining the invariants being tested. Bumping up our recently updated guidelines here

p0mvn · 2023-08-08T18:11:02Z

app/upgrades/v17/upgrades_test.go


 			},
-			func(ctx sdk.Context, keepers *keepers.AppKeepers, expectedCoinsUsedInUpgradeHandler sdk.Coins, lastPoolID uint64) {
+			func(ctx sdk.Context, keepers *keepers.AppKeepers, expectedCoinsUsedInUpgradeHandler sdk.Coins, lastPoolID uint64, clPoolTwapRecordPreUpgrade []types.TwapRecord) {


Consider avoiding passing in clPoolTwapRecordPreUpgrade here and returning them from pre-upgrade logic. Instead, use the pool id similar to the other pool

okay that sounds good too

addressed 8460510

p0mvn · 2023-08-08T18:13:48Z

app/upgrades/v17/upgrades_test.go

+					if (clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].PoolId == lastPoolID) || (clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].PoolId == lastPoolIdMinusOne) {
+						suite.Require().Equal(clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i].Asset0Denom, clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].Asset1Denom)
+						suite.Require().Equal(clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i].Asset1Denom, clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].Asset0Denom)
+						suite.Require().Equal(clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i].P0LastSpotPrice, clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].P1LastSpotPrice)
+						suite.Require().Equal(clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i].P1LastSpotPrice, clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i].P0LastSpotPrice)
+					}


Please also add an else case here and check that for other pools these are not switched. Please make sure that at least one other pool has historical time indexed twap record

since all CL pool gets flipped & we delete old index and create new index this was a bit hard to check. I checked with balancer pool and confirmed records are not flipped

czarcas7ic

Nice work! The changes you made to the test file make me feel much better about this change.

Side note, when we upgrade testnet, can you check that this flipped the testnet record as expected as well?

app/upgrades/v17/upgrades.go

czarcas7ic · 2023-08-09T01:21:22Z

app/upgrades/v17/upgrades_test.go

+				// check that all TWAP records aren't empty
+				suite.Require().NotEmpty(clPool1TwapRecordPostUpgrade, "Most recent TWAP records should not be empty after upgrade.")
+				suite.Require().NotEmpty(clPool1TwapRecordHistoricalPoolIndexPostUpgrade, "Historical Pool Index TWAP record should not be empty after upgrade.")
+				suite.Require().NotEmpty(clPool2TwapRecordPostUpgrade, "Most recent TWAP records should not be empty after upgrade.")
+				suite.Require().NotEmpty(clPool2TwapRecordHistoricalPoolIndexPostUpgrade, "Historical Pool Index TWAP record should not be empty after upgrade.")
+				suite.Require().NotEmpty(clPoolsTwapRecordHistoricalTimeIndexPostUpgrade, "Historical Time Index TWAP record should not be empty after upgrade.")


nit: i think this is just unnecessary verbosity and that gets indirectly checked below

removed the text but kept the check!

app/upgrades/v17/upgrades_test.go

stackman27 · 2023-08-09T01:55:32Z

Merging since it has 2 approvals!!

* added logic * added upgrade handler test * added changelog * flip spot price too * adams suggestions * rebased * adams final suggestion * roman comments * roman suggestion * cleanup * romans suggestion * lint * misc cleanup * added test * more tests * added verbose tests * nit * romans comment for different twap request * nit * roman suggestion * adams suggestion

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

stackman27 commented Aug 1, 2023

View reviewed changes

app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved

stackman27 commented Aug 1, 2023

View reviewed changes

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

stackman27 added the V:state/breaking State machine breaking PR label Aug 1, 2023

stackman27 marked this pull request as ready for review August 1, 2023 21:02

stackman27 requested review from nicolaslara, p0mvn and AlpinYukseloglu as code owners August 1, 2023 21:02

stackman27 force-pushed the stack/fix-twap branch from 1bb751a to 23c923f Compare August 1, 2023 22:05

czarcas7ic reviewed Aug 2, 2023

View reviewed changes

app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades.go Show resolved Hide resolved

app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved

github-actions bot added the C:x/concentrated-liquidity label Aug 2, 2023

stackman27 requested a review from czarcas7ic August 2, 2023 19:54

stackman27 force-pushed the stack/fix-twap branch from d64828a to 617d6af Compare August 3, 2023 02:34

czarcas7ic reviewed Aug 3, 2023

View reviewed changes

app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved

czarcas7ic reviewed Aug 3, 2023

View reviewed changes

app/upgrades/v16/upgrades.go Outdated Show resolved Hide resolved

stackman27 requested a review from czarcas7ic August 3, 2023 16:03

p0mvn requested changes Aug 3, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

app/upgrades/v16/upgrades.go Outdated Show resolved Hide resolved

x/concentrated-liquidity/pool.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades.go Show resolved Hide resolved

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

github-actions bot removed the C:x/concentrated-liquidity label Aug 3, 2023

stackman27 requested a review from p0mvn August 3, 2023 21:44

p0mvn reviewed Aug 7, 2023

View reviewed changes

app/upgrades/v17/upgrades.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades.go Show resolved Hide resolved

p0mvn reviewed Aug 7, 2023

View reviewed changes

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

stackman27 force-pushed the stack/fix-twap branch from c67904e to 5c3b4cf Compare August 7, 2023 16:16

stackman27 requested a review from p0mvn August 7, 2023 16:22

stackman27 force-pushed the stack/fix-twap branch from ee93924 to 8709c4a Compare August 7, 2023 16:37

ValarDragon assigned p0mvn Aug 7, 2023

czarcas7ic reviewed Aug 7, 2023

View reviewed changes

stackman27 force-pushed the stack/fix-twap branch from 169850f to 78c7cec Compare August 8, 2023 03:12

stackman27 added 9 commits August 8, 2023 06:23

adams final suggestion

ce8d5f0

roman comments

7aaf632

roman suggestion

166d0f9

cleanup

3ff7e90

romans suggestion

9e1ed18

lint

2a5ac83

misc cleanup

460dada

added test

c20d814

more tests

21a7552

stackman27 force-pushed the stack/fix-twap branch from 78c7cec to 21a7552 Compare August 8, 2023 06:30

github-actions bot removed the C:x/tokenfactory label Aug 8, 2023

stackman27 requested a review from czarcas7ic August 8, 2023 06:37

p0mvn reviewed Aug 8, 2023

View reviewed changes

app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved

app/upgrades/v17/upgrades_test.go Show resolved Hide resolved

stackman27 added 2 commits August 8, 2023 16:58

added verbose tests

4ea33fa

nit

fe7d3bd

stackman27 requested a review from p0mvn August 8, 2023 17:11

p0mvn reviewed Aug 8, 2023

View reviewed changes

app/upgrades/v17/upgrades_test.go Show resolved Hide resolved

app/upgrades/v17/upgrades_test.go Outdated Show resolved Hide resolved

stackman27 added 2 commits August 8, 2023 17:55

romans comment for different twap request

fd6f19f

nit

500b303

stackman27 requested a review from p0mvn August 8, 2023 17:59

p0mvn approved these changes Aug 8, 2023

View reviewed changes

roman suggestion

8460510

czarcas7ic approved these changes Aug 9, 2023

View reviewed changes

adams suggestion

520d6b1

stackman27 merged commit aae894f into main Aug 9, 2023
1 check passed

stackman27 deleted the stack/fix-twap branch August 9, 2023 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Flip existing twapRecords base/quote price denoms #5939

Fix: Flip existing twapRecords base/quote price denoms #5939

stackman27 commented Aug 1, 2023 •

edited

Loading

czarcas7ic commented Aug 2, 2023

czarcas7ic left a comment

stackman27 commented Aug 2, 2023 •

edited

Loading

czarcas7ic left a comment

czarcas7ic Aug 7, 2023

stackman27 Aug 8, 2023

czarcas7ic Aug 7, 2023

stackman27 Aug 8, 2023

stackman27 Aug 8, 2023 •

edited

Loading

stackman27 Aug 8, 2023

p0mvn commented Aug 8, 2023

p0mvn left a comment

p0mvn left a comment

p0mvn left a comment

p0mvn Aug 8, 2023

stackman27 Aug 8, 2023 •

edited

Loading

p0mvn Aug 8, 2023

stackman27 Aug 8, 2023 •

edited

Loading

czarcas7ic left a comment

czarcas7ic Aug 9, 2023

stackman27 Aug 9, 2023

stackman27 commented Aug 9, 2023

Fix: Flip existing twapRecords base/quote price denoms #5939

Fix: Flip existing twapRecords base/quote price denoms #5939

Conversation

stackman27 commented Aug 1, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

czarcas7ic commented Aug 2, 2023

czarcas7ic left a comment

Choose a reason for hiding this comment

stackman27 commented Aug 2, 2023 • edited Loading

czarcas7ic 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

Choose a reason for hiding this comment

stackman27 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Aug 8, 2023

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Aug 9, 2023

stackman27 commented Aug 1, 2023 •

edited

Loading

stackman27 commented Aug 2, 2023 •

edited

Loading

stackman27 Aug 8, 2023 •

edited

Loading

stackman27 Aug 8, 2023 •

edited

Loading

stackman27 Aug 8, 2023 •

edited

Loading