TWAP logic #2168

ValarDragon · 2022-07-20T15:57:33Z

What is the purpose of the change

Core logic for TWAP implementation. Right now wanted to ask if we wanted me to split up PR'ing the types package, osmoutils, and twap/store changes first. I think the work in osmoutils is generally quite helpful for our codebase, happy to break that out sooner before finishing the remainder.

Remaining steps needed:

Brief Changelog

Implement TWAPs

Testing and Verifying

This change added tests and can be verified as follows:

Tests are added in this PR, will be breaking up components to be more easily reviewable. First broken out component is #2175

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? yes
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
How is the feature or change documented? x/gamm/twap/README

faddat · 2022-07-21T05:39:44Z

Dev, this PR is aimed at giving users the ability to buy / sell using twap?

ValarDragon · 2022-07-21T06:32:51Z

Nah, thats DCA'ing. This PR exposes on-chain TWAPs that people can then with cosmwasm to build that though!

Exposes an on-chain TWAP oracle for every pool

p0mvn

I finished reviewing the core logic but haven't reviewed the tests in detail yet.

I would like to spend more time tomorrow trying to run the tests.

LGTM overall, great job on this! Also, it is fine with me if this PR is not split further

x/gamm/twap/api.go

x/gamm/twap/logic.go

p0mvn · 2022-08-02T23:51:12Z

x/gamm/twap/hook_listener.go

 	sdk "github.com/cosmos/cosmos-sdk/types"

 	epochtypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
 	"github.com/osmosis-labs/osmosis/v10/x/gamm/types"
 )

+func (k Keeper) MigrateExistingPools(ctx sdk.Context, poolIds []uint64) error {


Not sure if it is desired to group this with hooks. Please consider moving this to migrations.go (like it is in the sdk) or to keeper.go.

I wanted this file to be listeners for things that could trigger a state change. Maybe I just change the file name to listeners.go?

I'm imagining we try to to change the entire Hook system into a publisher subscriber system

Maybe I just change the file name to listeners.go?

I like listeners.go since hook and listener feel to be synonymous. Also, I think listener is frequently used when speaking of pub-sub systems.

I wanted this file to be listeners for things that could trigger a state change

But this specific function is a one-time migration that is done during the upgrade. Why would we ever use pub-sub for this logic?

Also, I was referring to existing examples such as:

osmosis/x/lockup/keeper/migration.go

Line 40 in 3b9e72c

func MergeLockupsForSimilarDurations(

where upgrade migrations logic exists in the keeper's migrations.go file

hrmm yeah your right the migraiton being here is weird.

I just realized that we can do this in InitGenesis, which is likely the correct entry point. WIll move it there.

ah rip we can't do it in InitGenesis without adding to the gamm keeper interface, which seems worse.

I feel like keeping it in listeners.go, as what an external module can trigger logic for, is ok personally. I'd prefer not blocking on where this function lives, as it will ultimately be deleted later.

Perhaps we can call it InitGenesisAtMigration?

We agreed to leaves as is offline - I guess we can discuss this in a separate issue later

x/gamm/twap/logic.go

Closes: #XXX ## What is the purpose of the change Some minor pass through changes discovered while reviewing #2168 ## 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

p0mvn

I'm part way through tests. Still have a couple of files to review

x/gamm/twap/api_test.go

p0mvn · 2022-08-04T14:05:07Z

x/gamm/twap/api_test.go

+	tPlus10sp5Record := newTwapRecordWithDefaults(
+		baseTime.Add(10*time.Second), sdk.NewDec(5), OneSec.MulInt64(10*10), OneSec)
+	// record with t=baseTime+20, sp0=2, sp1=.5, accumulators updated from tPlus10sp5Record
+	// tPlus20sp2Record := newTwapRecordWithDefaults(


Can deadcode be removed?

p0mvn · 2022-08-04T14:06:49Z

x/gamm/twap/api_test.go

+			input:        makeSimpleTwapInput(baseTime.Add(-time.Hour), baseTime, quoteAssetA),
+			expErrorStr:  "too old",
+		},
+		// TODO: overflow tests, multi-asset pool handling, make more record interpolation cases


Agree that these would be useful. Do we have an issue for adding these?

We do for multi-asset pool handling. Not yet for overflow, but handling overflow is one of the big remaining twap items

x/gamm/twap/api_test.go

p0mvn · 2022-08-04T14:42:12Z

x/gamm/twap/hook_listener.go

 	sdk "github.com/cosmos/cosmos-sdk/types"

 	epochtypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
 	"github.com/osmosis-labs/osmosis/v10/x/gamm/types"
 )

+func (k Keeper) MigrateExistingPools(ctx sdk.Context, poolIds []uint64) error {


Maybe I just change the file name to listeners.go?

I like listeners.go since hook and listener feel to be synonymous. Also, I think listener is frequently used when speaking of pub-sub systems.

I wanted this file to be listeners for things that could trigger a state change

But this specific function is a one-time migration that is done during the upgrade. Why would we ever use pub-sub for this logic?

Also, I was referring to existing examples such as:

osmosis/x/lockup/keeper/migration.go

Line 40 in 3b9e72c

func MergeLockupsForSimilarDurations(

where upgrade migrations logic exists in the keeper's migrations.go file

app/upgrades/v12/upgrades.go

x/gamm/twap/export_test.go

x/gamm/twap/keeper_test.go

x/gamm/twap/logic.go

p0mvn

Tests LGTM. My first full pass on this PR is now complete.

Left some minor comments. I agree for the need for more tests outlined in TODOs. However, the test coverage in the core functions seems pretty good already, great work on that.

x/gamm/twap/hook_test.go

p0mvn · 2022-08-05T14:01:57Z

x/gamm/twap/hook_test.go

+	"github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
+)
+
+// TestCreatePoolFlow tests that upon a pool being created,


Most tests in this file seem to be more functional than unit in nature. While it is useful to have both, I don't think we have any unit tests for hooks.

Do we have an issue for unit testing hooks?

I don't think theres any unit tests that makes sense - they should all be functionality tests, with the expected integration.

Every hook is just "capture input from elsewhere, call unit tested method"

I guess we could make these more table driven! Definitely very open to figuring out how to make better tests for what we want here!

I will make a separate issue for this to discuss further

p0mvn · 2022-08-05T14:07:08Z

x/gamm/twap/hook_test.go

+)
+
+// TestCreatePoolFlow tests that upon a pool being created,
+// we have made the correct store entries.


I think we should also be more specific about what exactly we are testing in this file. At first, I was looking in the hook_listener.go due to the name of this file. However, then, I released that a lot of the functionality is actually related to endBlock which is defined in logic.go

So I suggest renaming this to hook_functional_test.go and adding a comment that all these tests are functional and testing the relationship between twap with other system components.

x/gamm/twap/hook_listener.go

x/gamm/twap/logic.go

x/gamm/twap/logic_test.go

p0mvn · 2022-08-05T14:42:40Z

x/gamm/twap/logic_test.go

+			interpolateTime: time.Unix(1, 0),
+			expRecord:       newExpRecord(oneDec, twoDec),
+		},
+		// TODO: Overflow tests


Bump for TODO - is this being kept track of?

p0mvn · 2022-08-05T14:42:50Z

x/gamm/twap/logic_test.go

+
+		"accumulator = 10*OneSec, t=100s. 0 base accum (asset 1)": testCaseFromDeltasAsset1(sdk.ZeroDec(), OneSec.MulInt64(10), 100*time.Second, sdk.NewDecWithPrec(1, 1)),
+
+		// TODO: Overflow, rounding


Bump for TODO - is this being kept track of?

p0mvn · 2022-08-05T14:44:26Z

