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

Feat/external txs #328

Merged
merged 24 commits into from Aug 4, 2019
Merged
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9fac7f5
Merge pull request #1 from aragon/master
Schwartz10 Jun 19, 2019
5797668
route external tx intents through tx observable
Schwartz10 Jun 23, 2019
0601c36
support uninstalled app tx intents
Schwartz10 Jul 15, 2019
615ab98
move logic to wrapper instance
Schwartz10 Jul 15, 2019
f8c5aa0
cleanup
Schwartz10 Jul 15, 2019
6f8f4eb
add tests
Schwartz10 Jul 15, 2019
0a00b93
fix lint errors
Schwartz10 Jul 15, 2019
d1da169
Merge branch 'master' of github.com:aragon/aragon.js into feat/extern…
Schwartz10 Jul 15, 2019
52b77be
review fixes
Schwartz10 Jul 26, 2019
df3419c
fix merge conflicts
Schwartz10 Jul 26, 2019
5c8124b
fix lint errors
Schwartz10 Jul 29, 2019
e403ba6
Cosmetic comment and documentation clarifications
sohkai Jul 31, 2019
eec0459
API: split external tests for events and calls
sohkai Jul 31, 2019
307f85a
Wrapper: reorder transaction pathing test
sohkai Jul 31, 2019
0efdaa4
Wrapper: add createDirectTransactionForApp helper and refactor getExt…
sohkai Jul 31, 2019
cea6eb5
Wrapper: rewrite external handler test via mocks instead
sohkai Aug 1, 2019
602cba5
Cosmetic: update ABI-spec link, and change method ABI naming to be 'd…
sohkai Aug 2, 2019
31a6236
Wrapper: fix describing external transaction path to non-installed app
sohkai Aug 2, 2019
310a6d1
Wrapper: add tests for external transaction pathing
sohkai Aug 2, 2019
992aa47
Wrapper: re-order external handler creation to be alphabetical
sohkai Aug 4, 2019
3c4a7c3
Wrapper: cosmetic comment clarifications
sohkai Aug 4, 2019
57b83b1
Wrapper: use options param for specifying external transactions with …
sohkai Aug 4, 2019
2b411d7
Wrapper: expect external option to be specified in external intent tests
sohkai Aug 4, 2019
d3ccb9a
Wrapper: export externalIntent as intent for consistency
sohkai Aug 4, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -168,18 +168,15 @@ Returns **[Observable](https://rxjs-dev.firebaseapp.com/api/index/class/Observab

Creates a handle to interact with an external contract (i.e. a contract that is **not** your app's smart contract, such as a token).

> **Note**<br>
> Sending transactions to these external contracts is not yet supported as additional security and disclosure enhancements are required in frontend clients (this is a large attack vector for malicious applications to invoke dangerous functionality).
#### Parameters

- `address` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)**: The address of the external contract
- `address` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)**: The proxy address of the external contract
This conversation was marked as resolved by Schwartz10

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 24, 2019

Member

Note that this is not necessarily true, especially for external (which may be anything like tokens or ENS or etc.). They do not have to be proxy contracts.

