Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions x/twap/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,40 @@ func (k Keeper) GetArithmeticTwap(
quoteAssetDenom string,
startTime time.Time,
endTime time.Time,
) (sdk.Dec, error) {
arithmeticStrategy := &arithmetic{k}
return k.getTwap(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime, endTime, arithmeticStrategy)
}

// GetArithmeticTwapToNow returns arithmetic twap from start time until the current block time for quote and base
// assets in a given pool.
func (k Keeper) GetArithmeticTwapToNow(
ctx sdk.Context,
poolId uint64,
baseAssetDenom string,
quoteAssetDenom string,
startTime time.Time,
) (sdk.Dec, error) {
arithmeticStrategy := &arithmetic{k}
return k.getTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime, arithmeticStrategy)
}

// getTwap computes and returns twap from the start time until the end time. The type
// of twap returned depends on the strategy given and can be either arithmetic or geometric.
func (k Keeper) getTwap(
stackman27 marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context,
poolId uint64,
baseAssetDenom string,
quoteAssetDenom string,
startTime time.Time,
endTime time.Time,
strategy twapStrategy,
) (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 k.getTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime, strategy)
} else if endTime.After(ctx.BlockTime()) {
return sdk.Dec{}, types.EndTimeInFutureError{EndTime: endTime, BlockTime: ctx.BlockTime()}
}
Expand All @@ -68,17 +96,19 @@ func (k Keeper) GetArithmeticTwap(
if err != nil {
return sdk.Dec{}, err
}
return computeTwap(startRecord, endRecord, quoteAssetDenom, arithmeticTwapType)

return strategy.computeTwap(startRecord, endRecord, quoteAssetDenom)
}

// GetArithmeticTwapToNow returns GetArithmeticTwap on the input, with endTime being fixed to ctx.BlockTime()
// This function does not mutate records.
func (k Keeper) GetArithmeticTwapToNow(
// getTwapToNow computes and returns twap from the start time until the current block time. The type
// of twap returned depends on the strategy given and can be either arithmetic or geometric.
func (k Keeper) getTwapToNow(
stackman27 marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context,
poolId uint64,
baseAssetDenom string,
quoteAssetDenom string,
startTime time.Time,
strategy twapStrategy,
) (sdk.Dec, error) {
if startTime.After(ctx.BlockTime()) {
return sdk.Dec{}, types.StartTimeAfterEndTimeError{StartTime: startTime, EndTime: ctx.BlockTime()}
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update #3514 to add it there then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I will update today

}

// GetBeginBlockAccumulatorRecord returns a TwapRecord struct corresponding to the state of pool `poolId`
// as of the beginning of the block this is called on.
// This uses the state of the beginning of the block, as if there were swaps since the block has started,
// these swaps have had no time to be arbitraged back.
// This accumulator can be stored, to compute wider ranged twaps.
func (k Keeper) GetBeginBlockAccumulatorRecord(ctx sdk.Context, poolId uint64, asset0Denom string, asset1Denom string) (types.TwapRecord, error) {
return k.getMostRecentRecord(ctx, poolId, asset0Denom, asset1Denom)
}
24 changes: 24 additions & 0 deletions x/twap/strategy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package twap

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/osmosis-labs/osmosis/v13/x/twap/types"
)

// twapStrategy is an interface for computing TWAPs.
// We have two strategies implementing the interface - arithmetic and geometric.
// We expose a common TWAP API to reduce duplication and avoid complexity.
type twapStrategy interface {
stackman27 marked this conversation as resolved.
Show resolved Hide resolved
// computeTwap calculates the TWAP with specific startRecord and endRecord.
computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

type arithmetic struct {
keeper Keeper
}

var _ twapStrategy = &arithmetic{}

func (s *arithmetic) computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error) {
return computeTwap(startRecord, endRecord, quoteAsset, arithmeticTwapType)
}