Expose auth providers from taco
#534
Conversation
✅ Deploy Preview for taco-nft-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for taco-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
4 times, most recently
from
934d7e6
to
cba9fbd
Compare
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
from
cba9fbd
to
51e5402
Compare
piotr-roslaniec
force-pushed
the
epic-auth
branch
from
4eea8e7
to
5a4d072
Compare
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
3 times, most recently
from
9ac0d00
to
582d140
Compare
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
from
582d140
to
9ce1a99
Compare
piotr-roslaniec
requested review from
KPrasch,
cygnusv,
theref,
derekpierre and
manumonti
| const maybeOrigin = window?.location?.origin; | ||
| return { | ||
| domain: maybeOrigin.split('//')[1].split('.')[0], | ||
| uri: maybeOrigin, | ||
| }; |
There was a problem hiding this comment.
Judging by this code here - https://1.x.wagmi.sh/examples/sign-in-with-ethereum#step-3-sign--verify-message, assuming window.location is available, can we just use the host property for the domain value instead of the parsing?
Also, out of curiosity is document an option in addition to window? Noticed this here - https://github.com/spruceid/siwe-notepad/blob/main/src/providers.ts#L132. Not sure what environment document is available ... 🤔
There was a problem hiding this comment.
Yes, document and window are globally available objects provided by the browser environment.
I updated this snippet.
packages/taco/src/taco.ts
Outdated
| provider: ethers.providers.Provider, | ||
| domain: Domain, | ||
| messageKit: ThresholdMessageKit, | ||
| authProviders?: AuthProviders, |
There was a problem hiding this comment.
Interested to see what this parameter looks like when EIP712 eventually gets removed.
There was a problem hiding this comment.
If we don't foresee new providers near-term, it could be just authProvider: EIP4361Provider
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
4 times, most recently
from
05a2248
to
4687bc6
Compare
piotr-roslaniec
force-pushed
the
expose-auth-providers
branch
from
4687bc6
to
f452c6a
Compare
piotr-roslaniec
added
the
blocked
| @@ -46,7 +64,7 @@ export class EIP4361AuthProvider { | |||
|
|
|||
| private async createSIWEAuthMessage(): Promise<AuthSignature> { | |||
| const address = await this.signer.getAddress(); | |||
| const { domain, uri } = this.getParametersOrDefault(); | |||
| const { domain, uri } = this.providerParams; | |||
| const version = '1'; | |||
| const nonce = generateNonce(); | |||
| const chainId = (await this.provider.getNetwork()).chainId; | |||
There was a problem hiding this comment.
Can we do signer.getChainId( ) here? Then we can get rid of provider from the constructor.
There was a problem hiding this comment.
Not every signer has a connected provider. In ethers, it's possible to create a signer using a raw private key without ever connecting to an RPC.
I think we may want to explore requiring that every signer has a provider already connected and to do that in the whole codebase. But I think this is for another PR since we would be changing quite a lot of APIs.
Type of PR:
Required reviews:
What this does:
decryptmethod intaconow takesauthProvider: EIP4361instead ofsigner: ethers.SignerEIP4361Providerfromtacorather thantaco-authto avoid fragmenting TACo API across multiple packages. To be reassessed when/if we publishtaco-authEIP4361ProviderIssues fixed/closed:
@nucypher/taco#533Why it's needed:
Notes for reviewers: