-
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: add standardised common Provider #184
Conversation
src/common/utils.ts
Outdated
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." |
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.
"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" |
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.
ok
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.
missed this
expect(provider?.connection?.url).toMatch(/(infura)/i); | ||
}); | ||
|
||
it("should still generate a provider even if only one option (url) is provided", () => { |
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.
surprised this works, what does ethers.js do when you provide a url like that?
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.
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"; |
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.
Who owns those 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.
erm me, but i will revert. it to use ethers.js default 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.
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. |
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.
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
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.
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. |
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.
PROVIDER_ENDPOINT_URL instead
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.
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". |
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.
PROVIDER_NETWORK
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.
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". |
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.
PROVIDER_ENDPOINT_TYPE
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.
ok
src/common/utils.ts
Outdated
|
||
case "alchemy": | ||
apiKey = options?.apiKey || process.env.ALCHEMY_API_KEY || ""; | ||
!apiKey && |
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.
do the apikey check earlier instead of repeating this statement 3 times?
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.
ok
src/common/utils.ts
Outdated
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." |
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.
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, { |
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.
store and restore the values just incase any other test is doing anything with these variables
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.
ok will do
@@ -131,7 +150,7 @@ describe("verify v3(integration)", () => { | |||
] | |||
`); | |||
expect(valid).toBe(true); | |||
}); | |||
}, 10000); |
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.
are these tests slower now after the PR?
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, some test will timeout so need to extend the timeout for the tests.
🎉 This PR is included in version 7.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In this PR: