Move spot price safety checks to keeper layer #2508

ValarDragon · 2022-08-25T12:13:42Z

Closes: #2507

What is the purpose of the change

Move safety checks for spot price against internal logic issues, sig fig rounding, and overflow to the keeper layer
Fix bug where grpc query uses pool spot price instead of keeper method
Document these safety guarantees

~~This is currently blocked on #2405 , as create pool panics right now~~

Testing and Verifying

This change should be moving + adding new test cases in total, each more modularly guaranteeing a component.

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 (TODO)
How is the feature or change documented? specification (x/<module>/spec/) (TODO)

app/apptesting/gamm.go

x/gamm/types/constants.go

Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>

mattverse

Left some questions and comments, otherwise lgtm!

x/gamm/keeper/pool_service.go

mattverse · 2022-09-02T08:01:48Z

x/gamm/pool-models/balancer/pool.go

 	// spot_price = (Base_supply / Weight_base) / (Quote_supply / Weight_quote)
 	// spot_price = (weight_quote / weight_base) * (base_supply / quote_supply)
 	invWeightRatio := quote.Weight.ToDec().Quo(base.Weight.ToDec())
 	supplyRatio := base.Token.Amount.ToDec().Quo(quote.Token.Amount.ToDec())
-	fullRatio := supplyRatio.Mul(invWeightRatio)


confirmed this logic has been correctly moved

mattverse · 2022-09-02T08:04:29Z

x/twap/types/utils.go

@@ -10,6 +10,8 @@ import (
 	"github.com/osmosis-labs/osmosis/v11/osmoutils"
 )

+var MaxSpotPrice = sdk.NewDec(2).Power(128).Sub(sdk.OneDec())


what's the reason we don't import it / use it directly from gamm module but declare it twice both in twap module and gamm module?

imports basically, introduces a dependency on its types package. (Maybe this doesn't make sense though)

I think it is ok for the two to diverge with time

mattverse · 2022-09-02T08:12:27Z

x/twap/logic.go

@@ -53,7 +53,12 @@ func getSpotPrices(ctx sdk.Context, k types.AmmInterface, poolId uint64, denom0,
 			sp1 = sdk.ZeroDec()
 		}
 	}