x/gamm/twap/logic_test.go

+			interpolateTime: time.Unix(55, 0),
+			expRecord:       newExpRecord(oneDec.Add(OneSec.MulInt64(44*10)), twoDec.Add(OneSec.MulInt64(44).QuoInt64(10))),
+		},
+		"same time": {


What happens if newTime is before record time by mistake so the timeDelta is negative? How is this handled by the core logic? Is there a test covering this?

Hrmm, there isn't a test handling it because its incorrect behavior. (Breaks the precondition on the function - thats the responsibility of the caller) But you should still get the same result. This is because TWAP is:

(accumEnd - accumStart) / (timeEnd - timeStart)

So if you reverse the arguments, you compute

(accumStart - accumEnd) / (timeStart - timeEnd) = -(accumEnd - accumStart) / - (timeEnd - timeStart) = (accumEnd - accumStart) / (timeEnd - timeStart)

Thus the function is argument order independent.

This is for the same reason that you can don't care about the order of points when trying to compute the slope of a line.

That makes sense, thanks for explaining. I think it still makes to add a test even if we expect that to not happen and be incorrect. We should aim to have test cases relative to all possible inputs of a function even though some might be invalid when looking at the system holistically

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

ValarDragon · 2022-08-05T17:39:44Z

TODO: overflow tests are mentally kept track of, as items that need to be addressed in a subsequent overflow handling issue & PR.

Those are not the responsibility of this PR to address imo

p0mvn

LGTM. Great work!

One last thing I would like to bump is discussed here:
#2168 (comment)

x/gamm/twap/logic.go

p0mvn · 2022-08-05T17:55:21Z

x/gamm/twap/hook_listener.go

 	sdk "github.com/cosmos/cosmos-sdk/types"

 	epochtypes "github.com/osmosis-labs/osmosis/v10/x/epochs/types"
 	"github.com/osmosis-labs/osmosis/v10/x/gamm/types"
 )

+func (k Keeper) MigrateExistingPools(ctx sdk.Context, poolIds []uint64) error {


We agreed to leaves as is offline - I guess we can discuss this in a separate issue later

p0mvn · 2022-08-05T17:56:48Z

x/gamm/twap/hook_test.go

+	"github.com/osmosis-labs/osmosis/v10/x/gamm/twap/types"
+)
+
+// TestCreatePoolFlow tests that upon a pool being created,


I will make a separate issue for this to discuss further

Co-authored-by: Roman <roman@osmosis.team>

ValarDragon · 2022-08-05T19:43:11Z

Added that test! Planning to merge upon test pass, to unblock other fronts

ValarDragon added 8 commits July 18, 2022 14:31

TWAP wip

34a49b1

Add proto files

32e0406

more twap WIP

6292394

sync

daf4d2d

Merge branch 'main' into dev/twap_wip

6ff72a1

More TWAP stub progress

3f2b7a7

Add update logic

e452c24

More TWAP implementation updates

c8c0339

github-actions bot added C:app-wiring Changes to the app folder C:x/gamm Changes, features and bugs related to the gamm module. C:x/pool-incentives labels Jul 20, 2022

ValarDragon added 3 commits July 20, 2022 11:24

Do app integration

840c799

Fix simulator & bug it found

4a650f1

More logic added / data flow reordered a bit

16a0ce4

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

ValarDragon added 4 commits July 20, 2022 12:58

Add to migration handler

b9c36a0

Fix lint (by making prune stub)

7c96ede

integrate BeforeSwap hook, add key format test

d96b1d9

More keys tests

20f94fd

ValarDragon mentioned this pull request Jul 20, 2022

TWAP proto, types package, and osmoutils #2175

Merged

ValarDragon added 6 commits July 20, 2022 16:40

Add TestGetAllUniqueDenomPairs

11d6071

Test for the create pool flow

00e05d3

Correct historical iterations

52ac746

Add TrackChangedPool tests

bde6f0a

