From 8ca48311ee41535f9c02407c4fccf506d229fce2 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 19 Oct 2023 22:46:43 +0800 Subject: [PATCH 1/4] chainfee: add a new interface method `GetFeeMap` This commit adds a new method `GetFeeMap` to handle querying the web API and parse the response into a map that's used by the fee estimator. In doing so we can provide a deeper abstraction over the `WebAPIFeeSource` interface. --- lnwallet/chainfee/estimator.go | 76 ++++++++++++++++++----------- lnwallet/chainfee/estimator_test.go | 4 ++ 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 1b766fe80..8bf61e0ac 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -502,6 +502,10 @@ type WebAPIFeeSource interface { // 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 @@ -543,6 +547,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: 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 := s.GenQueryURL() + 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 +807,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..75bf950d8 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -24,6 +24,10 @@ func (e mockSparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, return e.fees, nil } +func (e mockSparseConfFeeSource) GetFeeMap() (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. From efcba4a8360465a57f3af0e4dcc98fa16dc20b48 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 19 Oct 2023 22:52:35 +0800 Subject: [PATCH 2/4] chainfee: simplify `WebAPIFeeSource` interface This commit removes the interface methods `GenQueryURL` and `ParseResponse` so the `WebAPIEstimator` can care less about the implementation details of this interface. --- lnwallet/chainfee/estimator.go | 31 ++++++----------------------- lnwallet/chainfee/estimator_test.go | 30 +++++++--------------------- 2 files changed, 13 insertions(+), 48 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 8bf61e0ac..dbeb87ee5 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -493,16 +493,6 @@ 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) @@ -517,21 +507,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"` } @@ -567,7 +548,7 @@ func (s SparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) { // 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.GenQueryURL() + targetURL := s.URL resp, err := netClient.Get(targetURL) if err != nil { log.Errorf("unable to query web api for fee response: %v", @@ -578,7 +559,7 @@ func (s SparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) { // 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) + feesByBlockTarget, err := s.parseResponse(resp.Body) if err != nil { log.Errorf("unable to parse fee api response: %v", err) diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index 75bf950d8..f962df5ec 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -3,8 +3,6 @@ package chainfee import ( "bytes" "encoding/json" - "io" - "reflect" "testing" "github.com/btcsuite/btcd/btcutil" @@ -16,14 +14,6 @@ type mockSparseConfFeeSource struct { 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 -} - func (e mockSparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) { return e.fees, nil } @@ -117,10 +107,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. @@ -129,17 +115,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} @@ -149,10 +135,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 From 090892c613378dabf9e0254f8f79a1d393923f43 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 19 Oct 2023 22:59:57 +0800 Subject: [PATCH 3/4] chainfee: use proper mock for `WebAPIFeeSource` in tests --- lnwallet/chainfee/estimator_test.go | 21 +++++++-------------- lnwallet/chainfee/mocks.go | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-) create mode 100644 lnwallet/chainfee/mocks.go diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index f962df5ec..fc16c9b12 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -9,15 +9,6 @@ import ( "github.com/stretchr/testify/require" ) -type mockSparseConfFeeSource struct { - url string - fees map[uint32]uint32 -} - -func (e mockSparseConfFeeSource) GetFeeMap() (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. @@ -204,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) @@ -238,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) +} From 3c62ba57d3d2f40b05a3b4de7575863e7e2886fa Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 19 Oct 2023 23:09:57 +0800 Subject: [PATCH 4/4] chainfee: make timeout values constant --- lnwallet/chainfee/estimator.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index dbeb87ee5..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 ( @@ -537,12 +545,12 @@ func (s SparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) { // overloaded, we can exit early and use our default fee. netTransport := &http.Transport{ Dial: (&net.Dialer{ - Timeout: 5 * time.Second, + Timeout: WebAPIConnectionTimeout, }).Dial, - TLSHandshakeTimeout: 5 * time.Second, + TLSHandshakeTimeout: WebAPIConnectionTimeout, } netClient := &http.Client{ - Timeout: time.Second * 10, + Timeout: WebAPIResponseTimeout, Transport: netTransport, }