Authentication providers #531
Conversation
…rt generic auth-provider
piotr-roslaniec
force-pushed
the
auth-provider
branch
from
edee0d5
to
41cebb2
Compare
piotr-roslaniec
force-pushed
the
auth-provider
branch
from
794119b
to
5eec120
Compare
piotr-roslaniec
force-pushed
the
auth-provider
branch
from
da5c631
to
56e9907
Compare
piotr-roslaniec
requested review from
KPrasch,
cygnusv,
theref,
derekpierre and
manumonti
| private readonly storage: LocalStorage; | ||
|
|
||
| constructor( | ||
| // TODO: We only need the provider to fetch the chainId, consider removing it | ||
| private readonly provider: ethers.providers.Provider, |
There was a problem hiding this comment.
Seems like the only reason for provider is to get the chain id, but isn't this available via the signer - https://docs.ethers.org/v5/api/signer/#Signer-getChainId;
There was a problem hiding this comment.
Yes, that's correct. Signer is only to sign things, provider is to maintain communication with an external RPC / RPC provider
| @@ -153,6 +154,8 @@ export const decrypt = async ( | |||
| messageKit.acp.publicKey, | |||
| ); | |||
| const ritual = await DkgClient.getActiveRitual(provider, domain, ritualId); | |||
| // TODO: Temporary helper method to keep the external taco.ts decrypt function simple | |||
There was a problem hiding this comment.
What's the long-term plan/vision for the TODO?
There was a problem hiding this comment.
To expose authentication providers in the user-facing TACo API once we are ready to release. This is going to be a breaking change and it makes sense to bundle it with other auth-related changes, and maybe context parameters refactor (more user-facing API).
More of a general question and something I may have missed - how is the above done by apps? |
| @@ -53,6 +54,7 @@ export class EIP4361SignatureProvider { | |||
| return { signature, address, scheme, typedData: message }; | |||
| } | |||
|
|
|||
| // TODO: Create a facility to set these parameters or expose them to the user | |||
There was a problem hiding this comment.
If we haven't already, we should track this in an issue somewhere. L70 is particularly concerning - it's unclear how generic it is, as opposed to specific to the taco-composedb demo.
There was a problem hiding this comment.
I will do that on the back of exposing authentication providers from taco.
Created: #533
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: