backend: replace ratelimiter with x/time/rate. #3136
Conversation
| @@ -50,17 +52,22 @@ const ERC20GasErr = "insufficient funds for gas * price + value" | |||
| type EtherScan struct { | |||
| url string | |||
| httpClient *http.Client | |||
| limiter rate.Limiter | |||
There was a problem hiding this comment.
usually *rate.Limiter so you don't need to dereference *rate.NewLimiter(...) below, or is there a reason for making this a value instead of a pointer?
backend/rates/history.go
Outdated
| @@ -278,38 +278,36 @@ func (updater *RateUpdater) fetchGeckoMarketRange(ctx context.Context, coin, fia | |||
| } | |||
|
|
|||
| // Make the call, abiding the upstream rate limits. | |||
| msg := fmt.Sprintf("fetch coingecko coin=%s fiat=%s start=%s", coin, fiat, timeRange.start) | |||
| // msg := fmt.Sprintf("fetch coingecko coin=%s fiat=%s start=%s", coin, fiat, timeRange.start) | |||
There was a problem hiding this comment.
Hey yeah, the issue is that this was passed to a fn call that doesn't exist anymore, I think for logging purposes and this is a bit harder now to achieve. I was planning to find a way around this and only then open for review :)
There was a problem hiding this comment.
I don't think there is a way here to have the log message show up periodically as we did before. Wait() is a blocking call and we have no way of loggin periodically as we did before.
bznein
force-pushed
the
RateLimiting
branch
3 times, most recently
from
976c710 to
ca3e504
Compare
There was a problem hiding this comment.
@benma this is overall ready for review.
the changes are mostly on backend/rates and backend/coins/eth/etherscan
I was able to test them in rates by adding a log line whenever we make the API call and decrease massively the number of calls allowed per second.
However, as discussed offline, I currently have no way to test them on etherscan, so I won't be anyway merging this until I can.
| // Etherscan rate limits to one request per 0.2 seconds. | ||
| var CallInterval = 260 * time.Millisecond | ||
| var CallsPerSec = 3.8 |
There was a problem hiding this comment.
~= 1/0.26 (previous duration between requests)
| } | ||
| } | ||
|
|
||
| func (etherScan *EtherScan) call(params url.Values, result interface{}) error { | ||
| if err := etherScan.limiter.Wait(context.TODO()); err != nil { |
There was a problem hiding this comment.
Wait needs a context that we do not have here. I'm open to suggestions. FWIW we didn't have it before either
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| @@ -323,7 +330,7 @@ func (etherScan *EtherScan) Transactions( | |||
| result := struct { | |||
| Result []*Transaction | |||
| }{} | |||
| if err := etherScan.call(params, &result); err != nil { | |||
| if err := etherScan.call(context.TODO(), params, &result); err != nil { | |||
There was a problem hiding this comment.
the linter was forcing me to pass the context down the stack (which I think makes absolutely sense!) but there are two places (this function and another one) in which we don't have it, so I'm open to suggestions here
Removes the ratelimiter package and replaces its usages with x/time/rate.
|
Closing in favour of #3155 |
Removes the ratelimiter package and replaces its usages with x/time/rate.