-	// if sp0.GT(gammtypes.MaxSpotPrice)
+	if sp0.GT(types.MaxSpotPrice) {


this is just a safety check since CalculateSpotPrice shouldnt return values over types.MaxSpotPrice right?

yup! In case a different spot price backend gets put in

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

alexanderbez

LGTM!

p0mvn

Great work!

Before merging, I would like to understand whether we need to update the latestErrTime when getting spot prices in TWAP and assuming a different backend.

Also, to discuss and plan increasing test coverage for the keeper's CalculateSpotPrice

x/twap/logic.go

x/gamm/keeper/pool_service.go

p0mvn · 2022-09-02T18:26:45Z

osmomath/sigfig_round.go

@@ -6,6 +6,9 @@ var pointOne = sdk.OneDec().QuoInt64(10)

 // SigFigRound rounds to a specified significant figure.
 func SigFigRound(d sdk.Dec, tenToSigFig sdk.Int) sdk.Dec {
+	if d.IsZero() {
+		return d


We are completely missing tests for this function. We should add them

Agreed, should be done in a separate PR, and is not v12 blocking

p0mvn · 2022-09-02T18:48:14Z

x/gamm/keeper/pool_service.go

+	if spotPrice.GT(types.MaxSpotPrice) {
+		return types.MaxSpotPrice, types.ErrSpotPriceOverflow
+	} else if !spotPrice.IsPositive() {
+		return sdk.Dec{}, types.ErrSpotPriceInternal


It seems that the only time we are hitting this branch during the tests is when we attempt to set up a pool. Can we have a test case directly covering this branch?

I don't think this matters that much to directly test, as a blocker for this PR. Its a safety check to give a clear pre-condition for successful results.

p0mvn · 2022-09-02T18:48:59Z

x/gamm/keeper/pool_service.go

+	// defer to catch panics, in case something internal overflows.
+	defer func() {
+		if r := recover(); r != nil {
+			spotPrice = sdk.Dec{}


We don't have a test case covering this branch. Is it possible to add it?

Not easily, I tried.

Actually I tried harder, it is!

Thanks for pointing this out :)

x/gamm/keeper/pool_service_test.go

Move spot price safety checks to keeper layer + safety checks

bd4b934

github-actions bot added C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. labels Aug 25, 2022

ValarDragon changed the title ~~Move spot price safety checks to keeper layer + safety checks~~ Move spot price safety checks to keeper layer Aug 25, 2022

ValarDragon mentioned this pull request Aug 25, 2022

Make GAMM queries follow TWAP query code format #2509

Open

6 tasks

nicolaslara reviewed Aug 26, 2022

View reviewed changes

app/apptesting/gamm.go Outdated Show resolved Hide resolved

ValarDragon commented Aug 29, 2022

View reviewed changes

x/gamm/types/constants.go Outdated Show resolved Hide resolved

ValarDragon mentioned this pull request Sep 1, 2022

Make TWAP safe under spot price errors #2470

Merged

ValarDragon and others added 3 commits September 1, 2022 14:40

Merge branch 'main' into dev/spot_price_keeper_layer_move

8d5ba79

Fix safety issues

38eaaec

Update app/apptesting/gamm.go

6f1c43d

Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>

czarcas7ic added this to the V12 Blockers milestone Sep 1, 2022

czarcas7ic assigned ValarDragon Sep 1, 2022

Update constant, duplicate bounds check in TWAP

fbed1d8

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

Fix existing tests

971291c

ValarDragon marked this pull request as ready for review September 1, 2022 20:22

ValarDragon requested a review from a team September 1, 2022 20:22

Return max spot price when overflowing

0cdc109

mattverse approved these changes Sep 2, 2022

View reviewed changes

Update x/gamm/keeper/pool_service.go

79999a7

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

alexanderbez approved these changes Sep 2, 2022

View reviewed changes

p0mvn reviewed Sep 2, 2022

View reviewed changes

ValarDragon added 2 commits September 3, 2022 00:27

Set last error time in particular case

9687ec3

Add defer case test

5b55367

ValarDragon commented Sep 3, 2022

View reviewed changes

x/gamm/keeper/pool_service_test.go Outdated Show resolved Hide resolved

Update x/gamm/keeper/pool_service_test.go

0221096

ValarDragon merged commit 83ed9ea into main Sep 3, 2022

ValarDragon deleted the dev/spot_price_keeper_layer_move branch September 3, 2022 12:54

p0mvn mentioned this pull request Sep 6, 2022

test: SigFigRound #2621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move spot price safety checks to keeper layer #2508

Move spot price safety checks to keeper layer #2508

ValarDragon commented Aug 25, 2022 •

edited

Loading

mattverse left a comment

mattverse Sep 2, 2022

mattverse Sep 2, 2022

ValarDragon Sep 2, 2022

mattverse Sep 2, 2022

ValarDragon Sep 2, 2022

alexanderbez left a comment

p0mvn left a comment

p0mvn Sep 2, 2022

ValarDragon Sep 2, 2022 •

edited

Loading

p0mvn Sep 2, 2022

ValarDragon Sep 2, 2022 •

edited

Loading

p0mvn Sep 2, 2022

ValarDragon Sep 2, 2022

ValarDragon Sep 3, 2022

Move spot price safety checks to keeper layer #2508

Move spot price safety checks to keeper layer #2508

Conversation

ValarDragon commented Aug 25, 2022 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez 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

ValarDragon Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Sep 2, 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

ValarDragon commented Aug 25, 2022 •

edited

Loading

ValarDragon Sep 2, 2022 •

edited

Loading

ValarDragon Sep 2, 2022 •

edited

Loading