Update EIP-191: Improve Comprehensibility #5804

YamenMerhi · 2022-10-19T20:29:16Z

What does this PR introduce?

Updating the example to be more comprehensive + more updated.
Simplifying the EIP191 Standard, by providing more explanation of the different versions.
- Re-ordering the text so we start by:
  - The formula for the standard
  - Why we have 0x19
  - What are the different bytes version we have
- Explicitly mentioning that E is version 0x45and thereum Signed Message:\n" + len(message) is version specific data.

eth-bot · 2022-10-19T20:30:19Z

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

(fail) eip-191.md

classification
`updateEIP`

eip-191.md is in state final at the head commit, not draft or last call or review; an EIP editor needs to approve this change
This PR requires review from one of [@axic, @SamWilsn, @Pandapip1]
eip-191.md requires approval from one of (@holiman, @Arachnid)

YamenMerhi · 2022-10-20T09:52:25Z

I know this is an old standard and missing some sections, I am happy to add them.
Also was wondering if we should mention in a Security Consideration section that it's advised to use 0x00 for any execution based on signature as people could be easily tricked to sign normal 0x45 messages as it's normally used for SIWE and for offchain verification

holiman

Looks good to me, aside from one nitpick

EIPS/eip-191.md

holiman

LGTM

EIPS/eip-191.md

SamWilsn · 2022-10-28T18:12:53Z

EIPS/eip-191.md

-function submitTransactionPreSigned(address destination, uint value, bytes data, uint nonce, uint8 v, bytes32 r, bytes32 s)
-    public
-    returns (bytes32 transactionHash)
-{
+function signatureBasedExecution(address target, uint256 nonce, bytes memory payload, bytes memory signature)
+    public payable {
+
    // Arguments when calculating hash to validate
    // 1: byte(0x19) - the initial 0x19 byte
    // 2: byte(0) - the version byte
-    // 3: this - the validator address
-    // 4-7 : Application specific data
-    transactionHash = keccak256(abi.encodePacked(byte(0x19),byte(0),address(this),destination, value, data, nonce));
-    sender = ecrecover(transactionHash, v, r, s);
-    // ...
+    // 3: address(this) - the validator address
+    // 4-6 : Application specific data
+
+    bytes32 hash = keccak256(abi.encodePacked(byte(0x19), byte(0), address(this), msg.value, nonce, payload));
+
+    // recovering the signer from the hash and the signature
+    addressRecovered = ECDSA.recover(hash, signature);
+
+    // logic of the wallet
+    if (addressRecovered == owner) executeOnTarget(target,payload);


This is not the minimal change required to fix errata. Since this is a final EIP, we only allow clarifications and correcting errors. The minimal change would likely be just changing the 7 to a 6, unless there's something else I missed.

I see your point @SamWilsn , I will revert the OZ integration and keep plain ecrecover but what if the example is not clear ? Just for the sake of backwards compatibility we leave it like this ? (Knowing that no one will use this code snippet)

The name of the function IMO is not perfect, the same for the naming of variables, as it's not a TransactionPreSigned or a transactionHash, it's a signed message and hash of a signed message. This whole standard was created to find a standard way to differentiate between signed transactions and signed messages and here the naming is super confusing.

Also the example show how to sign but doesn't give info about what's the possibilities when signing the message. It's just like showing someone a fish hook but not teaching them how to start fishing 😄

I would find a balance between your suggestions and mine like that:

Reverted the OZ integration

Made the new logic as a comment just to emphasize on how this signed message could be used

Made change to the naming of the function and variables

function signatureBasedExecution(address target, uint256 nonce, bytes memory payload, uint8 v, bytes32 r, bytes32 s) public payable { // Arguments when calculating hash to validate // 1: byte(0x19) - the initial 0x19 byte // 2: byte(0) - the version byte // 3: address(this) - the validator address // 4-6 : Application specific data bytes32 hash = keccak256(abi.encodePacked(byte(0x19), byte(0), address(this), msg.value, nonce, payload)); // recovering the signer from the hash and the signature addressRecovered = ecrecover(hash, v, r, s); // logic of the wallet // if (addressRecovered == owner) executeOnTarget(target, payload); }

Please let me know what do you think 😄 🙏

I think you can go ahead and make this change to the PR. This version is a smaller change.

Co-authored-by: Pandapip1 <[email protected]>

YamenMerhi · 2022-11-16T08:34:14Z

@SamWilsn @Pandapip1 Applied the changes 👍

Pandapip1

On line 39, ERC needs to be changed to EIP. On line 35, the external link needs to be removed.

YamenMerhi · 2022-11-16T12:54:15Z

@Pandapip1 Done 👍

Pandapip1

+1 from me. Will need manual merge.

Changes addressed

xinbenlv · 2022-11-19T23:30:16Z

EIPS/eip-191.md

+#### Version `0x00`
+
+```
+0x19 <0x00> <intended validator address> <data to sign>


Wait, I must be missing something here, are we adding the <0x00> <intended validator address> to the original EIP-191?

@xinbenlv The version 0x00 was always there in the standard from the start, but it was not explained very well, and that's one of the purposes of the PR

xinbenlv · 2022-11-19T23:32:20Z

EIPS/eip-191.md

+
+#### Version `0x01`
+
+The version `0x01` is for structured data as defined in [EIP-712]


QQ: Should we mention anything about the EIP-712 "domain" besides the "structured data" here?

@xinbenlv According to @holiman the co-author of the EIP191 standard:

I'd prefer to leave the definition fully to EIP-712. So simply say:
The version 0x01 is for structured data as defined in EIP-712

improve EIP191 Standard

d1fcd98

YamenMerhi requested a review from eth-bot as a code owner October 19, 2022 20:29

github-actions bot added c-update Modifies an existing proposal s-final This EIP is Final t-erc labels Oct 19, 2022

Pandapip1 requested review from Arachnid and holiman October 19, 2022 22:26

Pandapip1 added the a-review Waiting on author to review label Oct 19, 2022

Pandapip1 changed the title ~~improve EIP191 Standard~~ Update EIP-191: Improve Comprehensibility Oct 19, 2022

YamenMerhi mentioned this pull request Oct 21, 2022

Implement 0x00 version of EIP191 in ECDSA Library OpenZeppelin/openzeppelin-contracts#3772

Closed

holiman reviewed Oct 27, 2022

View reviewed changes

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

update description for EIP712

0377a95

holiman previously approved these changes Oct 27, 2022

View reviewed changes

Pandapip1 previously approved these changes Oct 27, 2022

View reviewed changes

Pandapip1 added this to the Manual Merge Queue milestone Oct 27, 2022

eth-bot enabled auto-merge (squash) October 27, 2022 13:06

eth-bot previously approved these changes Oct 27, 2022

View reviewed changes

SamWilsn previously requested changes Oct 28, 2022

View reviewed changes

This comment was marked as spam.

Sign in to view

Pandapip1 removed this from the Manual Merge Queue milestone Nov 5, 2022

poojaranjan mentioned this pull request Nov 14, 2022

EIP Editing Office Hour Meeting 6 ethcatherders/EIPIP#188

Closed

auto-merge was automatically disabled November 15, 2022 12:39
Head branch was pushed to by a user without write access

Update EIPS/eip-191.md

0f462d1

Co-authored-by: Pandapip1 <[email protected]>

YamenMerhi dismissed stale reviews from eth-bot, Pandapip1, and holiman via 0f462d1 November 15, 2022 12:39

update example code snippet

20cbd13

Pandapip1 suggested changes Nov 16, 2022

View reviewed changes

remove external links and update naming

f7acb2a

Pandapip1 approved these changes Nov 16, 2022

View reviewed changes

Pandapip1 added this to the Manual Merge Queue milestone Nov 16, 2022

xinbenlv reviewed Nov 19, 2022

View reviewed changes

SamWilsn merged commit b26436b into ethereum:master Nov 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update EIP-191: Improve Comprehensibility #5804

Update EIP-191: Improve Comprehensibility #5804

YamenMerhi commented Oct 19, 2022

eth-bot commented Oct 19, 2022 •

edited

Loading

YamenMerhi commented Oct 20, 2022

holiman left a comment

holiman left a comment

SamWilsn Oct 28, 2022

YamenMerhi Oct 31, 2022

SamWilsn Nov 15, 2022

This comment was marked as spam.

YamenMerhi commented Nov 16, 2022

Pandapip1 left a comment

YamenMerhi commented Nov 16, 2022

Pandapip1 left a comment

xinbenlv Nov 19, 2022

YamenMerhi Nov 20, 2022 •

edited

Loading

xinbenlv Nov 19, 2022

YamenMerhi Nov 20, 2022


		#### Version `0x01`

		The version `0x01` is for structured data as defined in [EIP-712]

Update EIP-191: Improve Comprehensibility #5804

Update EIP-191: Improve Comprehensibility #5804

Conversation

YamenMerhi commented Oct 19, 2022

What does this PR introduce?

eth-bot commented Oct 19, 2022 • edited Loading

(fail) eip-191.md

YamenMerhi commented Oct 20, 2022

holiman left a comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

SamWilsn Oct 28, 2022

Choose a reason for hiding this comment

YamenMerhi Oct 31, 2022

Choose a reason for hiding this comment

SamWilsn Nov 15, 2022

Choose a reason for hiding this comment

This comment was marked as spam.

YamenMerhi commented Nov 16, 2022

Pandapip1 left a comment

Choose a reason for hiding this comment

YamenMerhi commented Nov 16, 2022

Pandapip1 left a comment

Choose a reason for hiding this comment

xinbenlv Nov 19, 2022

Choose a reason for hiding this comment

YamenMerhi Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

xinbenlv Nov 19, 2022

Choose a reason for hiding this comment

YamenMerhi Nov 20, 2022

Choose a reason for hiding this comment

eth-bot commented Oct 19, 2022 •

edited

Loading

YamenMerhi Nov 20, 2022 •

edited

Loading