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.

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

ERC: Executable Signed Messages refunded by the contract #1077

Merged
merged 7 commits into from May 15, 2018

Conversation

alexvandesande
Copy link

@alexvandesande alexvandesande commented May 14, 2018

Allowing users to sign messages to show intent of execution, but allowing a third party relayer to execute them is an emerging pattern being used in many projects. Standardizing a common format for them, as well as a way in which the user allows the transaction to be paid in tokens, gives app developers a lot of flexibility and can become the main way in which app users interact with the Blockchain.

alexvansande and others added 5 commits May 14, 2018 12:16
Allowing users to sign messages to show intent of execution, but allowing a third party relayer to execute them is an emerging pattern being used in many projects. Standardizing a common format for them, as well as a way in which the user allows the transaction to be paid in tokens, gives app developers a lot of flexibility and can become the main way in which app users interact with the Blockchain.
@Arachnid
Copy link
Contributor

@alexvandesande I've edited the doc to fix the header formatting, but you still have build failures - please see the details message and fix.

@alexvandesande
Copy link
Author

Ok, it seems the issue was a dependency on #735 which doesn't exist in the new format. Removed it as it wasn't very important for the standard.

@alexvandesande
Copy link
Author

@Arachnid
The new build fails because it says 'replaces #877' which is in draft but not merged. Should I:

  1. remove the 877 reference (which unfortunately removes the information that this has been tried before)
  2. adapt 877 to the new format and have it merged (only for historical purposes, since I do not stand behind that one anymore)

@Arachnid
Copy link
Contributor

@alexvandesande It's up to you - either of those are fine.

@alexvandesande
Copy link
Author

It's ready to merge now. The other depends on this

@Arachnid Arachnid merged commit 7b56fab into ethereum:master May 15, 2018
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Jul 19, 2018
* ERC: Executable Signed Messages refunded by the contract

Allowing users to sign messages to show intent of execution, but allowing a third party relayer to execute them is an emerging pattern being used in many projects. Standardizing a common format for them, as well as a way in which the user allows the transaction to be paid in tokens, gives app developers a lot of flexibility and can become the main way in which app users interact with the Blockchain.

* Update eip-1077.md

* Update eip-1077.md

* Added uPort and Gnosis examples

* Swarm city

* change dependencies

* moves reference to 877
@shine2lay
Copy link

I've implemented a version of this standard here: https://github.com/shine2lay/SmartContracts

@alexvandesande I have a question about gasEstimate function, can we just use web3.eth.estimateGas to get whether or not a transaction can be executed?

@PhABC
Copy link
Contributor

PhABC commented Aug 8, 2018

Hey @alexvandesande,

I am not convinced the extraHash argument is necessary in the first place and I think dataHash should not be a bytes32 hash, but arbitrary length bytes array. Because data would be of arbitrary length, extraData becomes a bit useless. I therefore suggest to remove extraHash and rename dataHash to data. I would also remove callPrefix since this should be part of the data byte array.

I think we should better detail what the gas refund process are. Currently, gasPrice specified by signer could be different from gasPrice used by executor, which provides an incentive for the executor to execute the transaction with the lowest gas price possible. This could be fine, but something I think we should mention. Using ERC20 tokens instead of ETH would need to adjust gasPrice properly, per specified by gasPrice in the signed message. It's not clear to me from the standard whether the gas refund is supposed to be profitable for the executor or not. Perhaps it would make more sense to calculate gasRefund and a bounty, both denominated in ETH or ERC20.

@wighawag
Copy link
Contributor

Hi all,

This is a great initiative.

Few comments:

1) It seems that the current draft focuses on contract wallet/identity contract as the sole processors of such messages.

But such message could also work at the token level. An ERC20/ERC777 token contract could process such messages and perform the transaction without being an identity contract itself.

As such the from field should not be necessarily the contract as it is described in the current draft but could also be an external address.
To avoid replay attacks on other smart contracts, we can add a contract field that would serve the same purpose. I don't think we need callPrefix though.

2) There are several ambiguities that might make hard for users to know what they are signing.

The draft states :

How the contract interprets the intended action depends on its purpose. For instance a token contract can decide to implement it in a way that interprets all actions as token transfers, and uses the  value  to mean token value and  bytecode  as the data to pass to the recipient contract

This would be a nightmare for wallet UX. I think we should define unambiguously what each field represent so that a wallet receiving such message signature request could let the user know what is being transfered.

It also states :

Executes the signed message. Execution usually means that a contract will execute a  call  to the  to  address, with  value amount of ether and  data  as its data. But in some special cases, a token can decide instead to interpret it specifically as executing a  transferAndCall(to,from,data)  or the equivalent.

Again I think the standard would benefit in being less ambiguous and define exactly what the smart contract should do so wallet can inform the users accurately. If want different behavior, we add more signature format options

@wighawag
Copy link
Contributor

@PhABC
Not sure if you misunderstood but the dataHash is just used for the hash to be signed. The data sent as part of the transaction will contains all the bytes needed.

if we wanted to add extra options to the message format we would not necessarily want to pollute the data being used to call a function. Else this would require the contract processing the message to extract and remove that info from data before making the call to to hence the usefulness of extraHash

RegardingcallPrefix : if I understand correctly it is for the function of the identity contract itself that will perform the execution of the message, not for the function of to that will use the prefix contained in data
I guess it was added to prevent replay attack in the same contract in a different function. I don't think it is necessary though as such contract should not accept these messages through different pathways.

Good point about gasPrice, maybe the contract should refuse a call that use a smaller gasPrice that the one defined in the message.
Then for token based transaction, we should probably not reuse the same gasPrice field but add a tokenGasPrice field that is defined in term of gasPrice

This way the message signer is in control of the tx gasPrice (at least a minimum)

@wighawag wighawag mentioned this pull request Mar 2, 2019
@wighawag wighawag mentioned this pull request Mar 9, 2019
@PaulRBerg
Copy link
Contributor

Pardon the pedantic comment @alexvandesande, but I think I spotted a typo here:

operationType type will define what sort of operation will be executed (in assembly): 0 for a standard call, 1 for a DelegateCall and 0 for a create opcode.

Shouldn't that be "2" or other integer for the create opcode?

@3esmit
Copy link
Contributor

3esmit commented Sep 16, 2019

There are some problems with the current spec merged.

  1. The excess of parameters in the execution method is not only making the signature checking more expansive, as also impossible to compile in solidity due "stack too deep".
  2. Gas Abstraction, as a whole picture, is undefined

For 1. the best way is to have separate optional functions for each type supporting. The use of ERC165 could be useful here, although, when gas abstraction is achieved it wont matter.
2. Gas Abstraction is possible thanks to CREATE2. An update of the mining/validator node (no hard fork required) would be important to enable gas abstraction, so this "meta transactions" can be included directly by miners, which configure ERC20 they accept.

An optional EVM consensus change can be implemented to support "validator functions" in the contracts, which would enable block creators to call a contract without a sender, this special transactions cannot use msg.value or msg.sender and requires gas.price == 0, and only have destination and data, and if fails, then it would cause an invalid block. This transactions would be used to call the gas relay functions, which would use block.coinbase for the meta-transaction ERC20 gas payment. Suggested this here https://ethresear.ch/t/gas-abstraction-non-signed-block-validator-only-procedures/4388

For 1. I guess we should update the spec, for 2. I guess we need to fork an eth client, experiment, make some more EIPs to suggest what exactly would be changed for this soft fork and also for the potential hard fork.

Let me know what you think by replying here or you can e-mail me ricardo3 at status.im, or send me a message by whisper in Status.im: 3esmit.stateofus.eth

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

Successfully merging this pull request may close these issues.

None yet

8 participants