Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update EIP-191: Improve Comprehensibility #5804

Merged
merged 5 commits into from
Nov 25, 2022
Merged

Update EIP-191: Improve Comprehensibility #5804

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1fcd98
improve EIP191 Standard
YamenMerhi Oct 19, 2022
0377a95
update description for EIP712
YamenMerhi Oct 27, 2022
0f462d1
Update EIPS/eip-191.md
YamenMerhi Nov 15, 2022
20cbd13
update example code snippet
YamenMerhi Nov 16, 2022
f7acb2a
remove external links and update naming
YamenMerhi Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 36 additions & 12 deletions EIPS/eip-191.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ We propose the following format for `signed_data`
```
0x19 <1 byte version> <version specific data> <data to sign>.
```
Version `0` has `<20 byte address>` for the version specific data, and the `address` is the intended validator. In the case of a Multisig wallet, that is the wallet's own address .

The initial `0x19` byte is intended to ensure that the `signed_data` is not valid [RLP](https://github.com/ethereum/wiki/wiki/RLP)

Expand All @@ -55,29 +54,54 @@ Using `0x19` thus makes it possible to extend the scheme by defining a version `
| `0x01` | [712][eip-712] | Structured data
| `0x45` | [191][eip-191] | `personal_sign` messages

#### Version `0x00`

```
0x19 <0x00> <intended validator address> <data to sign>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@YamenMerhi YamenMerhi Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

```

The version `0x00` has `<intended validator address>` for the version specific data. In the case of a Multisig wallet that perform an execution based on a passed signature, the validator address is the address of the Multisig itself. The data to sign could be any arbitrary data.

#### Version `0x01`

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


#### Version `0x45` (E)

```
0x19 <0x45 (E)> <thereum Signed Message:\n" + len(message)> <data to sign>
```

The version `0x45` (E) has `<thereum Signed Message:\n" + len(message)>` for the version specific data. The data to sign could be any arbitrary data.
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved

[EIP-191]: ./eip-191.md
[EIP-712]: ./eip-712.md

### Example

The following snippet has been written in Solidity 0.5.0.
The following snippets has been written in Solidity 0.8.0.

#### Version `0x00`

```solidity
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);
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved

// logic of the wallet
if (addressRecovered == owner) executeOnTarget(target,payload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
```

## Copyright

Copyright and related rights waived via [CC0](../LICENSE.md).