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: add standardised common Provider #184

Merged
merged 10 commits into from
Aug 17, 2021
Merged

Conversation

isaackps
Copy link
Contributor

@isaackps isaackps commented Jun 18, 2021

In this PR:

  • Added a common way to use provider

export const generateProvider = (options?: ProviderDetails): providers.Provider | Signer => {
if (!!options && Object.keys(options).length === 1 && options.apiKey) {
throw new Error(
"We could link the apiKey provided to a provider, please state the provider to use in the parameter."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"We could link the apiKey provided to a provider, please state the provider to use in the parameter."
"Provider needs to be specified when providing an API key to use"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

missed this

expect(provider?.connection?.url).toMatch(/(infura)/i);
});

it("should still generate a provider even if only one option (url) is provided", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

surprised this works, what does ethers.js do when you provide a url like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it will just use this url, there wont be any error msg when generating the provider

src/config.ts Outdated
@@ -1 +1,3 @@
export const INFURA_API_KEY = process.env.INFURA_API_KEY || "bb46da3f80e040e8ab73c0a9ff365d18";
export const ALCHEMY_API_KEY = process.env.ALCHEMY_API_KEY || "OlOgD-8qs5l3pQm-B_fcrMAmHTmAwkGj";
Copy link
Contributor

Choose a reason for hiding this comment

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

Who owns those keys ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

erm me, but i will revert. it to use ethers.js default keys

src/common/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rjchow rjchow left a comment

Choose a reason for hiding this comment

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

ok after resolving the mentioned issues, update oa-verify to use this function and we're good to go

README.md Outdated
```

## Advanced usage

### Environment Variables

- `INFURA_API_KEY`: let you provide your own `INFURA` API key.
- `ALCHEMY_API_KEY`: let you provide your own `ALCHEMY` API key.
Copy link
Contributor

Choose a reason for hiding this comment

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

just use PROVIDER_API_KEY for all and figure it out internally, but also continue supporting INFURA_API_KEY for backward compatibility - add a warning when people use that, and ask them to use PROVIDER_API_KEY instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

README.md Outdated
```

## Advanced usage

### Environment Variables

- `INFURA_API_KEY`: let you provide your own `INFURA` API key.
- `ALCHEMY_API_KEY`: let you provide your own `ALCHEMY` API key.
- `ETHERSCAN_API_KEY`: let you provide your own `ETHERSCAN` API key.
- `JSONRPC_PROVIDER_URL`: let you provide your preferred JSON-RPC HTTP API URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

PROVIDER_ENDPOINT_URL instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

README.md Outdated
- `ALCHEMY_API_KEY`: let you provide your own `ALCHEMY` API key.
- `ETHERSCAN_API_KEY`: let you provide your own `ETHERSCAN` API key.
- `JSONRPC_PROVIDER_URL`: let you provide your preferred JSON-RPC HTTP API URL.
- `NETWORK`: let you specify the network to use, i.e. "homestead", "mainnet", "ropsten", "rinkeby".
Copy link
Contributor

Choose a reason for hiding this comment

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

PROVIDER_NETWORK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

README.md Outdated
- `ETHERSCAN_API_KEY`: let you provide your own `ETHERSCAN` API key.
- `JSONRPC_PROVIDER_URL`: let you provide your preferred JSON-RPC HTTP API URL.
- `NETWORK`: let you specify the network to use, i.e. "homestead", "mainnet", "ropsten", "rinkeby".
- `PROVIDER`: let you specify the provider to use, i.e. "infura", "alchemy", "etherscan", "jsonrpc".
Copy link
Contributor

Choose a reason for hiding this comment

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

PROVIDER_ENDPOINT_TYPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


case "alchemy":
apiKey = options?.apiKey || process.env.ALCHEMY_API_KEY || "";
!apiKey &&
Copy link
Contributor

Choose a reason for hiding this comment

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

do the apikey check earlier instead of repeating this statement 3 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

export const generateProvider = (options?: ProviderDetails): providers.Provider | Signer => {
if (!!options && Object.keys(options).length === 1 && options.apiKey) {
throw new Error(
"We could link the apiKey provided to a provider, please state the provider to use in the parameter."
Copy link
Contributor

Choose a reason for hiding this comment

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

missed this

@@ -47,8 +48,25 @@ const verifyRopsten = verificationBuilder(openAttestationVerifiers, { network: "
const verifyRinkeby = verificationBuilder(openAttestationVerifiers, { network: "rinkeby" });

describe("verify(integration)", () => {
beforeEach(() => {
jest.resetModules();
process.env = Object.assign(process.env, {
Copy link
Contributor

Choose a reason for hiding this comment

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

store and restore the values just incase any other test is doing anything with these variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will do

@@ -131,7 +150,7 @@ describe("verify v3(integration)", () => {
]
`);
expect(valid).toBe(true);
});
}, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

are these tests slower now after the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, some test will timeout so need to extend the timeout for the tests.

@isaackps isaackps changed the title feat: add standardised fallBackProvider feat: add standardised common Provider Aug 6, 2021
@isaackps isaackps merged commit 59581d1 into master Aug 17, 2021
@isaackps isaackps deleted the feat/common-provider branch August 17, 2021 07:00
@john-dot-oa
Copy link
Contributor

🎉 This PR is included in version 7.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants