multi: add socks proxy flag #117
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellemouton
commented
May 6, 2021
| @@ -91,17 +93,56 @@ var priceBackendNames = map[PriceBackend]string{ | |||
| CoinDeskPriceBackend: "coindesk", | |||
| } | |||
|
|
|||
| var priceBackendProxySupport = map[PriceBackend]bool{ | |||
There was a problem hiding this comment.
thoughts on doing a config map instead of the 2 separate priceBackendNames and priceBackendProxySupport maps? So something like this:
type backendsConfig struct {
name string
proxySupport bool
}
var priceBackendsConfig = map[PriceBackend]backendsConfig {
UnknownPriceBackend: {
name: "unknown",
proxySupport: false,
},
CoinCapPriceBackend: {
name: "coincap",
proxySupport: false,
},
CoinDeskPriceBackend: {
name: "coindesk",
proxySupport: true,
},
}
ellemouton
force-pushed
the
proxyFlag
branch
3 times, most recently
from
f3e9a5f to
761b463
Compare
ellemouton
changed the title
This commit adds support for the coindesk backends to be initialized with a socks proxy if one is provided.
This commit adds a socks proxy option to the rpc config. This option is set using the SocksProxy flag in the main faraday config.
carlaKC
reviewed
Jun 14, 2021
fiat/prices.go
Outdated
| // Check that the proxy flag is not set for a price backend that does | ||
| // not support requests through a socks proxy. | ||
| if proxy != "" && !priceBackendProxySupport[backend] { | ||
| return nil, errors.New("can't use a socks proxy for the " + |
There was a problem hiding this comment.
nit: add the backend here for a friendlier user err
| // String returns the string representation of a price backend. | ||
| func (p PriceBackend) String() string { | ||
| return priceBackendNames[p] | ||
| } | ||
|
|
||
| // NewPriceSource returns a PriceSource which can be used to query price | ||
| // data. | ||
| func NewPriceSource(backend PriceBackend, granularity *Granularity) ( | ||
| *PriceSource, error) { | ||
| func NewPriceSource(backend PriceBackend, granularity *Granularity, |
There was a problem hiding this comment.
Can we add a unit test for this? Just to assert that we error out with the correct combinations etc.
| @@ -108,7 +108,7 @@ type CommonConfig struct { | |||
| func NewOnChainConfig(ctx context.Context, lnd lndclient.LndServices, startTime, | |||
| endTime time.Time, disableFiat bool, txLookup fees.GetDetailsFunc, | |||
| fiatBackend fiat.PriceBackend, granularity *fiat.Granularity, | |||
| categories []CustomCategory) *OnChainConfig { | |||
| categories []CustomCategory, socksProxy string) *OnChainConfig { | |||
There was a problem hiding this comment.
Getting to the point where it may be worth pulling all the fiat params out into a FiatConfig.
But I think we can kick that can down the road for now, if we hit > 3 params then it's time :')
|
@ellemouton, remember to re-request review from reviewers for your latest update |
|
!lightninglabs-deploy mute |
|
closing for now - anyone else is welcome to take this over |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses this comment suggestion to add a
--proxyoption to proxy http requests for price data over Tor.It seems to me from testing that I have done than CoinCap does not allow requests over tor, but CoinDesk does. So in this PR the default backend is CoinCap if no proxy is set and is CoinDesk if a proxy is set. If both proxy and backend are specified then a validation check is done to check if the specified backend supports proxy requests.