feat(x/twap): implement reusable TWAP API

[p0mvn]

Background

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.

Consider the current implementation of GetArithmeticTWAP():

func (k Keeper) GetArithmeticTwap(
	ctx sdk.Context,
	poolId uint64,
	baseAssetDenom string,
	quoteAssetDenom string,
	startTime time.Time,
	endTime time.Time,
) (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) // difference 1
	} else if endTime.After(ctx.BlockTime()) {
		return sdk.Dec{}, types.EndTimeInFutureError{EndTime: endTime, BlockTime: ctx.BlockTime()}
	}
	startRecord, err := k.getInterpolatedRecord(ctx, poolId, startTime, baseAssetDenom, quoteAssetDenom)
	if err != nil {
		return sdk.Dec{}, err
	}
	endRecord, err := k.getInterpolatedRecord(ctx, poolId, endTime, baseAssetDenom, quoteAssetDenom)
	if err != nil {
		return sdk.Dec{}, err
	}
	return computeArithmeticTwap(startRecord, endRecord, quoteAssetDenom) // difference 2
}

With the GetGeometricTWAP(), all code would be similar besides the lines marked as "difference" in comments. Even the lines that differ would have the same API.

That is, in the naive code-duplication implementation:

• GetArithmeticTwapToNow would become GetGeometricTwapToNow
• computeArithmeticTwap would become computeGeometricTwap

taking the same arguments.

Suggested Design

To avoid code duplication we should consider using strategy pattern.

// x/twap/strategy.go
type twapStrategy interface {
	computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error)
}

type arithmetic struct {
	keeper Keeper
}

var _ twapStrategy = &arithmetic{}


func (s *arithmetic) computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error) {
        // move implementation from computeArithmeticTwap
	// return result
}

Then, we can extract a reusable function that takes strategy as a parameter:

// x/twap/api.go

func (k Keeper) getTwap(
	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 strategy.getTwapToNow(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime)
	} else if endTime.After(ctx.BlockTime()) {
		return sdk.Dec{}, types.EndTimeInFutureError{EndTime: endTime, BlockTime: ctx.BlockTime()}
	}
	startRecord, err := k.getInterpolatedRecord(ctx, poolId, startTime, baseAssetDenom, quoteAssetDenom)
	if err != nil {
		return sdk.Dec{}, err
	}
	endRecord, err := k.getInterpolatedRecord(ctx, poolId, endTime, baseAssetDenom, quoteAssetDenom)
	if err != nil {
		return sdk.Dec{}, err
	}
	return computeTwap(startRecord, endRecord, quoteAssetDenom, strategy)
}

and the existing GetArithmeticTwap exported function would become:

// x/twap/api.go

func (k Keeper) GetArithmeticTwap(
	ctx sdk.Context,
	poolId uint64,
	baseAssetDenom string,
	quoteAssetDenom string,
	startTime time.Time,
	endTime time.Time,
) (sdk.Dec, error) {
	arithmeticStrategy := &arithmetic{k}
	return k.getTwap(ctx, poolId, baseAssetDenom, quoteAssetDenom, startTime, endTime, arithmeticStrategy)
}

Similarly, we should be able to introduce geometric strategy and refactor GetArithmaticTwapToNow

Acceptance Criteria

1. twapStrategy interface is introduced in `strategy.go
2. arithmetic implementation is added to strategy.go
   • tested with coverage maximized
3. geometric implementation is added to strategy.go
   • tested with coverage maximized
4. Refactor computeTwap to take twapStrategy instead of twapType.
   • move logic from computeGeometrictTwap to its respective strategy's computeTwap.
   • port tests / add new if needed
   • move logic from computeArithmeticTwap to its respective strategy's computeTwap.
   • port tests / add new if needed
5. getTwap function is introduced to api.go
   • thoroughly tested, having old tests ported from getArithmaticTwap
   • run these tests for both arithmetic and geometric strategies, minimizing code duplication in tests
6. getArithmeticTwap utilizes arithmetic strategy and calls getTwap
   • only have a few tests covering all code branches while porting the core tests to getTwap
7. getTwapToNow function is introduced to api.go
   • thoroughly tested, having old tests ported from getArithmaticTwapToNow
   • • run these tests for both arithmetic and Geometric strategies, minimizing code duplication in tests
8. getArithmeticTwapToNow utilizes arithmetic strategy and calls getTwapToNow
   • only have a few tests covering all code branches while porting the core tests to getTwap
9. Similarly, introduce getGeometricTwap and getGeometricTwapToNow

[p0mvn]

The PR split could be done in the following way to limit the scope and simplify reviews:

• Points 1-3
• Points 4-5
• Points 6-7
• Point 8

First PR (1-3) can be started immediately while the rest is blocked on #3420

[p0mvn]

@stackman27 please let me know if you're interested in picking this up

Would also appreciate reviews of the proposal from anyone to agree on the direction

[stackman27]

Hi @p0mvn yea i can give points 1-3 a shot

[nicolaslara]

This looks good to me. I don't expect other TWAP strategies to exist, but I like the abstraction pattern so that we can use it on other parts of the codebase

[p0mvn]

Sweet, thanks for reviewing @nicolaslara .

Could you please take a look at the first PR towards this when you have the time: #3529

[p0mvn]

@stackman27 FYI, updated the A/C by adding a step 4 to cover changes we discussed in #3529

[p0mvn]

closed by #3696 , thanks @stackman27