From 452e5c73a93291fd63dfd898e20f8d6ad2ad6198 Mon Sep 17 00:00:00 2001 From: tamirms Date: Mon, 28 Mar 2022 09:50:17 -0400 Subject: [PATCH] Code review fixes --- services/horizon/internal/actions/path.go | 4 ++-- services/horizon/internal/actions_path_test.go | 4 ++-- services/horizon/internal/config.go | 5 +++-- services/horizon/internal/flags.go | 8 ++------ .../internal/integration/parameters_test.go | 6 +++--- services/horizon/internal/paths/ratelimit.go | 16 ++++++++-------- .../horizon/internal/paths/ratelimit_test.go | 4 ++-- 7 files changed, 22 insertions(+), 25 deletions(-) diff --git a/services/horizon/internal/actions/path.go b/services/horizon/internal/actions/path.go index 16a15aebc5..fca06fa970 100644 --- a/services/horizon/internal/actions/path.go +++ b/services/horizon/internal/actions/path.go @@ -167,7 +167,7 @@ func (handler FindPathsHandler) GetResource(w HeaderWriter, r *http.Request) (in switch err { case simplepath.ErrEmptyInMemoryOrderBook: return nil, horizonProblem.StillIngesting - case paths.ErrLimitExceeded: + case paths.ErrRateLimitExceeded: return nil, horizonProblem.ServerOverCapacity default: if err != nil { @@ -345,7 +345,7 @@ func (handler FindFixedPathsHandler) GetResource(w HeaderWriter, r *http.Request switch err { case simplepath.ErrEmptyInMemoryOrderBook: return nil, horizonProblem.StillIngesting - case paths.ErrLimitExceeded: + case paths.ErrRateLimitExceeded: return nil, horizonProblem.ServerOverCapacity default: if err != nil { diff --git a/services/horizon/internal/actions_path_test.go b/services/horizon/internal/actions_path_test.go index 4f366c8143..1bb2bc8df6 100644 --- a/services/horizon/internal/actions_path_test.go +++ b/services/horizon/internal/actions_path_test.go @@ -84,9 +84,9 @@ func TestPathActionsLimitExceeded(t *testing.T) { assertions := &test.Assertions{tt.Assert} finder := paths.MockFinder{} finder.On("Find", mock.Anything, mock.Anything, uint(3)). - Return([]paths.Path{}, uint32(0), paths.ErrLimitExceeded).Times(2) + Return([]paths.Path{}, uint32(0), paths.ErrRateLimitExceeded).Times(2) finder.On("FindFixedPaths", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return([]paths.Path{}, uint32(0), paths.ErrLimitExceeded).Times(1) + Return([]paths.Path{}, uint32(0), paths.ErrRateLimitExceeded).Times(1) rh := mockPathFindingClient( tt, diff --git a/services/horizon/internal/config.go b/services/horizon/internal/config.go index 35dcb9fd30..888acae0cd 100644 --- a/services/horizon/internal/config.go +++ b/services/horizon/internal/config.go @@ -49,11 +49,12 @@ type Config struct { MaxPathLength uint // MaxAssetsPerPathRequest is the maximum number of assets considered for `/paths/strict-send` and `/paths/strict-recieve` MaxAssetsPerPathRequest int - // DisablePoolPathFinding configures horizon to run without the path finding endpoint. + // DisablePoolPathFinding configures horizon to run path finding without including liquidity pools + // in the path finding search. DisablePoolPathFinding bool // MaxPathFindingRequests is the maximum number of path finding requests horizon will allow // in a 1-second period. A value of 0 disables the limit. - MaxPathFindingRequests int + MaxPathFindingRequests uint NetworkPassphrase string SentryDSN string diff --git a/services/horizon/internal/flags.go b/services/horizon/internal/flags.go index 6c3afe24c1..d4651e2ef1 100644 --- a/services/horizon/internal/flags.go +++ b/services/horizon/internal/flags.go @@ -395,8 +395,8 @@ func Flags() (*Config, support.ConfigOptions) { &support.ConfigOption{ Name: "max-path-finding-requests", ConfigKey: &config.MaxPathFindingRequests, - OptType: types.Int, - FlagDefault: 0, + OptType: types.Uint, + FlagDefault: uint(0), Required: false, Usage: "The maximum number of path finding requests per second horizon will allow." + " A value of zero (the default) disables the limit.", @@ -728,9 +728,5 @@ func ApplyFlags(config *Config, flags support.ConfigOptions, options ApplyOption return fmt.Errorf("Invalid config: Only one option of --behind-cloudflare and --behind-aws-load-balancer is allowed. If Horizon is behind both, use --behind-cloudflare only.") } - if config.MaxPathFindingRequests < 0 { - return fmt.Errorf("Invalid config: --max-path-finxing-requests must not be negative") - } - return nil } diff --git a/services/horizon/internal/integration/parameters_test.go b/services/horizon/internal/integration/parameters_test.go index ae343d08f7..a117baad37 100644 --- a/services/horizon/internal/integration/parameters_test.go +++ b/services/horizon/internal/integration/parameters_test.go @@ -172,17 +172,17 @@ func TestMaxPathFindingRequests(t *testing.T) { err := test.StartHorizon() assert.NoError(t, err) test.WaitForHorizon() - assert.Equal(t, test.Horizon().Config().MaxPathFindingRequests, 0) + assert.Equal(t, test.Horizon().Config().MaxPathFindingRequests, uint(0)) _, ok := test.Horizon().Paths().(simplepath.InMemoryFinder) assert.True(t, ok) test.Shutdown() }) t.Run("set to 5", func(t *testing.T) { - test := NewParameterTest(t, map[string]string{"max-max-path-finding-requests": "5"}) + test := NewParameterTest(t, map[string]string{"max-path-finding-requests": "5"}) err := test.StartHorizon() assert.NoError(t, err) test.WaitForHorizon() - assert.Equal(t, test.Horizon().Config().MaxPathFindingRequests, 5) + assert.Equal(t, test.Horizon().Config().MaxPathFindingRequests, uint(5)) finder, ok := test.Horizon().Paths().(*paths.RateLimitedFinder) assert.True(t, ok) assert.Equal(t, finder.Limit(), 5) diff --git a/services/horizon/internal/paths/ratelimit.go b/services/horizon/internal/paths/ratelimit.go index 58e59cbf6b..ecb77938ec 100644 --- a/services/horizon/internal/paths/ratelimit.go +++ b/services/horizon/internal/paths/ratelimit.go @@ -10,8 +10,8 @@ import ( ) var ( - // ErrLimitExceeded indicates that the in memory order book is not yet populated - ErrLimitExceeded = errors.New("Empty orderbook") + // ErrRateLimitExceeded indicates that the Finder is not able to fulfill the request due to rate limits. + ErrRateLimitExceeded = errors.New("Rate limit exceeded") ) // RateLimitedFinder is a Finder implementation which limits the number of path finding requests. @@ -22,10 +22,10 @@ type RateLimitedFinder struct { // NewRateLimitedFinder constructs a new RateLimitedFinder which enforces a per // second limit on path finding requests. -func NewRateLimitedFinder(finder Finder, limit int) *RateLimitedFinder { +func NewRateLimitedFinder(finder Finder, limit uint) *RateLimitedFinder { return &RateLimitedFinder{ finder: finder, - limiter: rate.NewLimiter(rate.Limit(limit), limit), + limiter: rate.NewLimiter(rate.Limit(limit), int(limit)), } } @@ -34,16 +34,16 @@ func (f *RateLimitedFinder) Limit() int { return f.limiter.Burst() } -// Find implements the Finder interface and returns ErrLimitExceeded if the +// Find implements the Finder interface and returns ErrRateLimitExceeded if the // RateLimitedFinder is unable to complete the request due to rate limits. func (f *RateLimitedFinder) Find(ctx context.Context, q Query, maxLength uint) ([]Path, uint32, error) { if !f.limiter.Allow() { - return nil, 0, ErrLimitExceeded + return nil, 0, ErrRateLimitExceeded } return f.finder.Find(ctx, q, maxLength) } -// FindFixedPaths implements the Finder interface and returns ErrLimitExceeded if the +// FindFixedPaths implements the Finder interface and returns ErrRateLimitExceeded if the // RateLimitedFinder is unable to complete the request due to rate limits. func (f *RateLimitedFinder) FindFixedPaths( ctx context.Context, @@ -53,7 +53,7 @@ func (f *RateLimitedFinder) FindFixedPaths( maxLength uint, ) ([]Path, uint32, error) { if !f.limiter.Allow() { - return nil, 0, ErrLimitExceeded + return nil, 0, ErrRateLimitExceeded } return f.finder.FindFixedPaths(ctx, sourceAsset, amountToSpend, destinationAssets, maxLength) } diff --git a/services/horizon/internal/paths/ratelimit_test.go b/services/horizon/internal/paths/ratelimit_test.go index d4d210a460..21a1b1351b 100644 --- a/services/horizon/internal/paths/ratelimit_test.go +++ b/services/horizon/internal/paths/ratelimit_test.go @@ -48,7 +48,7 @@ func TestRateLimitedFinder(t *testing.T) { for _, f := range []func(Finder){find, findFixedPaths} { wg.Add(totalCalls) - rateLimitedFinder := NewRateLimitedFinder(mockFinder, limit) + rateLimitedFinder := NewRateLimitedFinder(mockFinder, uint(limit)) assert.Equal(t, limit, rateLimitedFinder.Limit()) for i := 0; i < totalCalls; i++ { go f(rateLimitedFinder) @@ -57,7 +57,7 @@ func TestRateLimitedFinder(t *testing.T) { requestsExceedingLimit := totalCalls - limit for i := 0; i < requestsExceedingLimit; i++ { err := <-errorChan - assert.Equal(t, ErrLimitExceeded, err) + assert.Equal(t, ErrRateLimitExceeded, err) } wg.Add(-requestsExceedingLimit)