Skip to content
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

Merged
merged 14 commits into from
Aug 19, 2024

Conversation

Aviksaikat
Copy link
Contributor

@Aviksaikat Aviksaikat commented Aug 6, 2024

What I did

fixes: #87

How I did it

  • Added option to load API Keys in random order. Have to define API keys as an array of items.

Example

# bash/zsh/sh
export WEB3_INFURA_PROJECT_ID="cxxxxxxxxxxxxx, f4xxxxxxxxxxxxx, 6xxxxxxxxxxxxx, axxxxxxxxxxxxx"

# fish
set -gx WEB3_INFURA_PROJECT_IDS "cxxxxxxxxxxxxx, f4xxxxxxxxxxxxx, 6xxxxxxxxxxxxx, axxxxxxxxxxxxx"

How to verify it

  • Trust me bro ;)

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@Aviksaikat
Copy link
Contributor Author

Have to add API keys as repo secret in a variable called WEB3_INFURA_PROJECT_IDS to pass the CIs

@Aviksaikat
Copy link
Contributor Author

Maybe we can work on the randomness better with seed or something else

@Aviksaikat
Copy link
Contributor Author

Keeping the load_api_keys method public which will help refresh the API key.

antazoey
antazoey previously approved these changes Aug 6, 2024
Copy link
Member

@antazoey antazoey left a 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?

ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
tests/test_provider.py Outdated Show resolved Hide resolved
@Aviksaikat
Copy link
Contributor Author

image

@Aviksaikat
Copy link
Contributor Author

Aviksaikat commented Aug 7, 2024

There is an issue. It's only loading a random API key & not constructing the URI to make the API calls.
image

Line: provider.py#L60-61 may be the cause.

        if (ecosystem_name, network_name) in self.network_uris:
            return self.network_uris[(ecosystem_name, network_name)]

@Aviksaikat
Copy link
Contributor Author

Aviksaikat commented Aug 7, 2024

There is an issue. It's only loading a random API key & not constructing the URI to make the API calls. image

Line: provider.py#L60-61 may be the cause.

        if (ecosystem_name, network_name) in self.network_uris:
            return self.network_uris[(ecosystem_name, network_name)]

Added a method get_new_uri to generate a new URI while keeping it backward compatible.

image

@antazoey
Copy link
Member

antazoey commented Aug 7, 2024

To change the API key after connection, you'd have to reconnect

@Aviksaikat
Copy link
Contributor Author

Do you think this requires any more corrections?

Copy link
Member

@antazoey antazoey left a 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!

setup.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
antazoey
antazoey previously approved these changes Aug 8, 2024
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
(1 typo)

ape_infura/provider.py Outdated Show resolved Hide resolved
@Aviksaikat
Copy link
Contributor Author

Let me know if it's ok or not.

@antazoey
Copy link
Member

Running pytest leads to failing tests:

Screenshot 2024-08-13 at 08 03 53

@Aviksaikat
Copy link
Contributor Author

Running pytest leads to failing tests:

Screenshot 2024-08-13 at 08 03 53

Ahh maybe I missed the assertion. It must be > 0. As you need to have more than I key setup in order to be > 1

@Aviksaikat
Copy link
Contributor Author

@antazoey to pass the tests you have to set multiple API keys like this
image

@antazoey
Copy link
Member

@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
Copy link
Contributor Author

I think the project has already env variable setup. When we call reconnect it's trying to connect to the API endpoint. If you setup the env here it's all passing

image

With mock API keys

image

@antazoey
Copy link
Member

@Aviksaikat We should be able to test the key selection logic alone without making requests to infura

@Aviksaikat
Copy link
Contributor Author

The provider fixture is trying to make the request

@antazoey
Copy link
Member

The provider fixture is trying to make the request

are you able to refactor?

@Aviksaikat
Copy link
Contributor Author

The provider fixture is trying to make the request

are you able to refactor?

Got rid of the last test

antazoey
antazoey previously approved these changes Aug 14, 2024
Copy link
Member

@antazoey antazoey left a 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

ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Show resolved Hide resolved
@antazoey
Copy link
Member

Also please address the flake8 issues!

@Aviksaikat
Copy link
Contributor Author

Done. But to pass the existing first 2 tests the variables need to be set

Copy link
Member

@antazoey antazoey left a 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

  1. it does not reconnect
  2. it might not be needed - can jus use disconnect and have that clear the keys

ape_infura/provider.py Outdated Show resolved Hide resolved
ape_infura/provider.py Outdated Show resolved Hide resolved
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@antazoey antazoey merged commit 86e575f into ApeWorX:main Aug 19, 2024
5 of 13 checks passed
@antazoey antazoey mentioned this pull request Aug 19, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add option to pass multiple API keys
3 participants