CAIP-275: remove extra request and expand on chain property usage #292
Conversation
CAIPs/caip-275.md
Outdated
|
|
||
| #### Wallet connect to any wallet | ||
| However, this configuration is not recommended as having multiple wallets in the browser generates a race condition, and can result in false positives where there is a wallet extension, but it does not have the account linked to the domain. | ||
| We recommend using [EIP-6963][] by specifying a wallet rnds in the URI field if possible, to avoid this issue. |
There was a problem hiding this comment.
Maybe it would be smoother to write this section as a
SHOULD check rdns against 6963 signaling channel first to avoid false-positives or wallet impersonation via protocol pollution
followed by
MUST check window.ethereum if 6963 signal not detected.
This way your own SDK could do it right with 1193 as fallback and lazy implementations or implementations biased towards certain wallets/formfactors would have the option to skimp, but a stronger warning not to?
There was a problem hiding this comment.
I'm adding an EIP-6963 example before this one and those notes
The configurations for 6963 and injected are explicitly different. Using the injected wallet should not be the default case when trying to use 6963 without success as there is a clear intent to use a specific one
CAIPs/caip-275.md
Outdated
|
|
||
| ## Self hosting authentication flows | ||
| ## Self-hosting authentication flows | ||
|
|
||
| In case the user does not trust any authentication flow provider to store their info, they can easily host it themselves using a public serverless function and saving its URL in the ENS `authenticator` record. | ||
|
|
||
| ## Direct authentication flows resolution |
There was a problem hiding this comment.
I'm a little confused on the organization here-- maybe we could move usecases to one ##-level section and map each ###-level usecase inside of it explicitly to a ###-level flow specification inside of a ##-level Flows section? Not urgent but as this doc gets longer making it more readable/skimmable will help
There was a problem hiding this comment.
I'm not sure I get your exact point here
There was a problem hiding this comment.
5 weeks passed and now I don't get (i.e. remember) my point either
| - The user wants a truly decentralized solution and does not care about the gas cost of storing lots of data in blockchain; then they can write a stringified version of their authentication flows definition in the `authenticator` text record | ||
| - Application will resolve only its issued names, therefore the authentication flows or their location are likely already known. In such cases the application will likely use its own backend to resolve `authenticator` instead of ENS or another Crypto Domain system NFT system. Responding with the stringified version of the saves the user of the extra request as it does not provide any useful information. | ||
| - The user wants a truly decentralized solution and does not care about the gas cost of storing lots of data in blockchain; then they can write a stringified version of their authentication flows definition in the `authenticator` text record. | ||
| - Application will resolve only its issued names, therefore the authentication flows or their location are likely already known. In such cases the application can use its own backend to resolve `authenticator` instead of ENS or another Crypto Domain system NFT system. Responding with the stringified version of the saves the user of the extra request as it does not provide any useful information. | ||
|
|
||
| # Login With Name Flow |
There was a problem hiding this comment.
similarly, i think it might be clearer to lead with the macro-level flow first and then detail individual flows second and use-cases third?
| @@ -350,9 +337,9 @@ export class ENS implements NameResolver { | |||
|
|
|||
| # Rationale | |||
There was a problem hiding this comment.
it might be good to add a kind of high-level rationale, like "this doesn't compete with other authentication method, it is just an opt-in discovery system that name-controllers can use and that applications can trust to automate one friction point."
it might also be the right section to explain how future authentication methods (#teamWearables) get added to the mix-- followup CAIPs? are you providing a template for those? 😄
There was a problem hiding this comment.
Added. I imagine the followups should come in the new proposals themselves.
Or in another CAIP that updates this one with the platforms and connections that start getting popular and used in web3
CAIPs/caip-275.md
Outdated
| @@ -361,8 +348,8 @@ Existing NFTs and applications will continue to function normally. | |||
|
|
|||
| ## Reference Implementation | |||
There was a problem hiding this comment.
Not to be a stickler, but the two main differences between a sample implementation and a reference implementation is that
1.) ongoing maintenance - it has to still work for a long time, and you have to remove or rename this section if you stop maintaining it later
2.) all optional extensions supported -- i.e., you have to update (or at least accept PRs) if #teamWearables writes a CAIP a year from now adding logic for a wearables flow.
one dodge if you don't want to commit to that before going final with this CAIP is calling it a "sample implementation" 😅
There was a problem hiding this comment.
It is, in fact, a great point. And anyway I can definitely see big players providing their own infra or versions of the implementation to suit their needs
BTW, sample implementation is open to contributions 🙂
|
Hi @bumblefudge ! |
|
Sorry, I missed that last commit somehow. Reviewing now |
There was a problem hiding this comment.
LGTM, 2nd reviewer and we're good. @obstropolos ? @glitch003, @davidlsneider, @GregTheGreek ?
Applied suggested changes. Thanks!!! |
Following comments on #290 (and this comment) we expand some more on how to treat the
chainproperty in authentication config. Also we remove the need for an extra request that can cause issues or degrade UX when using different wallets for authentication and other things