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
Add ERC: Single Sign-On for Account Discovery #99
base: master
Are you sure you want to change the base?
Conversation
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 should avoid referring to normal accounts (aka EOAs) as "addresses"
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
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.
Moar feedback 😁
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: Pedro Gomes <pedrogomes94@gmail.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
@lightclient can we merge as draft please :) |
ERCS/erc-7555.md
Outdated
```= | ||
type SIGNER_TYPE: string = "secp256k1" | "P256" | "BLS12-381"; | ||
``` | ||
The `signer_key` should be returned in the did:key format, as specified by the W3C. |
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.
Please link directly to the relevant standard using the format specified in EIP-1. Something like this maybe:
[did:key format](https://www.w3.org/TR/2022/REC-did-core-20220719/)
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.
I had this before, but the bot forced me to remove it
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.
It has to match this exact regular expression:
^https://www\.w3\.org/TR/[0-9][0-9][0-9][0-9]/.*$
Let me know if you have more trouble with it!
|
||
## Specification | ||
The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. | ||
|
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.
More of a suggestion than a hard requirement for merging, but I think providing an overview section that describes how everything works would be much appreciated.
```= | ||
parameters: | ||
- in: query | ||
name: redirect_uri | ||
schema: | ||
type: string | ||
description: The uri that the provider should redirect back to. | ||
- in: query | ||
name: chain_id | ||
schema: | ||
type: string | ||
description: The chain_id of a given 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.
Are these OpenAPI or something? If so, I'd be explicit about what format they're in and provide the full schema somewhere.
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.
It's from swagger, can switch to OpenAPI?
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.
You can use any format you like! Just note in the document what that format is so readers are aware.
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
There will be a small overhaul on some semantics post Denver workshops. Will update this week or next |
There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
name: tx_hash | ||
schema: | ||
type: string | ||
description: The hash of the transaction |
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.
for a 4337 account, is this the userOpHash
or the userOp bundle'stxHash
or something else?
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.
and in the case of a multisig or something that wasn't expected to be completed immediately, how shoudl the provider respond here?
|
||
##### Request Syntax | ||
```= | ||
https://<PROVIDER_URI>/auth/? |
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.
sorry if this is addressed elsewhere but where does the dapp get the PROVIDER_URI
from?
## Security Considerations | ||
|
||
<!-- Needs discussion. --> | ||
- Is there a concern that a user can spoof another persons address, and that could be malicious? For example, circumventing the provider, and manually calling the redirect_url with a chosen address. A way around this would be having the user actually sign a challenge message, perhaps leveraging SIWE. |
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.
afaik SIWE doesn't supper eip 6492 atm which seems potentially problematic for 4337 accounts
having said that, I don't see how manually calling the redirect_url
with a spoofed address could accomplish anything malicious. Ultimately dispatching a txn depends on the provider /sendTransaction
endpoint which can't be spoofed unless I'm missing something
--- | ||
eip: 7555 | ||
title: Single Sign-On for Account Discovery | ||
description: Discover accounts using a signing key that do not use the secp256k1 curve. |
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.
it's really just any smart contract accounts right? (e.g. this could be used to connect to a safe without going through WalletConnect iiuc)
smart_account_address=<SMART_ACCOUNT_ADDRESS> | ||
``` | ||
|
||
#### sendTransaction Route |
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 you need a separate route for getting [or in the case of multisig initiating?] a signature?
When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md
We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met: