EIPs icon indicating copy to clipboard operation
EIPs copied to clipboard

Update EIP-191: Improve Comprehensibility

Open YamenMerhi opened this issue 3 years ago • 5 comments

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.

YamenMerhi avatar Oct 19 '22 20:10 YamenMerhi

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)

eth-bot avatar Oct 19 '22 20:10 eth-bot

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

YamenMerhi avatar Oct 20 '22 09:10 YamenMerhi

Ok thank you I truly appreciate all the help and wisdom I am learning thank you for your care...I will send more gracious gifts in the near future.

On Fri, Oct 28, 2022, 1:14 PM Sam Wilson @.***> wrote:

@.**** requested changes on this pull request.

In EIPS/eip-191.md https://github.com/ethereum/EIPs/pull/5804#discussion_r1008321635:

+0x19 <0x00> +```

+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] + +#### 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.

So this treats the E as the version? I would explicitly mention that the E is consumed, and thereum is not a typo :P

In EIPS/eip-191.md https://github.com/ethereum/EIPs/pull/5804#discussion_r1008326879:

 // 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);

Hm, I'm leery about changing this from plain ecrecover to what I assume is supposed to be OZ's. Their implementation has additional restrictions that plain ecrecover doesn't have. Changes to final EIPs are supposed to be backwards compatible.

In EIPS/eip-191.md https://github.com/ethereum/EIPs/pull/5804#discussion_r1008327763:

-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.

— Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/pull/5804#pullrequestreview-1160440212, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3H6XK6YKMDAJ2TAF3FJBPDWFQJZ3ANCNFSM6AAAAAARJPOIPU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Cryptonics1 avatar Oct 28 '22 19:10 Cryptonics1

@SamWilsn @Pandapip1 Applied the changes 👍

YamenMerhi avatar Nov 16 '22 08:11 YamenMerhi

@Pandapip1 Done 👍

YamenMerhi avatar Nov 16 '22 12:11 YamenMerhi