TWAP proto, types package, and osmoutils #2175

ValarDragon · 2022-07-20T21:16:35Z

What is the purpose of the change

Smaller PR to extract complexity in review out of #2168 .

Brief Changelog

Adds a number of helper methods to osmoutils
Adds TwapRecord proto struct
Adds Twap types package

Testing and Verifying

This change adds tests for the types directory keys functions.

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? N/A, should come in logic PR, or other incremental updates

ValarDragon · 2022-07-20T21:21:15Z

Sorry the diff is so large 😅 , 900 of the lines are generated proto at least

At least its better than having the 1350 lines of periphery in the core logic PR

czarcas7ic

This looks good, only had a few questions!

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

x/gamm/twap/types/keys.go

x/gamm/twap/types/utils.go

x/gamm/twap/types/expected_interfaces.go

x/gamm/twap/types/keys.go

osmoutils/slice_helper.go

osmoutils/store_helper.go

p0mvn · 2022-07-21T00:58:48Z

x/gamm/twap/types/utils.go

+
+func NewTwapRecord(k AmmInterface, ctx sdk.Context, poolId uint64, denom0 string, denom1 string) TwapRecord {
+	if !(denom0 > denom1) {
+		panic(fmt.Sprintf("precondition denom0 > denom1 not satisfied. denom0 %s | denom1 %s", denom0, denom1))


Can we return an error here instead of panicking for testability?

Agreed, changing this

x/gamm/twap/types/utils_test.go

p0mvn · 2022-07-21T01:01:24Z

x/gamm/twap/types/utils.go

+	// sanity check
+	for i := 0; i < numPairs; i++ {
+		if pairGT[i] == pairLT[i] {
+			panic("input had duplicated denom")


Consider returning error for easier testing

Thanks for adding a test

nit: I still think that for this specific case error makes more than panic so that we can assert that a panic is coming exactly from here.

Otherwise, what if in the future this starts panicking in another location, then the test would be a false pass

x/gamm/twap/types/utils.go

x/gamm/twap/types/keys.go

ValarDragon · 2022-07-21T07:21:58Z

All comments resolved, except for panic <> error changes I believe!

(I guess no tests for store_helpers -- can we do that in a subsequent PR, just to get the main PR diff down. The usages in twap have unit tests in context already)

alexanderbez · 2022-07-21T09:09:34Z

x/gamm/twap/types/keys.go

+var mostRecentTWAPsNoSeparator = "recent_twap"
+var historicalTWAPTimeIndexNoSeparator = "historical_time_index"
+var historicalTWAPPoolIndexNoSeparator = "historical_pool_index"
+
+var mostRecentTWAPsPrefix = mostRecentTWAPsNoSeparator + KeySeparator
+var historicalTWAPTimeIndexPrefix = historicalTWAPTimeIndexNoSeparator + KeySeparator
+var historicalTWAPPoolIndexPrefix = historicalTWAPPoolIndexNoSeparator + KeySeparator


nit: can we group these?

x/gamm/twap/types/keys.go

alexanderbez · 2022-07-21T09:12:35Z

x/gamm/twap/types/expected_interfaces.go

+import sdk "github.com/cosmos/cosmos-sdk/types"
+
+// AmmInterface is the functionality needed from a given pool ID, in order to maintain records and serve TWAPs.
+type AmmInterface interface {


just a suggestion :)

Suggested change

type AmmInterface interface {

type AMMInterface interface {

mattverse

🌮

mattverse · 2022-07-21T12:40:38Z

osmoutils/store_helper.go

+	return keys
+}
+
+func GatherValuesFromStore[T any](storeObj store.KVStore, keyStart []byte, keyEnd []byte, parseValue func([]byte) (T, error)) ([]T, error) {


Ah we can replace so much existing code and remove so much unnecessary duplocation with this single method :)

Yup! And then we can do generic optimizations here as well

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

mattverse · 2022-07-21T13:14:57Z

x/gamm/twap/types/keys.go

+var historicalTWAPTimeIndexNoSeparator = "historical_time_index"
+var historicalTWAPPoolIndexNoSeparator = "historical_pool_index"
+
+var mostRecentTWAPsPrefix = mostRecentTWAPsNoSeparator + KeySeparator


Is there a reason this is plural?

x/gamm/twap/types/utils.go

p0mvn

LGTM, awesome work on TWAP so far @ValarDragon

I bumped up some comments that seem unaddressed or with no reply. However, I think all of them are minor / nits and should not be blocking this.

UPDATE: noticed that an issue is created so removed my earlier comment about an issue for osmoutils tests. Thanks!

p0mvn · 2022-07-21T17:42:13Z

x/gamm/twap/types/expected_interfaces.go

-// using the specified pool.
-// E.g. if pool 1 traded 2 atom for 3 osmo, the quote asset was atom, and the base asset was osmo,
-// this would return 1.5. (Meaning that 1 atom costs 1.5 osmo)
+// AmmInterface is the functionality needed from a given pool ID, in order to maintain records and serve TWAPs.
 type AmmInterface interface {
 	GetPoolDenoms(ctx sdk.Context, poolId uint64) (denoms []string, err error)


godoc for GetPoolDenoms please

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

p0mvn · 2022-07-21T17:43:55Z

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

+// A TWAP record should be indexed in state by pool_id, (asset pair), timestamp
+// The asset pair assets should be lexicographically sorted.
+// Technically (pool_id, asset_0_denom, asset_1_denom, height) do not need to
+// appear in the struct however we view this as the wrong performance tradeoff


p0mvn · 2022-07-21T17:44:44Z

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

+// The asset pair assets should be lexicographically sorted.
+// Technically (pool_id, asset_0_denom, asset_1_denom, height) do not need to
+// appear in the struct however we view this as the wrong performance tradeoff
+// given SDK today. Would rather we optimize for readability and correctness,


Thanks, please consider including this in the comment. Might be useful for future readers

proto/osmosis/gamm/twap/v1beta1/twap_record.proto

x/gamm/twap/types/keys.go

p0mvn · 2022-07-21T18:08:37Z

x/gamm/twap/types/keys_test.go

+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			gotTimeKey := FormatHistoricalTimeIndexTWAPKey(tt.time, tt.poolId, tt.denom1, tt.denom2)


Why not?

From:

osmosis/x/gamm/twap/types/keys.go

Lines 60 to 62 in 6924e57

if len(s) != 5 || s[0] != historicalTWAPTimeIndexNoSeparator {

panic("Called ParseTimeFromHistoricalTimeIndexKey on incorrectly formatted key")

}

What if we give bytes of invalid length to ParseTimeFromHistoricalTimeIndexKey?

p0mvn · 2022-07-21T18:19:47Z

x/gamm/twap/types/keys_test.go

+			Time:                        baseTime,
+			P0LastSpotPrice:             sdk.NewDecWithPrec(1, 5),
+			P1LastSpotPrice:             sdk.NewDecWithPrec(2, 5), // inconsistent value
+			P0ArithmeticTwapAccumulator: sdk.ZeroDec(),


Up to you, I don't think this is critical

p0mvn · 2022-07-21T18:22:21Z

x/gamm/twap/types/utils.go

+	// sanity check
+	for i := 0; i < numPairs; i++ {
+		if pairGT[i] == pairLT[i] {
+			panic("input had duplicated denom")


Thanks for adding a test

nit: I still think that for this specific case error makes more than panic so that we can assert that a panic is coming exactly from here.

Otherwise, what if in the future this starts panicking in another location, then the test would be a false pass

x/gamm/twap/types/utils.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

… dev/twap_types

ValarDragon · 2022-07-22T03:39:45Z

Thanks so much for the review here!

czarcas7ic · 2022-08-20T21:37:39Z

x/gamm/twap/types/utils_test.go

+		"basicRev": {[]string{"B", "A"}, []string{"B"}, []string{"A"}, false},
+		// AB > A
+		"prefixed": {[]string{"A", "AB"}, []string{"AB"}, []string{"A"}, false},
+		"basic-3":  {[]string{"A", "B", "C"}, []string{"C", "C", "B"}, []string{"B", "A", "A"}, false},


I don't understand the basic-3 test case. How is the the resulting GT pair and LT Pair the correct unique denom pairs

Sorts by gt symbol first then lt symbol

So
C,b
C,a
B,a

Extract osmoutils, proto & types from main TWAP PR

e6ec19c

ValarDragon requested a review from a team July 20, 2022 21:16

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Jul 20, 2022

Delete merge conflict

0ec532f

Add TestGetAllUniqueDenomPairs

625eebd

czarcas7ic reviewed Jul 20, 2022

View reviewed changes

proto/osmosis/gamm/twap/v1beta1/twap_record.proto Show resolved Hide resolved

x/gamm/twap/types/keys.go Outdated Show resolved Hide resolved

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

p0mvn reviewed Jul 21, 2022

View reviewed changes

Resolve more comments

6924e57

alexanderbez reviewed Jul 21, 2022

View reviewed changes

mattverse approved these changes Jul 21, 2022

View reviewed changes

ValarDragon mentioned this pull request Jul 21, 2022

Make tests for osmoutils/store_helper #2179

Closed

2 tasks

p0mvn mentioned this pull request Jul 21, 2022

refactor(osmoutils): make ReverseSlice mutative #2181

Closed

p0mvn approved these changes Jul 21, 2022

View reviewed changes

ValarDragon mentioned this pull request Jul 21, 2022

TWAP logic #2168

Merged

20 tasks

ValarDragon and others added 5 commits July 21, 2022 22:18

Fix incorrect pool interface (but was a no-op)

936dfcd

Apply suggestions from code review

f345722

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

apply review comment

0f2eb92

Merge branch 'dev/twap_types' of github.com:osmosis-labs/osmosis into…

421c2c9

… dev/twap_types

Make NewTwapRecord return an error

82710d6

ValarDragon merged commit e69e260 into main Jul 22, 2022

ValarDragon deleted the dev/twap_types branch July 22, 2022 03:39

czarcas7ic reviewed Aug 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TWAP proto, types package, and osmoutils #2175

TWAP proto, types package, and osmoutils #2175

ValarDragon commented Jul 20, 2022

ValarDragon commented Jul 20, 2022 •

edited

Loading

czarcas7ic left a comment

p0mvn Jul 21, 2022

ValarDragon Jul 22, 2022

p0mvn Jul 21, 2022

p0mvn Jul 21, 2022

ValarDragon commented Jul 21, 2022 •

edited

Loading

alexanderbez Jul 21, 2022

alexanderbez Jul 21, 2022

mattverse left a comment

mattverse Jul 21, 2022

ValarDragon Jul 21, 2022

mattverse Jul 21, 2022

p0mvn left a comment •

edited

Loading

p0mvn Jul 21, 2022

p0mvn Jul 21, 2022

p0mvn Jul 21, 2022

p0mvn Jul 21, 2022

p0mvn Jul 21, 2022 •

edited

Loading

p0mvn Jul 21, 2022

ValarDragon commented Jul 22, 2022

czarcas7ic Aug 20, 2022

ValarDragon Aug 21, 2022

	if len(s) != 5 \|\| s[0] != historicalTWAPTimeIndexNoSeparator {
	panic("Called ParseTimeFromHistoricalTimeIndexKey on incorrectly formatted key")
	}

TWAP proto, types package, and osmoutils #2175

TWAP proto, types package, and osmoutils #2175

Conversation

ValarDragon commented Jul 20, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon commented Jul 20, 2022 • 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

ValarDragon commented Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse 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

p0mvn left a comment • 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 Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Jul 20, 2022 •

edited

Loading

ValarDragon commented Jul 21, 2022 •

edited

Loading

p0mvn left a comment •

edited

Loading

p0mvn Jul 21, 2022 •

edited

Loading