[x/TWAP] Expose a geometric TWAP API #3529

stackman27 · 2022-11-26T01:53:23Z

Closes: Part of #3514 [1-3]

What is the purpose of the change

We need to expose a geometric TWAP API. Its implementation would be almost identical to the arithmetic TWAP API.
Therefore, we must develop proper abstractions to reduce code duplication and avoid complexity.

Brief Changelog

n/a

Testing and Verifying

n/a

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

x/twap/strategy.go

p0mvn · 2022-11-26T02:15:08Z

x/twap/strategy.go

+	quoteAssetDenom string,
+	startTime time.Time,
+) (sdk.Dec, error) {
+	// decide whether we want to use arithmetic or geometric twap


What do you mean by "decide between arithmetic or geometric" here? I think this might confuse folks.

arithmetic strategy should always utilize the respective arithmetic methods

isn't the idea to have a isArithmeticType bool to decide if we want to calculate arithmetic or geometric in this function?

There is no isArithmeticType boolean in this function.

If you are referring to some changes introduced in: #3420, those would actually need to be refactored to be integrated with the strategy pattern here. So the boolean parameter isArithmeticTwap in #3420 would likely be changed to taking a strategy instead.

I suggest assuming that #3420 does not exist for the context of this PR. We will integrate them after one or the other is merged

x/twap/api.go

stackman27 · 2022-11-26T05:53:43Z

@p0mvn Added some simple godocs because i couldn't figure out a lot to add. lmk what you think

x/twap/strategy.go

x/twap/api.go

x/twap/strategy.go

x/twap/api.go

p0mvn · 2022-11-27T01:30:27Z

@p0mvn Added some simple godocs because i couldn't figure out a lot to add. lmk what you think

Added some suggestions. LGTM once addressed!

stackman27 · 2022-11-27T06:03:36Z

@p0mvn Resolved comments bcad2b2 :D

p0mvn

LGTM. Nice work

stackman27 · 2022-11-29T03:30:15Z

@p0mvn rebased! might wanna take a quick skim

p0mvn · 2022-11-29T17:47:53Z

@czarcas7ic @nicolaslara hey, can we get one more review here please?

czarcas7ic

This LGTM

nicolaslara · 2022-11-30T08:42:38Z

x/twap/strategy.go

+		quoteAssetDenom string,
+		startTime time.Time) (sdk.Dec, error)
+	// computeTwap calculates the TWAP with specific startRecord and endRecord.
+	computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error)


nit: Should we unify these names: either both be called "get" or both called "compute"?

also. Is there a reason to have getTwapToNow be part of the strategy?

i think unifying makes sense, i'm up for naming it getTwapToNow and getTwap. @p0mvn lmk what you think of this

For getTwapToNow I was calling the keeper method directly here but i changed it to use strategy.go route

nicolaslara

Some nits in the comments, but other than that this LGTM

p0mvn · 2022-12-01T15:39:55Z

x/twap/api.go

 ) (sdk.Dec, error) {
 	if startTime.After(endTime) {
 		return sdk.Dec{}, types.StartTimeAfterEndTimeError{StartTime: startTime, EndTime: endTime}
 	}
 	if endTime.Equal(ctx.BlockTime()) {
-		return k.GetArithmeticTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime)
+		return strategy.getTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime)


Just realized something from @nicolaslara 's comment.

The only reason we define getTwapToNow on strategy is to be able to call it here.

However, we can instead do the following:

k.getTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime, strategy)

and remove the getTwapToNow from strategy in general

sounds good

x/twap/strategy.go

p0mvn · 2022-12-01T15:46:14Z

x/twap/api.go

