Mark EIP-4361 for Review #5034

obstropolos · 2022-04-24T10:43:47Z

Marks EIP-4361 from Draft stage to Review

eth-bot · 2022-04-24T10:44:25Z

All tests passed; auto-merging...

(pass) eip-4361.md

classification
`updateEIP`

passed!

(pass) assets/eip-4361/example.js

classification
`ambiguous`

file assets/eip-4361/example.js is associated with EIP 4361; because there are also changes being made to EIPS/eip-4361.md all changes to corresponding assets are also allowed

(pass) assets/eip-4361/package.json

classification
`ambiguous`

file assets/eip-4361/package.json is associated with EIP 4361; because there are also changes being made to EIPS/eip-4361.md all changes to corresponding assets are also allowed

(pass) assets/eip-4361/signing.png

classification
`ambiguous`

file assets/eip-4361/signing.png is associated with EIP 4361; because there are also changes being made to EIPS/eip-4361.md all changes to corresponding assets are also allowed

SamWilsn

You'll have to remove the external links from the "Reference Implementation" section. If it's relatively short/self-contained, you can add it under the assets/eip-4361 directory of this repository.

Otherwise, I think this looks good to go to review!

obstropolos · 2022-05-03T15:53:36Z

You'll have to remove the external links from the "Reference Implementation" section. If it's relatively short/self-contained, you can add it under the assets/eip-4361 directory of this repository.

Otherwise, I think this looks good to go to review!

Updated to remove the section from the EIP and migrated it into the assets dir for EIP-4361

SamWilsn · 2022-05-07T00:07:48Z

assets/eip-4361/Notes.md

+## Reference Implementation
+The reference implementation is in TypeScript and available at [https://github.com/spruceid/siwe](https://github.com/spruceid/siwe). It is also available on [NPM](https://www.npmjs.com/package/siwe).


Sorry if I wasn't clear, but generally we don't allow external links (even in the assets directory.)

What I meant was that you can include the reference implementation directly (not as a submodule) to this directory.

Edit: You should also keep the Reference Implementation section:

## Reference Implementation A reference implementation is available under [../assets/eip-4361](../assets/eip-4361).

You can't actually link to a directory like this. You have to link to a specific file (there is no directory listing capabilities in the rendered page).

Hey @MicahZoltu in this case would it be best practice to link to the readme in the assets folder?

That is one option. My general recommendation is to trim the reference implementation down to one file if possible. An ideal reference implementation focuses exclusively on the stuff that matters for a standard, and doesn't include a bunch of unrelated things that aren't critical for understanding/following the standard. When this is done, one can usually get the reference implementation down to something pretty small. If the reference implementation is still bigger than one file after this, it usually means the EIP is trying to do too much and is a good indication that perhaps it should be split up into multiple smaller EIPs that address more constrained issues.

Informative Fixes Across the Spec

lightclient

A few requests:

please remove the .DS_Store files
please rename Notes.md to README.md
remove period after "Ethereum" in title

EIPS/eip-4361.md

SamWilsn · 2022-05-27T18:22:19Z

Please see https://github.com/obstropolos/EIPs/pull/2 for additional editor feedback.

EIP-4361 feedback

Update EIP-4361 Contract Address Guidelines

obstropolos · 2022-05-31T21:26:44Z

Please see obstropolos#2 for additional editor feedback.

Thanks again for the additional feedback! It has been merged in.

Let me know if there's anything else here!

EIPS/eip-4361.md

obstropolos · 2022-06-21T14:02:28Z

Latest changes made based on comments. Thanks again @SamWilsn!

lightclient

Looking pretty good other than a nit on ordering of sections.

EIPS/eip-4361.md

lightclient · 2022-06-28T15:13:35Z

EIPS/eip-4361.md

-
-    - EIP-712 has the advantage of on-chain representation and on-chain verifiability, such as for their use in metatransactions, but this feature is not relevant for the specification's scope. **(2)**
- Why not use JWTs? Wallets don't support JWTs. The keccak hash function is [not assigned by IANA](https://www.iana.org/assignments/jose/jose.xhtml) for use as a JOSE algorithm. **(2, 3)**
+- Why [EIP-191](./eip-191.md) (Signed Data Standard) over [EIP-712](./eip-712.md) (Ethereum typed structured data hashing and signing)


I think EIP-712 is supported in all major wallets? I realize it isn't final status and we are working on it - I wouldn't want that to affect your decision on using 191 and not 712.

This specifically dealt with display to the end-user. Specifically where the wallet has no custom view for something like SIWE and defaults to the basics - the person is presented with a wall of JSON [712] instead of a personal sign message which has the advantage of being human-readable.

SamWilsn · 2022-07-17T21:23:52Z

@obstropolos might need to close and reopen the PR to get the required checks to run.

obstropolos · 2022-07-18T05:02:08Z

@obstropolos might need to close and reopen the PR to get the required checks to run.

Closing and reopening

SamWilsn · 2022-07-18T05:03:53Z

EIPS/eip-4361.md

 author: Wayne Chang (@wyc), Gregory Rocco (@obstropolos), Brantly Millegan (@brantlymillegan), Nick Johnson (@Arachnid)
 discussions-to: https://ethereum-magicians.org/t/eip-4361-sign-in-with-ethereum/7263
-status: Draft
+status: Review
 type: Standards Track
 category: ERC
 created: 2021-10-11


Ah, you'll need to sort out the following line.

Looks like EIP-1328 is stagnant. It'll need to go into review before this one can.

* Mark EIP-4361 for Review * Move reference impl section out of EIP * This contains some fixes, only informative, no normative changes... * fix: fixed reference links * fix: added reference implementation based on v1.1.6 * fix: undoing must since not in the core spec * fix: fixed typo * remove licenses from ref impl and update with file locations * updated spec and added new reference implementation * removed period from title * Remove added .DS_store * EIP-4361 editor feedback * Update EIP-4361 Contract Address Guidelines * Update EIP references, rationale section name, and title * Moved security considerations section to proper location Co-authored-by: Oliver Terbu <[email protected]> Co-authored-by: Sam Wilson <[email protected]> Co-authored-by: Wayne Chang <[email protected]>

Mark EIP-4361 for Review

f831e19

eth-bot added the statusChange label Apr 24, 2022

SamWilsn requested changes Apr 29, 2022

View reviewed changes

Move reference impl section out of EIP

3c0d632

eth-bot added the ambiguous label May 3, 2022

SamWilsn requested changes May 7, 2022

View reviewed changes

Oliver Terbu and others added 7 commits May 13, 2022 12:59

This contains some fixes, only informative, no normative changes...

5bdf59e

fix: fixed reference links

45fcc8a

fix: added reference implementation based on v1.1.6

5389711

fix: undoing must since not in the core spec

efdaa97

fix: fixed typo

04c40dc

Informative Fixes Across the Spec

f006eec

Informative Fixes Across the Spec

remove licenses from ref impl and update with file locations

ff712b1

poojaranjan mentioned this pull request May 17, 2022

EIP editor apprenticeship meeting 17 ethcatherders/EIPIP#135

Closed

lightclient requested changes May 17, 2022

View reviewed changes

EIPS/eip-4361.md Outdated Show resolved Hide resolved

obstropolos added 4 commits May 19, 2022 14:38

updated spec and added new reference implementation

a08cdc6

removed period from title

e42d75e

Remove added .DS_store

335a9d1

Merge branch 'master' into obstropolos/eip-4361-review

5d7c118

obstropolos requested a review from lightclient May 25, 2022 17:26

EIP-4361 editor feedback

c64afc3

obstropolos and others added 3 commits May 31, 2022 17:15

Merge pull request #2 from SamWilsn/eip-4361-sam-feedback

aa620cf

EIP-4361 feedback

Update EIP-4361 Contract Address Guidelines

3eda4b1

Merge pull request #3 from obstropolos/fix/contract-addrs

b26d9eb

Update EIP-4361 Contract Address Guidelines

SamWilsn reviewed Jun 17, 2022

View reviewed changes

EIPS/eip-4361.md Outdated Show resolved Hide resolved

EIPS/eip-4361.md Outdated Show resolved Hide resolved

EIPS/eip-4361.md Outdated Show resolved Hide resolved

Update EIP references, rationale section name, and title

de8fd05

poojaranjan mentioned this pull request Jun 28, 2022

EIP editor apprenticeship meeting 20 ethcatherders/EIPIP#145

Closed

lightclient requested changes Jun 28, 2022

View reviewed changes

Moved security considerations section to proper location

193904f

obstropolos requested a review from lightclient July 11, 2022 04:36

SamWilsn approved these changes Jul 17, 2022

View reviewed changes

eth-bot added updateEIP and removed statusChange labels Jul 17, 2022

eth-bot enabled auto-merge (squash) July 17, 2022 21:18

obstropolos closed this Jul 18, 2022

auto-merge was automatically disabled July 18, 2022 05:02
Pull request was closed

obstropolos reopened this Jul 18, 2022

SamWilsn requested changes Jul 18, 2022

View reviewed changes

kodiakhq bot merged commit 38e6cc6 into ethereum:master Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark EIP-4361 for Review #5034

Mark EIP-4361 for Review #5034

obstropolos commented Apr 24, 2022

eth-bot commented Apr 24, 2022 •

edited

Loading

SamWilsn left a comment

obstropolos commented May 3, 2022

SamWilsn May 7, 2022 •

edited

Loading

MicahZoltu May 7, 2022

obstropolos May 14, 2022

MicahZoltu May 17, 2022

lightclient left a comment

SamWilsn commented May 27, 2022

obstropolos commented May 31, 2022

obstropolos commented Jun 21, 2022

lightclient left a comment

lightclient Jun 28, 2022

obstropolos Jun 28, 2022

SamWilsn commented Jul 17, 2022

obstropolos commented Jul 18, 2022

SamWilsn Jul 18, 2022 •

edited

Loading

		## Reference Implementation
		The reference implementation is in TypeScript and available at [https://github.com/spruceid/siwe](https://github.com/spruceid/siwe). It is also available on [NPM](https://www.npmjs.com/package/siwe).

Mark EIP-4361 for Review #5034

Mark EIP-4361 for Review #5034

Conversation

obstropolos commented Apr 24, 2022

eth-bot commented Apr 24, 2022 • edited Loading

All tests passed; auto-merging...

(pass) eip-4361.md

(pass) assets/eip-4361/example.js

(pass) assets/eip-4361/package.json

(pass) assets/eip-4361/signing.png

SamWilsn left a comment

Choose a reason for hiding this comment

obstropolos commented May 3, 2022

SamWilsn May 7, 2022 • edited Loading

Choose a reason for hiding this comment

MicahZoltu May 7, 2022

Choose a reason for hiding this comment

obstropolos May 14, 2022

Choose a reason for hiding this comment

MicahZoltu May 17, 2022

Choose a reason for hiding this comment

lightclient left a comment

Choose a reason for hiding this comment

SamWilsn commented May 27, 2022

obstropolos commented May 31, 2022

obstropolos commented Jun 21, 2022

lightclient left a comment

Choose a reason for hiding this comment

lightclient Jun 28, 2022

Choose a reason for hiding this comment

obstropolos Jun 28, 2022

Choose a reason for hiding this comment

SamWilsn commented Jul 17, 2022

obstropolos commented Jul 18, 2022

SamWilsn Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

eth-bot commented Apr 24, 2022 •

edited

Loading

SamWilsn May 7, 2022 •

edited

Loading

SamWilsn Jul 18, 2022 •

edited

Loading