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 2) #527

Draft
wants to merge 4 commits into
base: meta-txs
Choose a base branch
from

Conversation

facuspagnuolo
Copy link
Contributor

@facuspagnuolo facuspagnuolo commented May 17, 2019

Following up #526

This PR aims to provide the required JS libraries to implement an meta-transactions server that will act as the off-chain part of the relayer for the model described in the previous PR.

We will probably want to move this logic outside of aragonOS. Once we have the relayer server repo, I think it will fit better there.

@coveralls
Copy link

coveralls commented May 17, 2019

Coverage Status

Coverage remained the same at 99.556% when pulling 4344738 on meta-txs-service into a439ac5 on meta-txs.

@facuspagnuolo facuspagnuolo self-assigned this May 20, 2019
@@ -37,7 +37,7 @@
"eth-ens-namehash": "^2.0.8",
"eth-gas-reporter": "^0.1.1",
"ethereumjs-abi": "^0.6.5",
"ganache-cli": "^6.4.2",
"ganache-cli": "~6.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, was the latest version failing due to the gas estimation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I was facing this issue


async _fetchMainnetGasPrice() {
try {
const axios = require('axios')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well lift this up as a top-level import

],
}

module.exports = web3 => class RelayTransactionSigner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about either placing the web3 instance inside these classes, or extracting the web3 instance used into its own util (e.g. getWeb3()).

Generally we don't run services in the context of a truffle environment (maybe we should? I'm a bit scared at needing to depend on them for the service to stay alive), so there's no global web3 instance available.


It'd also avoid defining a class in a function, which is a bit awkward, since you can have different definitions if you invoke the function more than once. This then starts to violate the instanceof checks and prototype chains, and gets very awkward if you ever need to compare / manipulate more than one object derived from what should be same class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree, it should be part of the constructor o at least a known object that resolves that dependency

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

Successfully merging this pull request may close these issues.

3 participants