-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: added option to load multiple api keys selected at random order #88
Conversation
Have to add API keys as repo secret in a variable called |
Maybe we can work on the randomness better with seed or something else |
Keeping the |
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.
Looks pretty good! I added some small feedback if you want try and address.
Do you have any screenshots or demos showing it working?
To change the API key after connection, you'd have to reconnect |
Do you think this requires any more corrections? |
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 is a nice feature and we appreciate it.
I think it needs a tad more work however!
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.
LGTM
(1 typo)
Let me know if it's ok or not. |
@antazoey to pass the tests you have to set multiple API keys like this |
Can you make tests add fake key for testing this feature? I don't want to have to create multiple keys to run the tests. |
@Aviksaikat We should be able to test the key selection logic alone without making requests to infura |
The provider fixture is trying to make the request |
are you able to refactor? |
Got rid of the last test |
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.
Looks good!, just delete the commented out part
Also please address the |
Done. But to pass the existing first 2 tests the variables need to be set |
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.
tests pass! but flake8 is still failing.
Also i am bit unsure of the reconnect
method for a few reasons
- it does not reconnect
- it might not be needed - can jus use disconnect and have that clear the keys
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.
lgtm
What I did
fixes: #87
How I did it
Example
How to verify it
Checklist