PTP-535 Allow Provider in Constructor #47
Conversation
efecarranza
added
help wanted
efecarranza
added
Reviewers Needed
and removed
help wanted
src/lib/Auth/Auth.js
Outdated
| @@ -18,29 +19,31 @@ export default class Auth { | |||
|
|
|||
| #provider; | |||
|
|
|||
| #signer; | |||
There was a problem hiding this comment.
this.#signer is not used in the file
src/lib/Auth/Auth.js
Outdated
| // eslint-disable-next-line new-cap | ||
| this.#provider = Provider.getProvider(this.#rpcUrl); | ||
|
|
||
| this.setProvider(privateKey, provider); |
There was a problem hiding this comment.
function signature should be using this.#privateKey instead of passing args. E.g,
this.setProvider(provider)
src/lib/Auth/Auth.js
Outdated
| throw new Error( | ||
| '[Auth.setInjectedProvider] You need to pass an injected provider to this function!', | ||
| ); | ||
| setProvider(privateKey, provider) { |
There was a problem hiding this comment.
function signature should be using this.#privateKey instead of passing args. E.g,
setProvider(provider) {
src/lib/Auth/Auth.js
Outdated
| '[Auth.setInjectedProvider] You need to pass an injected provider to this function!', | ||
| ); | ||
| setProvider(privateKey, provider) { | ||
| if (privateKey) { |
src/lib/Auth/Auth.js
Outdated
| ); | ||
| setProvider(privateKey, provider) { | ||
| if (privateKey) { | ||
| // eslint-disable-next-line new-cap |
| @@ -15,7 +15,7 @@ export default class Provider { | |||
| } | |||
|
|
|||
| static getInjectedProvider(injectedProvider) { | |||
There was a problem hiding this comment.
Not sure about this method anymore: do we really want the SDK to expose the "provider" class so that the user can use "provider.getInjectedProvider()" ? Shouldn't we have a "Auth.getProvider()" method that acts as the wrapper? or maybe directly passing the "window.ethereum" as a parameter in the Auth object, like:
const accWithProvider = new Auth({
projectId: process.env.INFURA_PROJECT_ID,
secretId: process.env.INFURA_PROJECT_SECRET,
rpcUrl: process.env.EVM_RPC_URL,
chainId: 5,
provider: window.ethereum,
});| it('should throw when passing both privateKey and provider', () => { | ||
| expect( | ||
| () => | ||
| // eslint-disable-next-line implicit-arrow-linebreak |
There was a problem hiding this comment.
eslint is not going through test file. You can remove this line.
PS: if eslint bothers you in test file, add it as an exception in .eslintignore
| it('should throw when passing invalid provider', () => { | ||
| expect( | ||
| () => | ||
| // eslint-disable-next-line implicit-arrow-linebreak |
test/auth.test.js
Outdated
| projectId: process.env.INFURA_PROJECT_ID, | ||
| secretId: process.env.INFURA_PROJECT_SECRET, | ||
| rpcUrl: process.env.EVM_RPC_URL, | ||
| chainId: 5, | ||
| provider: provider, |
There was a problem hiding this comment.
no need for "provider: provider". Just "provider" is enough.
usage.js
Outdated
| ///////// Alternative Auth Instantiation with MetaMask ///////// | ||
| // When using SDK in a browser | ||
| // | ||
| // const provider = Provider.getInjectedProvider(window.ethereum); |
There was a problem hiding this comment.
check my comment above about the "usage" from the end-user perspective. I'm not sure we should allow SDK users to use "provider" (and, if it's the case, we need to export it in the index.js)
Best dev experience for me would be to either pass a private key OR pass the window.ethereum object to the Auth constructor.
WDYT @danielpavelicconsensys @salimtb @VGau @adriendelaroche ?
|
Kudos, SonarCloud Quality Gate passed! |
Changelog
Allow to pass provider in constructor.