multi: add Coindesk backends for fiat prices #116
Conversation
ellemouton
force-pushed
the
coindeskBackends
branch
3 times, most recently
from
560959b
to
ec0338b
Compare
fiat/coindesk_api.go
Outdated
| // nolint: prealloc | ||
| var usdRecords []*USDPrice |
fiat/prices.go
Outdated
| GetPrices(ctx context.Context, startTime, endTime time.Time) ([]*USDPrice, error) | ||
| } | ||
|
|
||
| type PriceBackend string |
There was a problem hiding this comment.
nit: godocs for this enum and the coincap/coindesk values below.
I also have a slight preference for uint8 for categories like this, I think it's a bit more standard, but that's a loose opinion loosely held. If you just do ints, you can add a String method which returns the names as you have them here.
fiat/coindesk_api.go
Outdated
|
|
||
| // GetPrices retrieves price information from coindesks's api for the given | ||
| // time range. | ||
| func (c *coinDeskAPI) GetPrices(ctx context.Context, start, |
There was a problem hiding this comment.
Time range validation and sort by timestamp is going to be common to all fiat backends, so could we cut the duplication out?
Thinking something like this:
type FiatSource struct{
impl fiatBackend
}
func (f *FiatSource) GetPrices(){
utils.ValidateTimeRange
records, err := f.impl.rawPriceData()
sort(records)
}
Where fiatBackend would be coincap/coindesk implementation that just fetches data. You'd need to rename the coincap GetPrices and move validation out, but then we can just return a FiatSource and we don't even need to export the interface.
fiat/prices.go
Outdated
| @@ -31,7 +31,7 @@ type PriceAPIBackend interface { | |||
| type PriceBackend string | |||
|
|
|||
| const ( | |||
| UnknownPriceBackend PriceBackend = "" | |||
| UnknownPriceBackend PriceBackend = "" | |||
There was a problem hiding this comment.
nit: format in previous commit?
ellemouton
force-pushed
the
coindeskBackends
branch
from
ec0338b
to
e74f11c
Compare
|
Updated 👍 |
ellemouton
force-pushed
the
coindeskBackends
branch
from
e74f11c
to
2477dc1
Compare
fiat/prices.go
Outdated
| var priceBackendNames = map[PriceBackend]string{ | ||
| UnknownPriceBackend: "", | ||
| CoinCapPriceBackend: "coincap", | ||
| CoinDeskPriceBackend: "coindesk", | ||
| } | ||
|
|
||
| func (p PriceBackend) ToString() string { | ||
| return priceBackendNames[p] | ||
| } | ||
|
|
There was a problem hiding this comment.
hmm so #117 adds another one of these maps. Could get messy if more things are added. How about something like this rather:
type backendsConfig struct {
name string
proxySupport bool
}
var priceBackendsConfig = map[PriceBackend]backendsConfig {
UnknownPriceBackend: {},
CoinCapPriceBackend: {
name: "coincap",
proxySupport: false,
},
CoinDeskPriceBackend: {
name: "coindesk",
proxySupport: true,
},
}
2477dc1
to
ca81510
Compare
There was a problem hiding this comment.
Great commit structure! Makes such a difference in reveiw :)
This is shaping up really well, just some nits, and the imports GH action is failing (experimenting with a check to group imports nicely, there may be some bugs in it so lmk if it seems unreasonable).
fiat/prices.go
Outdated
| ) | ||
|
|
||
| var priceBackendNames = map[PriceBackend]string{ | ||
| UnknownPriceBackend: "", |
There was a problem hiding this comment.
Probably better to make this "Unknown", otherwise once day we'll forget that it's empty and know why we're logging some random empty string.
fiat/prices.go
Outdated
| CoinCapPriceBackend: "coincap", | ||
| } | ||
|
|
||
| func (p PriceBackend) ToString() string { |
There was a problem hiding this comment.
String so that it satisfies Stringer interface?
frdrpc/rpcserver.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| prices, err := fiat.GetPrices(ctx, timestamps, *granularity) | ||
| prices, err := fiat.GetPrices(ctx, timestamps, fiatBackend, *granularity) |
There was a problem hiding this comment.
These look a bit long, are they wrapped at 80?
ellemouton
force-pushed
the
coindeskBackends
branch
3 times, most recently
from
6d2d620
to
18a4d01
Compare
|
Thanks for the review @carlaKC 😊 updated 👍 |
| ) | ||
|
|
||
| var priceBackendNames = map[PriceBackend]string{ |
There was a problem hiding this comment.
ok nice, following the logic for a map here given the context of the follow up PR 👌
This commit adds a fiatBackend interface that added fiat backend needs to implement.
This commit adds a coindesk implementation of the fiatBackend interface.
This commit adds a fiat_backends option to the RPC relevant RPC endpoints. Namely, the NodeAudit and ExchangeRate endpoint.
ellemouton
force-pushed
the
coindeskBackends
branch
from
18a4d01
to
38a0790
Compare
This PR addresses #37 by adding CoinDesk as a possible backend to use for price information. Users can use the
fiat_backendflag in theauditandfiatRPCs to chose a backend. The default is still CoinCap.For example:
(This PR kinda clashes with #115 but CoinDesk also has an endpoint for different currency rates so I can update this PR to include that if #115 ends up being accepted 👍 )