-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gateio: update rate limit definitions #1733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the limits!
) | ||
|
||
// GetRateLimit returns the rate limiter for the exchange | ||
func GetRateLimit() request.RateLimitDefinitions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello friend, would you consider this?
Sharing requesters and thus ratelimits isn't a super complex proposition, so someone using multiple exchange structs are better served running their own thing at the moment because this only affects GateIO
We're approaching the same problem in a different manner though. I would really like to see something across exchanges done.
editeroo: If you don't want to dilute this PR, do you think this is worthwhile to make a separate PR for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was considering that but wanted to go more in depth in another PR for a request.RegisterRateLimit(exchangeStructPointer any, map[request.EndpointLimit]*request.RateLimiter) func
and maybe a protected type just for exchanges. That way any future exchange package implementations don't reintroduce the whoopsie again.
- I can add your changes to this PR for now or
- You can open up a PR against this and I can merge or
- You can open a PR against master once this is merged cause I am focused on other things at the moment.
let me know how you want to proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its something to roll out separately in a different PR. Like I'm okay without it, as per my above second snippet. Can be revisited in future and per my first snippet, isn't crazy complex to implement
exchanges/gateio/ratelimiter.go
Outdated
walletSavedAddressesEPL: standardRateLimit(), | ||
walletTradingFeeEPL: standardRateLimit(), | ||
walletTotalBalanceEPL: personalAccountRateLimit(), | ||
walletWithdrawEPL: request.NewRateLimitWithWeight(time.Second*3, 1, 1), // 1r/3s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it a rate limit like the others. I initially thought this rate limit wasn't defined but was instead buried. This might just be on me as a reviewer and I will fold like paper to any argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only single one in docs for 1 request every 3 seconds only on withdrawal from wallet. 🤷
… item to nil for systems that do not require rate limiting; add glorious nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, rate limit EPLs match expected endpoints; nice work on this as that is always tedious
PR Description
func GetRateLimit() request.RateLimitDefinitions
as this allocates a new rate limiting system for the package when SetDefaults is called. If using as a library service and multiple instances &gateio.Gateio{} are allocated and setup (for different key usage etc or other segregation management) then there can be an issue with getting 429'd.Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
x
in[]
as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist