feat: twap key refactor #7472
feat: twap key refactor #7472
Conversation
github-actions
bot
added
T:build
C:app-wiring
| @@ -168,7 +168,6 @@ go-mock-update: | |||
| mockgen -source=x/poolmanager/types/pool.go -destination=tests/mocks/pool.go -package=mocks | |||
| mockgen -source=x/gamm/types/pool.go -destination=tests/mocks/cfmm_pool.go -package=mocks | |||
| mockgen -source=x/concentrated-liquidity/types/cl_pool_extensionI.go -destination=tests/mocks/cl_pool.go -package=mocks | |||
| mockgen -source=ingest/sqs/domain/pools.go -destination=tests/mocks/sqs_pool.go -package=mocks -mock_names=PoolI=MockSQSPoolI | |||
There was a problem hiding this comment.
Drive by change, this folder no longer lives in Osmosis repo
czarcas7ic
requested review from
a team,
nicolaslara,
p0mvn and
AlpinYukseloglu
as code owners
| // Now that the TWAP keys are refactored, we can delete all time indexed TWAPs | ||
| // since we only need the pool indexed TWAPs. | ||
| keepers.TwapKeeper.DeleteAllHistoricalTimeIndexedTWAPs(ctx) | ||
|
|
There was a problem hiding this comment.
Note to selves, we may have to make these deletes smoothed over several blocks
There was a problem hiding this comment.
I think it depends on what else exists in the upgrade handler. If there are some other larger changes then yeah I think we should, otherwise itll be just like any other epoch, but less.
| suite.Run(t, new(UpgradeTestSuite)) | ||
| } | ||
|
|
||
| func (s *UpgradeTestSuite) TestUpgrade() { |
x/twap/types/keys.go
Outdated
| var buffer bytes.Buffer | ||
| timeS := osmoutils.FormatTimeString(accumulatorWriteTime) | ||
| fmt.Fprintf(&buffer, "%s%s%s%d%s%s%s%s", HistoricalTWAPTimeIndexPrefix, timeS, KeySeparator, poolId, KeySeparator, denom1, KeySeparator, denom2) | ||
| fmt.Fprintf(&buffer, "%s%d%s%s%s%s", HistoricalTWAPPoolIndexPrefix, poolId, KeySeparator, denom1, KeySeparator, denom2) |
There was a problem hiding this comment.
This needs to include the final KeySeparator!
There was a problem hiding this comment.
Can you explain why? The previous format did not have the final KeySeparator either
There was a problem hiding this comment.
The prior one wasn't a prefix It was the full key
There was a problem hiding this comment.
In general, for anything that involves prefixes (not full key) it must include the final key separator, else iterations may include things that are just longer in the final key segment. (e.g. longer denom 1's) which could happen in pools with > 2 toknes.
There was a problem hiding this comment.
Dang, we should have a CI for this or something
There was a problem hiding this comment.
Ideally we switch to collections / ORM, and quit constructing keys ourselves :)
|
Can you update the README docs as well |
Co-authored-by: Dev Ojha <[email protected]>
|
I think we can merge once tests pass |
Closes: #7435
What is the purpose of the change
Previously, we required 2 different TWAP keys. One indexed by time, strictly used for pruning, and one indexed by the pool ID. We can refactor pruning logic to prune via the pool ID index. This allows us to reduce the 2x storage / deletion overhead.
Testing and Verifying
After implementing this change, the test that previously showed extra records being stored no longer shows these records being stored. All other tests show the same results as prior to this change.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)