@@ -92,14 +122,12 @@ func (k Keeper) GetArithmeticTwapToNow(
 	if err != nil {
 		return sdk.Dec{}, err
 	}
-	return computeTwap(startRecord, endRecord, quoteAssetDenom, arithmeticTwapType)
+
+	return strategy.computeTwap(startRecord, endRecord, quoteAssetDenom)


Just commenting for discussion. I think that in the future PR, we should actually revert this and make computeTwap take the strategy itself instead of the type to improve the low-level compute logic.

Should we update #3514 to add it there then?

Yeah I will update today

* refactored twap api.go for geometric TWAP * added barebon docs * romans feedback * new * fix * nichola feedback * final roman comments

…14 (#3925) * Upgrade IBC to v4.2.0 (#3838) * initial changes to migrate to ibc v4 * added checksum to proposal * begin and end block are now being called inside nextBlock * added changelog * linked pr on changelog * remove local replace * using error acks from osmoutils * osmoutils tagged * go sum * added checksum * feat(x/twap): modify cli to add geometric option (#3812) * feat(x/twap): geometric twap code gen query boilerplate * revert cli change * query gen * wire up API * test * fix * fixes * cli * lint * refactor via flag * refactor * refactor * fixes * lint * add arithmetic twap alias * Make wasm hooks importable (#3850) * moved ibc-hooks to its own go.mod * updated ibc hooks version * go sum * add ics23 patch into x/ibc-hooks * Fix wasm import version conflict * Update x/ibc-hooks to osmoutils v0.0.2 * Update versions Co-authored-by: Dev Ojha <[email protected]> * refactor(x/twap): handle spot price error case in the context of geometric twap (#3845) * refactor(x/twap): handle spot price error case * supporting test cases * table-driven log tests * test(x/twap): add randomized geometric twap test on a balancer pool (#3844) * test(x/twap): add randomized test with a balancer pool * comments * multiplicative tolerance, fewer retries and larger initial supply range * Basic geometric twap e2e test (#3835) * feat(x/twap): geometric twap code gen query boilerplate * revert cli change * query gen * wire up API * test * fix * fixes * add geometric queries * create pool.json * add test * resolve conflict * fix: swap uosmo in * fix problem with wallet creation * updates * simplify and add comments * Update tests/e2e/e2e_test.go * Update tests/e2e/e2e_test.go * Update tests/e2e/configurer/chain/queries.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/configurer/chain/queries.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Adam Tucker <[email protected]> * feat(x/twap): whitelist GeometricTwap and GeometricTwapToNow (#3852) * feat(x/twap): GeometricTwap and GeometricTwapToNow queries added to Stargate whitelist * update docs * fix(scripts): proto gen for osmoutils (#3854) * fix(scripts): proto gen for osmoutils * Update scripts/protocgen.sh * fix(scripts): proto gen osmoutils path (#3859) * added packet timeouts to wasm hooks (#3862) * add negative to cli (#3888) * Making osmoutils compile on chains that don't use our SDK fork (#3899) * making osmoutils compile on chains that don't use osmosis' fork of the cosmos sdk * updated imports for e2e tests * go fumpt * updated version everywhere * added changelog entry * remove deprecation from arithmetic & geometric twap to now query (#3917) * Add types & boilerplate for the Downtime detector module (#3609) Sub-component of #3603 Adds types for the thin module intended for downtime detection - Add downtime detection module types No tests added - Does this pull request introduce a new feature or user-facing behavior changes? somewhat - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes - How is the feature or change documented? In its spec * Add downtime detector module (#3688) * WIP * Switch to enum * Remove params query * Add query * Add wiring, add import/export test * Start begin block test * Finish keeper tests * Add CLI * Wire downtime detector CLI + queries * more module wiring * add types test * Fix state alteration test * Fix superfluid test * Add stargate whitelist support * Apply code comment * Rename folder * Add requested godoc * Update x/downtime-detector/genesis.go Co-authored-by: Adam Tucker <[email protected]> * Apply adam suggestion for having a `-` * move genesis test to own file Co-authored-by: Adam Tucker <[email protected]> * Initial by hand fixes * feat(osmomath): Exp2 function (#3708) * feat(osmomath): exp2 function * export exp2 * changelog * refactor ErrTolerance to use Dec instead of Int for additive tolerance * Update osmomath/exp2.go * Update osmomath/exp2.go * Update osmomath/exp2.go * Update osmomath/exp2_test.go * Update osmomath/exp2_test.go * do bit shift instead of multiplication * godoc about error bounds * comment about bit shift equivalency * merge conflict * improve godoc * typo * remove TODOs - confirmed obsolete * Runge's phenomenon comment * [x/TWAP] Expose a geometric TWAP API (#3529) * refactored twap api.go for geometric TWAP * added barebon docs * romans feedback * new * fix * nichola feedback * final roman comments * fix twap by hand * change to gamm * fix balancer test * bump to v14 upgrade * e2e fix * add remaining diff from main to ibc-rate-limit * update contracts test * osmomath: `AddMut` and `QuoMut` (#3779) * mut add * test add mut * quo mut * test quo mut/ remove want from test struct * refactor exp * change mutatives code * change * not allocaing * exp change to quomut * remove file * refactor quo * refactor ad * refactor tests * Modify CHANGELOG * Whitelist EstimateSwapExactAmountOut (#3693) * whitelist EstimateSwapExactAmountOut * doc: changelog * updated rate limit contract * Fix rust checks (#3576) * added cargo.lock * added Cargo.lock as an artifact * added new bytecode with lock file Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Supanat <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: mattverse <[email protected]> Co-authored-by: ByeongSu Hong <[email protected]>

stackman27 requested review from a team and p0mvn November 26, 2022 01:53

github-actions bot added the C:x/twap Changes to the twap module label Nov 26, 2022

stackman27 requested review from czarcas7ic, mattverse and ValarDragon November 26, 2022 01:53

p0mvn reviewed Nov 26, 2022

View reviewed changes

p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Nov 26, 2022

p0mvn reviewed Nov 27, 2022

View reviewed changes

p0mvn approved these changes Nov 27, 2022

View reviewed changes

stackman27 mentioned this pull request Nov 29, 2022

[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

Merged

stackman27 force-pushed the twap-geo branch from 515b583 to 70528e2 Compare November 29, 2022 03:30

p0mvn requested a review from nicolaslara November 29, 2022 17:47

p0mvn removed request for ValarDragon and mattverse November 29, 2022 17:48

p0mvn mentioned this pull request Nov 29, 2022

feat(x/twap): implement reusable TWAP API #3514

Closed

2 tasks

czarcas7ic approved these changes Nov 29, 2022

View reviewed changes

nicolaslara reviewed Nov 30, 2022

View reviewed changes

nicolaslara approved these changes Nov 30, 2022

View reviewed changes

stackman27 added 6 commits November 30, 2022 14:42

refactored twap api.go for geometric TWAP

ecfa8a0

added barebon docs

f46e4d5

romans feedback

d808d81

new

7a54da8

fix

03eb8ae

nichola feedback

5ae2eaa

stackman27 force-pushed the twap-geo branch from 70528e2 to 5ae2eaa Compare December 1, 2022 04:19

p0mvn reviewed Dec 1, 2022

View reviewed changes

final roman comments

a2e66da

p0mvn approved these changes Dec 1, 2022

View reviewed changes

p0mvn merged commit 0d13512 into main Dec 1, 2022

p0mvn deleted the twap-geo branch December 1, 2022 16:58

czarcas7ic pushed a commit that referenced this pull request Jan 4, 2023

[x/TWAP] Expose a geometric TWAP API (#3529)

048a2a5

* refactored twap api.go for geometric TWAP * added barebon docs * romans feedback * new * fix * nichola feedback * final roman comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[x/TWAP] Expose a geometric TWAP API #3529

[x/TWAP] Expose a geometric TWAP API #3529

stackman27 commented Nov 26, 2022

p0mvn Nov 26, 2022

stackman27 Nov 26, 2022

p0mvn Nov 27, 2022

stackman27 Nov 28, 2022

stackman27 commented Nov 26, 2022

p0mvn commented Nov 27, 2022

stackman27 commented Nov 27, 2022 •

edited

Loading

p0mvn left a comment

stackman27 commented Nov 29, 2022 •

edited

Loading

p0mvn commented Nov 29, 2022

czarcas7ic left a comment

nicolaslara Nov 30, 2022

nicolaslara Nov 30, 2022

stackman27 Nov 30, 2022

nicolaslara left a comment

p0mvn Dec 1, 2022

stackman27 Dec 1, 2022

p0mvn Dec 1, 2022

stackman27 Dec 1, 2022

p0mvn Dec 1, 2022

[x/TWAP] Expose a geometric TWAP API #3529

[x/TWAP] Expose a geometric TWAP API #3529

Conversation

stackman27 commented Nov 26, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Nov 26, 2022

p0mvn commented Nov 27, 2022

stackman27 commented Nov 27, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

stackman27 commented Nov 29, 2022 • edited Loading

p0mvn commented Nov 29, 2022

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

nicolaslara 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

Choose a reason for hiding this comment

stackman27 commented Nov 27, 2022 •

edited

Loading

stackman27 commented Nov 29, 2022 •

edited

Loading