Skip to content

Changed WithRateLimiter costFunc-arg to include the methods of all calls#122

Merged
lmittmann merged 4 commits intolmittmann:mainfrom
joel-u410:joel/include-cost-of-batch-request-itself
Mar 30, 2024
Merged

Changed WithRateLimiter costFunc-arg to include the methods of all calls#122
lmittmann merged 4 commits intolmittmann:mainfrom
joel-u410:joel/include-cost-of-batch-request-itself

Conversation

@joel-u410
Copy link
Copy Markdown
Contributor

This allows the cost func to provide a cost for the batch itself in batch requests. Some providers (Infura) include this as part of their rate limiting.

Ref: https://docs.infura.io/api/networks/ethereum/how-to/make-batch-requests

This allows the cost func to provide a cost for the batch itself in
batch requests. Some providers (Infura) include this as part of their
rate limiting.

Ref: https://docs.infura.io/api/networks/ethereum/how-to/make-batch-requests
@lmittmann
Copy link
Copy Markdown
Owner

I don't really like the convention of using the empty string method as signal for a batch request. The next special case will be a provider that charges CUs for 10+ calls in a batch requests. Maybe the more generic solution is

- costFunc func(method string) (cost int)
+ costFunc func(methods []string) (cost int)

What do you think?

@joel-u410
Copy link
Copy Markdown
Contributor Author

joel-u410 commented Mar 19, 2024

If you're ok with introducing a breaking API change, that solution makes sense to me! I didn't love using the empty string either but it seemed like the least-worst way to achieve it without breaking the WithRateLimiter interface.

The only thought I have is it leaks a little bit of the Call abstraction, because in the case where methods contains only one, it doesn't make a batch request (but this fact is really an internal implementation detail) -- it would be nice if costFunc didn't need to know about that. Maybe a 2nd parameter isBatch bool? But that seems like too much boilerplate for this one edge case. I guess maybe just clearly document that if len(methods) == 1 then it is not sent as a batch request.

@joel-u410
Copy link
Copy Markdown
Contributor Author

I've updated costFunc to receive methods and updated the comment to clarify the special case of len == 1. LMK what you think.

@joel-u410 joel-u410 force-pushed the joel/include-cost-of-batch-request-itself branch from af4f8b4 to f038ac1 Compare March 19, 2024 15:58
@lmittmann lmittmann added the enhancement New feature or request label Mar 24, 2024
@lmittmann
Copy link
Copy Markdown
Owner

Looks good 👍. Thank you for the PR. I will include this in v0.15.

@lmittmann lmittmann changed the title When rate limiting, include cost of batch request itself too. Changed WithRateLimiter costFunc-arg to include the methods of all calls Mar 30, 2024
@lmittmann lmittmann merged commit e60a0e8 into lmittmann:main Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants