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

facuspagnuolo · 2019-05-09T19:54:07Z

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:

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

contracts/relayer/BaseRelayer.sol

contracts/relayer/RelayerAragonAppWithParameterizedSender.sol

bingen

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.

contracts/relayer/BaseRelayer.sol

contracts/relayer/RelayedAragonAppWithParameterizedSender.sol

contracts/relayer/RelayerAragonAppWithParameterizedSender.sol

contracts/relayer/RelayerAragonAppWithVolatileSender.sol

facuspagnuolo · 2019-05-14T22:49:34Z

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

facuspagnuolo · 2019-05-14T23:17:37Z

@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?

izqui · 2019-05-15T10:22:34Z

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

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 Proposal: add forwardWillExecuteImmediately() to Forwarder interface #521) as it is the way that 'membership' is managed in most organizations.

contracts/relayer/Relayer.sol

facuspagnuolo · 2019-05-16T13:15:31Z

@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 :/

bingen

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

bingen · 2019-05-21T17:39:41Z

contracts/relayer/Relayer.sol

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


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

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

bingen · 2019-05-21T17:51:54Z

contracts/relayer/Relayer.sol

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


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.

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.

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.

Yea, having groups would be amazing :)

bingen · 2019-05-21T17:52:19Z

contracts/relayer/Relayer.sol

+    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;


Could this be an ACL role too?

Explained above

contracts/relayer/Relayer.sol

bingen · 2019-05-21T20:39:28Z

contracts/relayer/Relayer.sol

+        returns (bool)
+    {
+        bytes32 messageHash = _messageHash(to, nonce, data, gasRefund, gasPrice);
+        address signer = messageHash.recover(signature);


Isn't toEthSignedMessageHash needed here?

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

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?

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

facuspagnuolo self-assigned this May 9, 2019

facuspagnuolo added the research label May 9, 2019

izqui reviewed May 10, 2019

View reviewed changes

facuspagnuolo mentioned this pull request May 10, 2019

Support meta-txs aragonone/product#87

Closed

5 tasks

facuspagnuolo added 7 commits May 10, 2019 10:45

meta-txs: measure gas ovearload for different alternatives

061b437

meta-txs: optimize nonce tracking costs

db8c7ff

meta-txs: pick relayed app using volatile storage approach

fd420ff

meta-txs: use depositable proxies functionality from relayer

87030a0

meta-txs: implement shared interface between relayer and apps

ee34afa

meta-txs: optimize off chain service authorization

3c6a500

meta-txs: use events helpers

a576413

facuspagnuolo force-pushed the meta-txs branch from 80468e6 to 6837a45 Compare May 10, 2019 17:33

meta-txs: optimize volatile sender using calldata

d83a209

facuspagnuolo force-pushed the meta-txs branch from 6837a45 to d83a209 Compare May 10, 2019 17:34

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

facuspagnuolo added the wip label May 10, 2019

facuspagnuolo marked this pull request as ready for review May 10, 2019 17:45

meta-txs: add solidity tests

ada4788

facuspagnuolo force-pushed the meta-txs branch from eab6b72 to ada4788 Compare May 13, 2019 19:33

bingen reviewed May 14, 2019

View reviewed changes

meta-txs: implement gas refunds quotas

464ee8f

meta-txs: fix linting rules

d528882

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

meta-txs: skip gas test for coverage measure

ec06318

facuspagnuolo force-pushed the meta-txs branch from 03fd3af to ec06318 Compare May 14, 2019 23:41

aragon deleted a comment from coveralls May 14, 2019

facuspagnuolo added 2 commits May 14, 2019 21:29

meta-txs: fix kernel tests

3d2b814

meta-txs: increase coverage

dcd3db7

izqui reviewed May 15, 2019

View reviewed changes

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

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

facuspagnuolo added 2 commits May 15, 2019 16:42

meta-txs: multiple enhancements

306d35c

meta-txs: implement whitelist of allowed senders

f1e30eb

meta-txs: support EIP712

f092d3b

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

meta-txs: ignore relayer contracts for coverage measure

3b91caa

facuspagnuolo force-pushed the meta-txs branch from 2759f3b to 3b91caa Compare May 17, 2019 15:49

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 mentioned this pull request May 17, 2019

Meta Txs: Adding support for meta transactions in aragon apps (Part 2) #527

Draft

facuspagnuolo removed the wip label May 17, 2019

bingen approved these changes May 21, 2019

View reviewed changes

meta-txs: store senders information using a struct

a439ac5

facuspagnuolo force-pushed the meta-txs branch from 43f1bbb to a439ac5 Compare May 23, 2019 22:33

sohkai changed the base branch from dev to next July 11, 2019 09:31

onbjerg mentioned this pull request Nov 25, 2019

Investigate if metatransactions will affect activity tracking 1Hive/apiary#99

Closed

This was referenced Mar 30, 2020

Easy Website Payments helpdao/help#3

Closed

How can volunteers cash out? helpdao/help#18

Open

gitcoinbot mentioned this pull request Apr 1, 2020

Challenge 2 - Fiat On-Ramp for Liquidity Pools bancorprotocol/contracts-solidity#345

Closed

sohkai marked this pull request as draft June 4, 2020 10:10

sohkai added this to the aragonOS 6.0 milestone Jun 4, 2020

sohkai mentioned this pull request Jul 12, 2020

[Prototype] AragonApp: signed calls and multi call execution (aka meta-txs support) #442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

facuspagnuolo commented May 9, 2019 •

edited

Loading

bingen left a comment •

edited

Loading

facuspagnuolo commented May 14, 2019

facuspagnuolo commented May 14, 2019 •

edited

Loading

izqui commented May 15, 2019

izqui left a comment

facuspagnuolo commented May 16, 2019 •

edited

Loading

bingen left a comment

bingen May 21, 2019

facuspagnuolo May 22, 2019 •

edited

Loading

bingen May 21, 2019

facuspagnuolo May 22, 2019

bingen May 22, 2019

facuspagnuolo May 23, 2019

bingen May 21, 2019

facuspagnuolo May 22, 2019

bingen May 21, 2019

facuspagnuolo May 22, 2019

bingen May 22, 2019

facuspagnuolo May 22, 2019

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

Are you sure you want to change the base?

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

Conversation

facuspagnuolo commented May 9, 2019 • edited Loading

Approach

Other approaches investigated

bingen left a comment • edited Loading

Choose a reason for hiding this comment

facuspagnuolo commented May 14, 2019

facuspagnuolo commented May 14, 2019 • edited Loading

izqui commented May 15, 2019

izqui left a comment

Choose a reason for hiding this comment

facuspagnuolo commented May 16, 2019 • edited Loading

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo May 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented May 9, 2019 •

edited

Loading

bingen left a comment •

edited

Loading

facuspagnuolo commented May 14, 2019 •

edited

Loading

facuspagnuolo commented May 16, 2019 •

edited

Loading

facuspagnuolo May 22, 2019 •

edited

Loading