Add interpolate record test

4378f9b

Fix endblock flow

4563c1d

Add UpdateRecord test

2cd49ae

de-dup interpolate, update record logic

50d0891

Add endblock flow test

4a48f4d

ValarDragon mentioned this pull request Jul 23, 2022

Twap: Endblock tests #2211

Closed

2 tasks

Merge branch 'main' into dev/twap_wip

3b9e72c

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

p0mvn reviewed Aug 3, 2022

View reviewed changes

p0mvn mentioned this pull request Aug 3, 2022

refactor: minor pass through twap imrovements #2284

Merged

p0mvn reviewed Aug 4, 2022

View reviewed changes

alexanderbez reviewed Aug 4, 2022

View reviewed changes

error msg update

ae4aa1a

p0mvn reviewed Aug 5, 2022

View reviewed changes

ValarDragon and others added 10 commits August 5, 2022 09:54

rename to listeners.go

1d02a33

Add comment for record creation on pool creation

654c82a

api_test requested updates

f5a6008

Update x/gamm/twap/export_test.go

aa3d79f

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

var block & denom0 / denom1 usage

ccf67b8

Improve ComputeArithmeticTwap comment

8842068

Review lint

36db073

Fix dangling comment

203018b

Delete obsolete TODO

8ebcac3

Delete InterpolateRecord, just call recordWithUpdatedAccumulators

29bd221

Clarify accumulator setting in api test

3e8fb91

p0mvn approved these changes Aug 5, 2022

View reviewed changes

ValarDragon and others added 3 commits August 5, 2022 13:03

Delete extra line

b568faa

Apply code review doc update

6b60e70

Co-authored-by: Roman <roman@osmosis.team>

Add test case roman pointed out as missinsg

e800f9f

ValarDragon merged commit 8cc17f7 into main Aug 5, 2022

ValarDragon deleted the dev/twap_wip branch August 5, 2022 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TWAP logic #2168

TWAP logic #2168

ValarDragon commented Jul 20, 2022 •

edited

Loading

faddat commented Jul 21, 2022

ValarDragon commented Jul 21, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Aug 2, 2022

ValarDragon Aug 4, 2022 •

edited

Loading

p0mvn Aug 4, 2022

ValarDragon Aug 5, 2022

ValarDragon Aug 5, 2022

p0mvn Aug 5, 2022

p0mvn left a comment

p0mvn Aug 4, 2022

p0mvn Aug 4, 2022

ValarDragon Aug 5, 2022

p0mvn Aug 4, 2022

p0mvn left a comment

p0mvn Aug 5, 2022

ValarDragon Aug 5, 2022 •

edited

Loading

ValarDragon Aug 5, 2022

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

ValarDragon Aug 5, 2022 •

edited

Loading

p0mvn Aug 5, 2022 •

edited

Loading

ValarDragon commented Aug 5, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

ValarDragon commented Aug 5, 2022


		"accumulator = 10OneSec, t=100s. 0 base accum (asset 1)": testCaseFromDeltasAsset1(sdk.ZeroDec(), OneSec.MulInt64(10), 100time.Second, sdk.NewDecWithPrec(1, 1)),

		// TODO: Overflow, rounding

TWAP logic #2168

TWAP logic #2168

Conversation

ValarDragon commented Jul 20, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

faddat commented Jul 21, 2022

ValarDragon commented Jul 21, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 4, 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

p0mvn 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

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon commented Aug 5, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 5, 2022

ValarDragon commented Jul 20, 2022 •

edited

Loading

ValarDragon commented Jul 21, 2022 •

edited

Loading

ValarDragon Aug 4, 2022 •

edited

Loading

ValarDragon Aug 5, 2022 •

edited

Loading

ValarDragon Aug 5, 2022 •

edited

Loading

p0mvn Aug 5, 2022 •

edited

Loading

ValarDragon commented Aug 5, 2022 •

edited

Loading