- `jsonInterface` **[Array](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array)&lt;[Object](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object)>**: The [JSON interface](https://web3js.readthedocs.io/en/1.0/glossary.html#glossary-json-interface) of the external contract

#### Examples

```javascript
const token = api.external(tokenAddress, tokenJsonInterface)
const token = api.external(tokenProxyAddress, tokenJsonInterface)
// Retrieve the symbol of the token
token.symbol().subscribe(symbol => console.log(`The token symbol is ${symbol}`))
@@ -208,7 +208,7 @@ export class AppProxy {
* Creates a handle to interact with an external contract
* (i.e. a contract that is **not** your app's smart contract, such as a token).
*
* @param {string} address The address of the external contract
* @param {string} address The proxy address of the external contract
* @param {Array<Object>} jsonInterface The [JSON interface](https://web3js.readthedocs.io/en/1.0/glossary.html#glossary-json-interface) of the external contract.
* @return {Object} An external smart contract handle. Calling any function on this object will send a call to the smart contract and return a single-emission Observable that emits the value of the call.
*/
@@ -265,6 +265,21 @@ export class AppProxy {
}
})

// bind contract non-call methods for external intents
const externalIntentMethods = jsonInterface.filter(
(item) => item.type === 'function' && !item.constant
)
externalIntentMethods.forEach((methodJsonInterface) => {
contract[methodJsonInterface.name] = (...params) => {
return this.rpc.sendAndObserveResponse(
'external_intent',
[address, methodJsonInterface, ...params]
).pipe(
pluck('result')
)
}
})

return contract
}

@@ -180,6 +180,39 @@ test('should return an handle for an external contract events', t => {
})
})

test('should return a handle for creating external transaction intents', t => {
t.plan(3)
// arrange
const externalFn = Index.AppProxy.prototype.external
const observableIntent = of({
id: 'uuid4',
result: 10
})

const jsonInterfaceStub = [
{ type: 'function', name: 'add', constant: false }
]

const instanceStub = {
rpc: {
// Mimic behaviour of @aragon/rpc-messenger
sendAndObserveResponse: createDeferredStub(observableIntent)
}
}

// act
const result = externalFn.call(instanceStub, '0xextContract', jsonInterfaceStub)

// assert
const hadAddFn = Object.keys(result).indexOf('add') > -1
t.is(hadAddFn, true)

result.add().subscribe(value => {
t.is(instanceStub.rpc.sendAndObserveResponse.getCall(0).args[0], 'external_intent')
t.is(value, 10)
})
})

test('should return the state from cache', t => {
t.plan(3)
// arrange
@@ -1159,6 +1159,7 @@ export default class Aragon {
handlers.createRequestHandler(request$, 'network', handlers.network),
handlers.createRequestHandler(request$, 'notification', handlers.notifications),
handlers.createRequestHandler(request$, 'external_call', handlers.externalCall),
handlers.createRequestHandler(request$, 'external_intent', handlers.externalIntent),
handlers.createRequestHandler(request$, 'external_events', handlers.externalEvents),
handlers.createRequestHandler(request$, 'external_past_events', handlers.externalPastEvents),
handlers.createRequestHandler(request$, 'identify', handlers.appIdentifier),
@@ -1319,6 +1320,42 @@ export default class Aragon {
return []
}

/**
* Calculate the transaction path for a transaction to `destination`
* that invokes `methodName` with `params`.
*
* @param {string} destination An address of an uninstalled smart contract application
* @param {string} method ABI of smart contract method
* @param {Array<*>} params
* @return {Promise<Array<Object>>} An array of a single Ethereum direct transaction
*/
async getUninstalledAppTransactionPath (destination, method, params) {

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 24, 2019

Member

Instead of having two functions, we could probably just combine this with getTransactionPath() and handle it as a case where the destination is not an installed app.

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Jul 26, 2019

Author Contributor

Hey @sohkai - originally I tried to approach external tx pathing this way, but what's the best strategy to handle the ABI of the external method without breaking things?

One approach is to include an optional methodAbi param:

 async getTransactionPath (destination, methodName, params, finalForwarder, methodAbi = []) { ... }

Another way that feels prettier, but also like a much bigger change, is to adjust the getTransactionPath method to take method (includes the abi) rather than just the methodName, and then grab the name from the method object. This is pretty much exactly what getUninstalledAppTransactionPath does: method is used as abi & using method.name as methodName

Although more concise, this strategy feels like its changes could propagate to other parts of aragon.js and potentially break things.

Am i missing something? or is there another strategy you like better that I didn't think of?

const accounts = await this.getAccounts()

for (let account of accounts) {
const mockApp = {
proxyAddress: destination,
abi: [method]
}

const directTransaction = await createDirectTransaction(
account,
mockApp,
method.name,
params,
this.web3
)

if (directTransaction) {
try {
return this.describeTransactionPath([directTransaction])
} catch (_) { }
}
}

return []
}

/**
* Calculate the transaction path for a basket of intents.
* Expects the `intentBasket` to be an array of tuples holding the following:
@@ -16,6 +16,35 @@ export function call (request, proxy, wrapper) {
return contract.methods[method.name](...params).call()
}

export async function externalIntent (request, proxy, wrapper) {
const [
proxyAddress,
method,
...params
] = request.params

const installedApp = await wrapper.getApp(proxyAddress)
const transactionPath = installedApp
? await wrapper.getTransactionPath(
proxyAddress,
method.name,
params
)
: await wrapper.getUninstalledAppTransactionPath(
proxyAddress,
method,
params
)

return wrapper.performTransactionPath(
transactionPath.map((tx) => ({
...tx,
external: true,
installedApp: !!installedApp
This conversation was marked as resolved by Schwartz10

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 24, 2019

Member

As mentioned in aragon/aragon#850 (comment), we don't really need this, since we can check in the client if the to is an installed app or not.

This comment has been minimized.

Copy link
@Schwartz10

Schwartz10 Jul 25, 2019

Author Contributor

Are you talking just about the installedApp var or external too? It seems apps would need a way to determine their own proxy address to determine external, but installedApp could definitely be calculated easily.

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 25, 2019

Member

Just installedApp. external is a little bit harder to get because it requires some context about the currently running app (which the client does still have, but involves a bit more state plumbing).

}))
)
}

export function events (request, proxy, wrapper) {
const web3 = wrapper.web3
const [
@@ -1,13 +1,69 @@
import test from 'ava'
import sinon from 'sinon'
import { EventEmitter } from 'events'
import Web3 from 'web3'

import { events } from './external'
import { events, externalIntent } from './external'

test.afterEach.always(() => {
sinon.restore()
})

test.beforeEach(t => {
const web3 = new Web3()
const { keccak256, padLeft } = web3.utils

const externalContractAddress = '0xd6ae8250b8348c94847280928c79fb3b63ca453e'
const permissionsCreator = '0x7ffC57839B00206D1ad20c69A1981b489f772031'
const app = { proxyAddress: externalContractAddress }
const jsonInterfaceStub = [
{
type: 'function',
name: 'add',
constant: false,
inputs: [{ name: 'number', type: 'uint8' }]
}
]
const txOpts = {
gasPrice: 1,
description: 'custom description'
}

const wrapper = {
web3,
applyTransactionGas: (tx) => Promise.resolve(tx),
performTransactionPath: sinon.stub(),
getAccounts: () => Promise.resolve([permissionsCreator]),
getTransactionPath: () => Promise.resolve([txObject]),
getUninstalledAppTransactionPath: () => Promise.resolve([txObject]),
getApp: (appAddress) => new Promise(resolve => {
if (appAddress === app.proxyAddress) resolve(app)
else resolve(null)
})
}

const functionSig = keccak256('add(uint8)').slice(0, 10)

const txObject = {
...txOpts,
to: externalContractAddress,
from: permissionsCreator,
data: `${functionSig}${padLeft(permissionsCreator.slice(2), 64).toLowerCase()}`
}

t.context = {
app,
externalContractAddress,
functionSig,
jsonInterfaceStub,
permissionsCreator,
txOpts,
txObject,
wrapper,
web3
}
})

test('should return an observable from the contract events', async (t) => {
t.plan(1)
// arrange
@@ -34,3 +90,65 @@ test('should return an observable from the contract events', async (t) => {

eventEmitter.emit('data', { event: 'pay_fee', amount: 5 })
})

test('should return the correct tx path from external tx intent', async t => {
t.plan(2)
// arrange
const {
externalContractAddress,
functionSig,
jsonInterfaceStub,
permissionsCreator,
txOpts,
wrapper,
web3
} = t.context

const requestFromExternalInstalledApp = {
jsonrpc: '2.0',
id: 'uuid4',
method: 'external_intent',
params: [
externalContractAddress,
jsonInterfaceStub,
permissionsCreator,
txOpts
]
}

// request from external, non-aragon app
const requestFromExternalNonInstalledApp = {
jsonrpc: '2.0',
id: 'uuid4',
method: 'external_intent',
params: [
'0x000000',
jsonInterfaceStub,
permissionsCreator,
txOpts
]
}
// act
await externalIntent(requestFromExternalInstalledApp, null, wrapper)
await externalIntent(requestFromExternalNonInstalledApp, null, wrapper)

const expected = {
...txOpts,
to: externalContractAddress,
from: permissionsCreator,
data: `${functionSig}${web3.utils.padLeft(permissionsCreator.slice(2), 64).toLowerCase()}`
}

// assert
t.deepEqual(wrapper.performTransactionPath.getCall(0).args[0], [{
...expected,
external: true,
installedApp: true
}])

t.deepEqual(wrapper.performTransactionPath.getCall(1).args[0], [{
...expected,
external: true,
installedApp: false
}])
})
@@ -54,6 +54,7 @@ export { default as describeScript } from './describe-script'
export { call as externalCall } from './external'
export { events as externalEvents } from './external'
export { pastEvents as externalPastEvents } from './external'
export { externalIntent } from './external'
export { default as events } from './events'
export { default as pastEvents } from './past-events'
export { default as intent } from './intent'
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.