diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 1b766fe80..824af3f11 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -35,6 +35,14 @@ const ( // maxFeeUpdateTimeout represents the maximum interval in which a // WebAPIEstimator will request fresh fees from its API. maxFeeUpdateTimeout = 20 * time.Minute + + // WebAPIConnectionTimeout specifies the timeout value for connecting + // to the api source. + WebAPIConnectionTimeout = 5 * time.Second + + // WebAPIResponseTimeout specifies the timeout value for receiving a + // fee response from the api source. + WebAPIResponseTimeout = 10 * time.Second ) var ( @@ -493,15 +501,9 @@ var _ Estimator = (*BitcoindEstimator)(nil) // implementation of this interface in order to allow the WebAPIEstimator to // be fully generic in its logic. type WebAPIFeeSource interface { - // GenQueryURL generates the full query URL. The value returned by this - // method should be able to be used directly as a path for an HTTP GET - // request. - GenQueryURL() string - - // ParseResponse attempts to parse the body of the response generated - // by the above query URL. Typically this will be JSON, but the - // specifics are left to the WebAPIFeeSource implementation. - ParseResponse(r io.Reader) (map[uint32]uint32, error) + // GetFeeMap will query the web API, parse the response and return a + // map of confirmation targets to sat/kw fees. + GetFeeMap() (map[uint32]uint32, error) } // SparseConfFeeSource is an implementation of the WebAPIFeeSource that utilizes @@ -513,21 +515,12 @@ type SparseConfFeeSource struct { URL string } -// GenQueryURL generates the full query URL. The value returned by this -// method should be able to be used directly as a path for an HTTP GET -// request. -// -// NOTE: Part of the WebAPIFeeSource interface. -func (s SparseConfFeeSource) GenQueryURL() string { - return s.URL -} - -// ParseResponse attempts to parse the body of the response generated by the +// parseResponse attempts to parse the body of the response generated by the // above query URL. Typically this will be JSON, but the specifics are left to // the WebAPIFeeSource implementation. -// -// NOTE: Part of the WebAPIFeeSource interface. -func (s SparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, error) { +func (s SparseConfFeeSource) parseResponse(r io.Reader) ( + map[uint32]uint32, error) { + type jsonResp struct { FeeByBlockTarget map[uint32]uint32 `json:"fee_by_block_target"` } @@ -543,6 +536,47 @@ func (s SparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, erro return resp.FeeByBlockTarget, nil } +// GetFeeMap will query the web API, parse the response and return a map of +// confirmation targets to sat/kw fees. +func (s SparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) { + // Rather than use the default http.Client, we'll make a custom one + // which will allow us to control how long we'll wait to read the + // response from the service. This way, if the service is down or + // overloaded, we can exit early and use our default fee. + netTransport := &http.Transport{ + Dial: (&net.Dialer{ + Timeout: WebAPIConnectionTimeout, + }).Dial, + TLSHandshakeTimeout: WebAPIConnectionTimeout, + } + netClient := &http.Client{ + Timeout: WebAPIResponseTimeout, + Transport: netTransport, + } + + // With the client created, we'll query the API source to fetch the URL + // that we should use to query for the fee estimation. + targetURL := s.URL + resp, err := netClient.Get(targetURL) + if err != nil { + log.Errorf("unable to query web api for fee response: %v", + err) + return nil, err + } + defer resp.Body.Close() + + // Once we've obtained the response, we'll instruct the WebAPIFeeSource + // to parse out the body to obtain our final result. + feesByBlockTarget, err := s.parseResponse(resp.Body) + if err != nil { + log.Errorf("unable to parse fee api response: %v", err) + + return nil, err + } + + return feesByBlockTarget, nil +} + // A compile-time assertion to ensure that SparseConfFeeSource implements the // WebAPIFeeSource interface. var _ WebAPIFeeSource = (*SparseConfFeeSource)(nil) @@ -762,38 +796,11 @@ func (w *WebAPIEstimator) getCachedFee(numBlocks uint32) (uint32, error) { // updateFeeEstimates re-queries the API for fresh fees and caches them. func (w *WebAPIEstimator) updateFeeEstimates() { - // Rather than use the default http.Client, we'll make a custom one - // which will allow us to control how long we'll wait to read the - // response from the service. This way, if the service is down or - // overloaded, we can exit early and use our default fee. - netTransport := &http.Transport{ - Dial: (&net.Dialer{ - Timeout: 5 * time.Second, - }).Dial, - TLSHandshakeTimeout: 5 * time.Second, - } - netClient := &http.Client{ - Timeout: time.Second * 10, - Transport: netTransport, - } - - // With the client created, we'll query the API source to fetch the URL - // that we should use to query for the fee estimation. - targetURL := w.apiSource.GenQueryURL() - resp, err := netClient.Get(targetURL) - if err != nil { - log.Errorf("unable to query web api for fee response: %v", - err) - return - } - defer resp.Body.Close() - // Once we've obtained the response, we'll instruct the WebAPIFeeSource // to parse out the body to obtain our final result. - feesByBlockTarget, err := w.apiSource.ParseResponse(resp.Body) + feesByBlockTarget, err := w.apiSource.GetFeeMap() if err != nil { - log.Errorf("unable to query web api for fee response: %v", - err) + log.Errorf("unable to get fee response: %v", err) return } diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index 8227b5f2b..fc16c9b12 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -3,27 +3,12 @@ package chainfee import ( "bytes" "encoding/json" - "io" - "reflect" "testing" "github.com/btcsuite/btcd/btcutil" "github.com/stretchr/testify/require" ) -type mockSparseConfFeeSource struct { - url string - fees map[uint32]uint32 -} - -func (e mockSparseConfFeeSource) GenQueryURL() string { - return e.url -} - -func (e mockSparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, error) { - return e.fees, nil -} - // TestFeeRateTypes checks that converting fee rates between the // different types that represent fee rates and calculating fees // work as expected. @@ -113,10 +98,6 @@ func TestSparseConfFeeSource(t *testing.T) { // Test that GenQueryURL returns the URL as is. url := "test" feeSource := SparseConfFeeSource{URL: url} - queryURL := feeSource.GenQueryURL() - if queryURL != url { - t.Fatalf("expected query URL of %v, got %v", url, queryURL) - } // Test parsing a properly formatted JSON API response. // First, create the response as a bytes.Reader. @@ -125,17 +106,17 @@ func TestSparseConfFeeSource(t *testing.T) { 2: 42, 3: 54321, } - testJSON := map[string]map[uint32]uint32{"fee_by_block_target": testFees} + testJSON := map[string]map[uint32]uint32{ + "fee_by_block_target": testFees, + } jsonResp, err := json.Marshal(testJSON) require.NoError(t, err, "unable to marshal JSON API response") reader := bytes.NewReader(jsonResp) // Finally, ensure the expected map is returned without error. - fees, err := feeSource.ParseResponse(reader) + fees, err := feeSource.parseResponse(reader) require.NoError(t, err, "unable to parse API response") - if !reflect.DeepEqual(fees, testFees) { - t.Fatalf("expected %v, got %v", testFees, fees) - } + require.Equal(t, testFees, fees, "unexpected fee map returned") // Test parsing an improperly formatted JSON API response. badFees := map[string]uint32{"hi": 12345, "hello": 42, "satoshi": 54321} @@ -145,10 +126,8 @@ func TestSparseConfFeeSource(t *testing.T) { reader = bytes.NewReader(jsonResp) // Finally, ensure the improperly formatted fees error. - _, err = feeSource.ParseResponse(reader) - if err == nil { - t.Fatalf("expected ParseResponse to fail") - } + _, err = feeSource.parseResponse(reader) + require.Error(t, err, "expected error when parsing bad JSON") } // TestWebAPIFeeEstimator checks that the WebAPIFeeEstimator returns fee rates @@ -216,10 +195,9 @@ func TestWebAPIFeeEstimator(t *testing.T) { maxTarget: minFeeRate, } - feeSource := mockSparseConfFeeSource{ - url: "https://www.github.com", - fees: feeRateResp, - } + // Create a mock fee source and mock its returned map. + feeSource := &mockFeeSource{} + feeSource.On("GetFeeMap").Return(feeRateResp, nil) estimator := NewWebAPIEstimator(feeSource, false) @@ -250,12 +228,15 @@ func TestWebAPIFeeEstimator(t *testing.T) { exp := SatPerKVByte(tc.expectedFeeRate).FeePerKWeight() require.Equalf(t, exp, est, "target %v failed, fee "+ - "map is %v", tc.target, feeSource.fees) + "map is %v", tc.target, feeRateResp) }) } // Stop the estimator when test ends. require.NoError(t, estimator.Stop(), "unable to stop fee estimator") + + // Assert the mocked fee source is called as expected. + feeSource.AssertExpectations(t) } // TestGetCachedFee checks that the fee caching logic works as expected. diff --git a/lnwallet/chainfee/mocks.go b/lnwallet/chainfee/mocks.go new file mode 100644 index 000000000..03d40e11e --- /dev/null +++ b/lnwallet/chainfee/mocks.go @@ -0,0 +1,17 @@ +package chainfee + +import "github.com/stretchr/testify/mock" + +type mockFeeSource struct { + mock.Mock +} + +// A compile-time assertion to ensure that mockFeeSource implements the +// WebAPIFeeSource interface. +var _ WebAPIFeeSource = (*mockFeeSource)(nil) + +func (m *mockFeeSource) GetFeeMap() (map[uint32]uint32, error) { + args := m.Called() + + return args.Get(0).(map[uint32]uint32), args.Error(1) +}