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

Meta Txs: Adding support for meta transactions in aragon apps (Part 1) #526

Open
wants to merge 19 commits into
base: next
from

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

commented May 9, 2019

The main idea of this PR is to add support for meta transactions to allow any member of a DAO to interact with its apps without having funds necessarily.

I've also started working on the off-chain functionality for the corresponding service here.

Approach

The main idea is to have shared trusted off-chain service in charge of submitting the transactions to an on-chain relayer that will be in charged of signature validation, nonce validation, and will forward these transactions to the target app if all the verifications passed. The off-chain service will get refunded by the on-chain relayer for every transaction it relays. This will allow every to manage and customize their own relayers as they want.

The following table shows the different costs added per transaction to support this approach:

Gas cost
Off-chain service auth 0.5k
Refund quota validation 1.8k
Nonce validation 0.4k
Signature validation 8.3k
Log relayed transaction 3k
Reset last nonce 5.1k
Reset refunds amount 5.5k
Off-chain service refund 7.6k

Among other things, the total gas overload for a relayed transaction is ~53k of gas


Other approaches investigated

I've been exploring other approaches for this topic based on the same concept of having a shared trusted off-chain service in charge of submitting the transactions on-chain that will get refunded by the DAOs for every transaction it relays to make sure it doesn't run out of funds. The different alternatives explored are the possible combinations between the following patterns:

  1. Having an on-chain relayer in charge of signature validation, nonce validation, and refunds, that will act as a middleman between the off-chain service and the target app, vs. integrating all these responsibilities within the AragonApp itself.
  2. Parameterizing the transaction signer to the functions of the target app, vs. using the volatile storage approach to query it when needed without changing the app interface.

@facuspagnuolo facuspagnuolo self-assigned this May 9, 2019

Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/RelayerAragonAppWithParameterizedSender.sol Outdated

@facuspagnuolo facuspagnuolo referenced this pull request May 10, 2019

Open

Support meta-txs #87

4 of 5 tasks complete

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from 80468e6 to 6837a45 May 10, 2019

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from 6837a45 to d83a209 May 10, 2019

@facuspagnuolo facuspagnuolo changed the title [PoC] Meta Txs: Measure gas ovearload [PoC] Adding support for meta transactions in aragon apps May 10, 2019

@facuspagnuolo facuspagnuolo added the wip label May 10, 2019

@facuspagnuolo facuspagnuolo marked this pull request as ready for review May 10, 2019

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from eab6b72 to ada4788 May 13, 2019

@bingen
Copy link
Member

left a comment

More than comments I left a bunch of questions, I'm afraid I'm not fully getting it 😅
Besides, about the volatile storage, I think the initial idea was counting on EIP 1283, which I think didn't get in because of that re-entrancy issue, so it seems we are not going to save those 10k extra gas.

Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/BaseRelayer.sol Outdated
Show resolved Hide resolved contracts/relayer/RelayedAragonAppWithParameterizedSender.sol Outdated
Show resolved Hide resolved contracts/relayer/RelayerAragonAppWithParameterizedSender.sol Outdated
Show resolved Hide resolved contracts/relayer/RelayerAragonAppWithVolatileSender.sol Outdated
@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Thanks for your review @bingen! Although, I think it is more interesting to review directly the last approach we decided to follow :)

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@izqui I've decided to drop the idea of calculating the amount of gas to be refunded to the off-chain service since it was actually taking a lot of gas to perform such calculation (I reached an overload of 100k gas for some edge cases). That said, I came up with the idea of delegating the decision of calculating the gas refund to the DAOs' members. Note that off-chain services can tell whether the submitted amount of gas actually covers the costs of a transaction before relaying it. Therefore, DAOs' members will be incentivized to estimate gas values correctly.

To achieve this, I propose using a monthly quota on the relayer side to limit the amount of txs users can relay. And ofc, this triggers some new questions we should discuss:

  • Should we allow to change quotas amount and how?
  • Should we allow to change quotas based period of time and how?
  • Should we have different quotas per address or is it okay to have the same one for all members?
  • Should we catch reverting relayed calls to ensure we always refund gas costs or should the off-chain service check if the tx will revert before relaying it?

@facuspagnuolo facuspagnuolo changed the title [PoC] Adding support for meta transactions in aragon apps Meta Txs: Adding support for meta transactions in aragon apps May 14, 2019

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from 03fd3af to ec06318 May 14, 2019

@aragon aragon deleted a comment from coveralls May 14, 2019

facuspagnuolo added some commits May 15, 2019

@izqui

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Therefore, DAOs' members will be incentivized to estimate gas values correctly.

This could be an issue if the 'service provider' can relay their own transactions (and in the current implementation it seems like any account can use the relayer), as their incentive would be to use a high gas limit/price.

Should we allow to change quotas amount and how?

I think we should, as the amount of activity that happens in the organization will change over time and the gas price can fluctuate.

Should we allow to change quotas based period of time and how?

I don't think this is really important if the amount can be changed.

Should we have different quotas per address or is it okay to have the same one for all members?

I think that having a unique quota amount for everyone should be fine for a first version.

Should we catch reverting relayed calls to ensure we always refund gas costs or should the off-chain service check if the tx will revert before relaying it?

I'd say the off-chain service should make sure that the transaction won't revert.

@izqui
Copy link
Member

left a comment

A couple of aspects that we should think about:

  • Should the Relayer only perform calls to other apps in the same DAO that it is installed in? Detecting whether an app is installed in a given DAO cannot be done on-chain, but we could check whether the to address is either the Kernel or AragonApp(to).kernel() is the Kernel of the DAO.

  • Right now, the Relayer can relay actions signed by any from account. We should implement some sort of authorization mechanism (using the ACL?) so the relayer only relays calls from authorized accounts. We should also look into supporting immediate forwarders like the Token Manager (see #521) as it is the way that 'membership' is managed in most organizations.

Show resolved Hide resolved contracts/relayer/Relayer.sol Outdated
Show resolved Hide resolved contracts/relayer/Relayer.sol Outdated
@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@izqui I already addressed your comments, feel free to take another look :)
I'll publish all the off-chain service stuff on a separate PR

PS: The CI coverage task is failing since testrpc does not support eth_signTypedData :/

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch 2 times, most recently from c85ed73 to 2759f3b May 16, 2019

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from 2759f3b to 3b91caa May 17, 2019

@facuspagnuolo facuspagnuolo changed the title Meta Txs: Adding support for meta transactions in aragon apps Meta Txs: Adding support for meta transactions in aragon apps (Part 1) May 17, 2019

@facuspagnuolo facuspagnuolo removed the wip label May 17, 2019

@bingen

bingen approved these changes May 21, 2019

Copy link
Member

left a comment

Awesome job! The signer address append to calldata is pretty neat!!!

// Constant values used to identify the domain for the current app following EIP 712 spec
string private constant EIP_712_DOMAIN_NAME = "Aragon Relayer";
string private constant EIP_712_DOMAIN_VERSION = "1";
uint256 private constant EIP_712_DOMAIN_CHAIN_ID = 1;

This comment has been minimized.

Copy link
@bingen

bingen May 21, 2019

Member

Maybe this could be passed in the constructor, or have a setter, to be able to be used in test chains?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 22, 2019

Author Contributor

I didn't want to do that to avoid loading from storage these values every time we need to verify a signature. WDYT?

uint256 internal monthlyRefundQuota;

// Mapping that indicates whether a given address is allowed to use the relay service
mapping (address => bool) internal allowedSenders;

This comment has been minimized.

Copy link
@bingen

bingen May 21, 2019

Member

Having to store all possible senders in this mapping looks like too much overhead, specially for big organizations, which probably already have a list of members, or a group of authorized senders.
I guess this should be an ACL, but I wonder if Forwarding mechanism (in order to use Token Manager, which seems a likely option) would work with this setup.
Another option (I don't really like the use of Token Manager as a group) would be some kind of interface IGroup which implements a belongs(member, group) function, which I actually think should be part of the ACL, but this looks way out of the scope of this PR.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 22, 2019

Author Contributor

Initially we did, but given that using the ACL increases gas costs in ~15k, we decided to use it but in a more "async" way. As you can see, we are using it to edit the whitelist of allowed senders, but avoiding it when we need to check if someone is in included in the list.

This comment has been minimized.

Copy link
@bingen

bingen May 22, 2019

Member

Ok, then I think this works for services, as I don't expect having so many, but imagine Aragon wants to subsidize some voting (or whatever) to all token holders. According to etherscan, that would be like ~21k holders, i.e., allowed senders. Adding them to that mapping would be quite an amount of gas (and there will probably be bigger orgs with this problem), besides we should monitor ANT transactions to edit allowances.
If we used such an IGroup interface, we could have Token Manager implementing it, or if we don't want to update it, just have a wrapper small contract implementing which would get the balance and return true if > 0.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 23, 2019

Author Contributor

Yea, having groups would be amazing :)

mapping (address => bool) internal allowedSenders;

// Mapping that indicates whether a given address is allowed as off-chain service to relay transactions
mapping (address => bool) internal allowedServices;

This comment has been minimized.

Copy link
@bingen

bingen May 21, 2019

Member

Could this be an ACL role too?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 22, 2019

Author Contributor

Explained above

Show resolved Hide resolved contracts/relayer/Relayer.sol Outdated
Show resolved Hide resolved contracts/relayer/Relayer.sol
returns (bool)
{
bytes32 messageHash = _messageHash(to, nonce, data, gasRefund, gasPrice);
address signer = messageHash.recover(signature);

This comment has been minimized.

Copy link
@bingen

bingen May 21, 2019

Member

Isn't toEthSignedMessageHash needed here?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 22, 2019

Author Contributor

No, that's the idea of supporting EIP 712 😉

This comment has been minimized.

Copy link
@bingen

bingen May 22, 2019

Member

I thought it may be related to it, but didn't read all the details of that EIP, and I was confused by the fact that toEthSignedMessageHash is still in ECDSA lib. Do we need it there then?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 22, 2019

Author Contributor

Not really, it is part of the library I picked from OpenZeppelin

@facuspagnuolo facuspagnuolo force-pushed the meta-txs branch from 43f1bbb to a439ac5 May 23, 2019

@sohkai sohkai changed the base branch from dev to next Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.