-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Reimplement & test GetCandidateRoute algorithm #379
Changes from all commits
dbbcf32
60d83a6
ee6a2b5
cb94185
88ad17b
4658dac
fa5e5af
399f817
2ad9587
515580e
63f991a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package domain | ||
|
||
import ( | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/osmosis-labs/sqs/sqsdomain" | ||
) | ||
|
||
// CandidateRouteSearchOptions represents the options for finding candidate routes. | ||
type CandidateRouteSearchOptions struct { | ||
// MaxRoutes is the maximum number of routes to find. | ||
MaxRoutes int | ||
// MaxPoolsPerRoute is the maximum number of pools to consider for each route. | ||
MaxPoolsPerRoute int | ||
// MinPoolLiquidityCap is the minimum liquidity cap for a pool to be considered. | ||
MinPoolLiquidityCap uint64 | ||
} | ||
|
||
// CandidateRouteSearcher is the interface for finding candidate routes. | ||
type CandidateRouteSearcher interface { | ||
// FindCandidateRoutes finds candidate routes for a given tokenIn and tokenOutDenom | ||
// using the given options. | ||
// Returns the candidate routes and an error if any. | ||
FindCandidateRoutes(tokenIn sdk.Coin, tokenOutDenom string, options CandidateRouteSearchOptions) (sqsdomain.CandidateRoutes, error) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package mocks | ||
|
||
import ( | ||
"github.com/cosmos/cosmos-sdk/types" | ||
"github.com/osmosis-labs/sqs/domain" | ||
"github.com/osmosis-labs/sqs/sqsdomain" | ||
) | ||
|
||
type CandidateRouteFinderMock struct { | ||
Routes sqsdomain.CandidateRoutes | ||
Error error | ||
} | ||
|
||
var _ domain.CandidateRouteSearcher = CandidateRouteFinderMock{} | ||
|
||
// FindCandidateRoutes implements domain.CandidateRouteSearcher. | ||
func (c CandidateRouteFinderMock) FindCandidateRoutes(tokenIn types.Coin, tokenOutDenom string, options domain.CandidateRouteSearchOptions) (sqsdomain.CandidateRoutes, error) { | ||
return c.Routes, c.Error | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ import ( | |
"fmt" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/osmosis-labs/sqs/domain" | ||
"github.com/osmosis-labs/sqs/domain/mvc" | ||
"github.com/osmosis-labs/sqs/log" | ||
"github.com/osmosis-labs/sqs/sqsdomain" | ||
"go.uber.org/zap" | ||
|
@@ -144,11 +146,23 @@ func GetCandidateRoutes(pools []sqsdomain.PoolI, tokenIn sdk.Coin, tokenOutDenom | |
return validateAndFilterRoutes(routes, tokenIn.Denom, logger) | ||
} | ||
|
||
// GetCandidateRoutesNew new algorithm for demo purposes. | ||
// Note: implementation is for demo purposes and is to be further optimized. | ||
// TODO: spec, unit tests via https://linear.app/osmosis/issue/DATA-250/[candidaterouteopt]-reimplement-and-test-getcandidateroute-algorithm | ||
func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sdk.Coin, tokenOutDenom string, maxRoutes, maxPoolsPerRoute int, minPoolLiquidityCap uint64, logger log.Logger) (sqsdomain.CandidateRoutes, error) { | ||
routes := make([][]candidatePoolWrapper, 0, maxRoutes) | ||
type candidateRouteFinder struct { | ||
candidateRouteDataHolder mvc.CandidateRouteSearchDataHolder | ||
logger log.Logger | ||
} | ||
|
||
var _ domain.CandidateRouteSearcher = candidateRouteFinder{} | ||
|
||
func NewCandidateRouteFinder(candidateRouteDataHolder mvc.CandidateRouteSearchDataHolder, logger log.Logger) candidateRouteFinder { | ||
return candidateRouteFinder{ | ||
candidateRouteDataHolder: candidateRouteDataHolder, | ||
logger: logger, | ||
} | ||
} | ||
|
||
// FindCandidateRoutes implements domain.CandidateRouteFinder. | ||
func (c candidateRouteFinder) FindCandidateRoutes(tokenIn sdk.Coin, tokenOutDenom string, options domain.CandidateRouteSearchOptions) (sqsdomain.CandidateRoutes, error) { | ||
routes := make([][]candidatePoolWrapper, 0, options.MaxRoutes) | ||
|
||
// Preallocate constant visited map size to avoid reallocations. | ||
// TODO: choose the best size for the visited map. | ||
|
@@ -158,9 +172,9 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
// Preallocate constant queue size to avoid dynamic reallocations. | ||
// TODO: choose the best size for the queue. | ||
queue := make([][]candidatePoolWrapper, 0, 100) | ||
queue = append(queue, make([]candidatePoolWrapper, 0, maxPoolsPerRoute)) | ||
queue = append(queue, make([]candidatePoolWrapper, 0, options.MaxPoolsPerRoute)) | ||
|
||
for len(queue) > 0 && len(routes) < maxRoutes { | ||
for len(queue) > 0 && len(routes) < options.MaxRoutes { | ||
currentRoute := queue[0] | ||
queue[0] = nil // Clear the slice to avoid holding onto references | ||
queue = queue[1:] | ||
|
@@ -173,12 +187,15 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
currenTokenInDenom = lastPool.TokenOutDenom | ||
} | ||
|
||
rankedPools, ok := poolsByDenom[currenTokenInDenom] | ||
if !ok { | ||
rankedPools, err := c.candidateRouteDataHolder.GetRankedPoolsByDenom(currenTokenInDenom) | ||
if err != nil { | ||
return sqsdomain.CandidateRoutes{}, err | ||
} | ||
if len(rankedPools) == 0 { | ||
return sqsdomain.CandidateRoutes{}, fmt.Errorf("no pools found for denom %s", currenTokenInDenom) | ||
} | ||
|
||
for i := 0; i < len(rankedPools) && len(routes) < maxRoutes; i++ { | ||
for i := 0; i < len(rankedPools) && len(routes) < options.MaxRoutes; i++ { | ||
// Unsafe cast for performance reasons. | ||
// nolint: forcetypeassert | ||
pool := (rankedPools[i]).(*sqsdomain.PoolWrapper) | ||
|
@@ -188,7 +205,7 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
continue | ||
} | ||
|
||
if pool.GetLiquidityCap().Uint64() < minPoolLiquidityCap { | ||
if pool.GetLiquidityCap().Uint64() < options.MinPoolLiquidityCap { | ||
visited[poolID] = struct{}{} | ||
// Skip pools that have less liquidity than the minimum required. | ||
continue | ||
|
@@ -247,9 +264,12 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
continue | ||
} | ||
|
||
_, ok := poolsByDenom[denom] | ||
if !ok { | ||
logger.Debug("no pools found for denom in candidate route search", zap.String("denom", denom)) | ||
rankedPools, err := c.candidateRouteDataHolder.GetRankedPoolsByDenom(currenTokenInDenom) | ||
if err != nil { | ||
return sqsdomain.CandidateRoutes{}, err | ||
} | ||
if len(rankedPools) == 0 { | ||
c.logger.Debug("no pools found for denom in candidate route search", zap.String("denom", denom)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a random idea, would it be any benefit feed this data to our observability service? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for suggestion!. Specifically for this one - I don't think so. The reason is that this branch may be taken in the expected cases. It also happens quite frequently. Adding some counter/alert for this specific case would result in too much noise. However, it might be helpful for debugging, thus, I added a log at the |
||
continue | ||
} | ||
|
||
|
@@ -267,7 +287,7 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
Idx: i, | ||
}) | ||
|
||
if len(newPath) <= maxPoolsPerRoute { | ||
if len(newPath) <= options.MaxPoolsPerRoute { | ||
if hasTokenOut { | ||
routes = append(routes, newPath) | ||
break | ||
|
@@ -284,7 +304,7 @@ func GetCandidateRoutesNew(poolsByDenom map[string][]sqsdomain.PoolI, tokenIn sd | |
} | ||
} | ||
|
||
return validateAndFilterRoutes(routes, tokenIn.Denom, logger) | ||
return validateAndFilterRoutes(routes, tokenIn.Denom, c.logger) | ||
} | ||
|
||
// Pool represents a pool in the decentralized exchange. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package usecase_test | ||
|
||
import ( | ||
"testing" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/osmosis-labs/osmosis/osmomath" | ||
"github.com/osmosis-labs/sqs/domain" | ||
"github.com/osmosis-labs/sqs/router/usecase/routertesting" | ||
) | ||
|
||
// Microbenchmark for the GetSplitQuote function. | ||
func BenchmarkCandidateRouteSearcher(b *testing.B) { | ||
// This is a hack to be able to use test suite helpers with the benchmark. | ||
// We need to set testing.T for assertings within the helpers. Otherwise, it would block | ||
s := RouterTestSuite{} | ||
s.SetT(&testing.T{}) | ||
|
||
mainnetState := s.SetupMainnetState() | ||
|
||
usecase := s.SetupRouterAndPoolsUsecase(mainnetState, routertesting.WithLoggerDisabled()) | ||
|
||
var ( | ||
amountIn = osmomath.NewInt(1_000_000) | ||
tokenIn = sdk.NewCoin(UOSMO, amountIn) | ||
tokenOutDenom = ATOM | ||
) | ||
|
||
routerConfig := usecase.Router.GetConfig() | ||
candidateRouteOptions := domain.CandidateRouteSearchOptions{ | ||
MaxRoutes: routerConfig.MaxRoutes, | ||
MaxPoolsPerRoute: routerConfig.MaxPoolsPerRoute, | ||
MinPoolLiquidityCap: 1, | ||
} | ||
|
||
b.ResetTimer() | ||
|
||
// Run the benchmark | ||
for i := 0; i < b.N; i++ { | ||
// System under test | ||
_, err := usecase.CandidateRouteSearcher.FindCandidateRoutes(tokenIn, tokenOutDenom, candidateRouteOptions) | ||
s.Require().NoError(err) | ||
if err != nil { | ||
b.Errorf("FindCandidateRoutes returned an error: %v", err) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be just a personal preference, but now
options.MaxRoute
reads much better thanmaxRoutes
, and even better, we have documented everything in the single place!