Skip to content
This repository has been archived by the owner on May 25, 2019. It is now read-only.

Format of signature #6

Open
s1na opened this issue Sep 20, 2018 · 6 comments
Open

Format of signature #6

s1na opened this issue Sep 20, 2018 · 6 comments

Comments

@s1na
Copy link
Collaborator

s1na commented Sep 20, 2018

One of the differences I have seen among projects is how they transmit the signature of the meta tx, with some sending v, r, s fields separately, and some using an encoded version.

What are the arguments for each? For an encoded signature, which EIP does it adhere to (EIP-191 or EIP-712)?

@austintgriffith
Copy link
Collaborator

This is a really great place to start! I've seen it done both ways and I would love to hear the pros and cons of each. I used the encoded version because that's what came with the ECDSA contracts from OpenZeppelin. To split the bytes they do:

    assembly {
      r := mload(add(signature, 32))
      s := mload(add(signature, 64))
      v := byte(0, mload(add(signature, 96)))
    }

The gas cost of this operation is minimal but I think that is the argument for splitting off-chain. However, keeping it together as bytes means you have more room for other arguments and variables on the stack. I don't know about you guys but "Stack Too Deep" errors are my worst enemy.

@Kyrrui
Copy link

Kyrrui commented Sep 20, 2018

Yeah, we were passing the V, R, and S separately for the assumed increased gas costs. (We hadn't tested these assumptions to see the true cost of doing this operation on chain). Have not run into the "Stack Too Deep" error, but I would agree that if the cost is negligible then the standard should follow that we do the operation on chain for cleaner smart contract code readability and to Austin's point, to allow for more arguments.

@radek1st
Copy link

radek1st commented Sep 21, 2018

Same here, I just assumed it will cost more gas, but I haven't tested the exact amount. My line of thought is that even a small saving on gas can make a huge difference if this is going to happen on every transaction. With regards to "Stack Too Deep" I've hit it sometimes, but normally walk around it by shuffling local variables.

@s1na
Copy link
Collaborator Author

s1na commented Sep 23, 2018

I definitely agree with the gas concern. On the other hand, going through EIP-191, these concerns have been raised for sending signature parts separetely (in multisig wallets):

  • An Ethereum TX that was signed by the user, which has the format RLP<nonce, gasPrice, startGas, to, value, data>, r, s, v, can be republished to the relay network and submitted to the network. I'm not sure if this would likely cause an issue (if the implementation doesn't use the same encoding, and does validation), due to nonce mismatch.
  • If multiple projects adopt the same standard, their meta-txes couldn't be distinguished from one another. I think then It'd be possible to submit a meta tx the user makes in one wallet, to another one.

And, because meta txes are structured, adopting EIP-712, which kind of supersedes EIP-191 and seems to be finding consensus among developers, would help with usability and security. Metamask is adding support soon, and the Gnosis team also mentioned, they will probably make use of it for Safe.

@pet3r-pan
Copy link
Member

Previous chat logs from the telegram group (archival purposes) https://hackmd.io/NPzA_EEFTuWR_DnnEEHOyw

@rmeissner
Copy link

If you have 1 signature it should be more gas efficient to pass the parts in separately (since bytes would be a dynamic type that is automatically copied to memory and therfore has some overhead). You can actually optimize this by hacking around a little in solitidy (external functions don't automatically copy their types into memory if not used).

The thing is that I would suggest to support multiple signatures and also standards like EIP-1271. For this it is way more dynamic to use a bytes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants