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

Conversation

@Schwartz10
Copy link
Contributor

commented Jun 25, 2019

Changes

If you are modifying the external API of one of the modules, please remember to also change the documentation

  • I have updated the associated documentation with my changes

Fixes #120

Merge pull request #1 from aragon/master
merge master of aragon/aragon.js

@auto-assign auto-assign bot requested review from 2color and sohkai Jun 25, 2019

@Schwartz10 Schwartz10 referenced this pull request Jun 25, 2019
@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Hey! You'll notice I added a getApps method that the front-end clients can use to get the proxy address of external apps. This might be redundant with #327 so please let me know if I should remove. Thanks!

@topocount

This comment has been minimized.

Copy link

commented Jul 1, 2019

It'd be awesome if we could interact with contracts that aren't registered as a DAO's app contracts. The primary use case for this right now is interacting with StandardBounties.sol, Users of the current Rinkeby Projects app deployment need to relay their calls through the projects contract when submitting work to fulfill bounties. This adds what would otherwise be unnecessary expense, and isn't the cleanest way to go about this.

@Schwartz10 Schwartz10 referenced this pull request Jul 3, 2019
1 of 1 task complete

@Schwartz10 Schwartz10 force-pushed the openworklabs:feat/external-txs branch from 889fd43 to 5797668 Jul 3, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

It'd be awesome if we could interact with contracts that aren't registered as a DAO's app contracts.

@topocount let me know if this is not working for you, and if so, what the error message is.

@sohkai - removed the getApps functionality and put that in #332 FYI

@sohkai

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

It'd be awesome if we could interact with contracts that aren't registered as a DAO's app contracts.

@topocount Agreed, @Schwartz10 and I discussed this yesterday about using the transaction pathing only if the transaction is to an installed app. Otherwise it'll be treated as a "direct" transaction.

@sohkai sohkai changed the title Feat/external txs [WIP] Feat/external txs Jul 3, 2019

@sohkai sohkai referenced this pull request Jul 3, 2019

@Schwartz10 Schwartz10 changed the title [WIP] Feat/external txs Feat/external txs Jul 15, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@sohkai @2color ready for your review!

One thing to note is that the transaction path contains a transaction object that now has 2 additional fields: installedApp and external

installedApp && external means an external intent on another application contract within your DAO
!installedApp && external means an external intent on a completely isolated contract (like @topocount use case with standard bounties)

Figured it's probably useful to give this information in the client, but I couldn't figure out a better way to display that information. This is what the tests I added are essentially checking for

@sohkai
Copy link
Member

left a comment

🙇 Sorry for taking a while to look at these!

This looks generally good, but I have a couple suggestions to make it a bit more succinct :). I can also make the changes in the next few days if you'd like!

docs/API.md Outdated Show resolved Hide resolved
packages/aragon-wrapper/src/rpc/handlers/external.js Outdated Show resolved Hide resolved
* @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?

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Thanks for the feedback @sohkai ! My goal is to wrap this up tomorrow (Friday).

Schwartz10 added some commits Jul 26, 2019

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@sohkai i'm confident these changes will work, but i want to test them in some real code on monday. Feel free to review whenever is convenient for you, and I'll leave a comment here once I've tested thoroughly

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@sohkai this is all set - tested with both installed, external contracts, and uninstall external contracts.

sohkai added some commits Jul 31, 2019

sohkai added some commits Jul 31, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

@Schwartz10 I've just added a few commits with mostly clarifications / small cleanups I saw. The big important one is 0efdaa4, where I've restructured a few of the internal plumbing. The only external API change is how external is propagated; now it's on the top-level object emitted by the transactions observable, rather than part of each transaction.

We still need to rework the external tests a bit. Here's what I'm thinking:

  • In rpc/handlers/external.test.js, use a mocked wrapper and we just stub and spy that the right functions are being called with the right arguments
  • In index.test.js, test out the flow, using most of the parts of your current test in rpc/handlers/external.test.js

It would be great if you could review and retest these changes!

sohkai added some commits Aug 1, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@Schwartz10 I've pushed the final changes for the tests, including a bug fix in 31a6236 on my previous pushes.

It also changes the language around the "JSON interface". web3.js likes to use jsonInterface in its argument names but it's quite unclear what it means (the ABI array format, or the specific method / event object description in the array?), so I've fallen back to using JsonDescription for objects since that's what the ABI-spec uses.

It would be greatly appreciated if you could re-test, and we can get this merged!

@Schwartz10

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@sohkai just re-tested with external installed, and external uninstalled. Both working properly. Let's pull this in asap! Once that's all set, I'll work on finishing up aragon/aragon#850

Schwartz10 added a commit to openworklabs/aragon that referenced this pull request Aug 2, 2019

sohkai added some commits Aug 4, 2019

@sohkai sohkai force-pushed the openworklabs:feat/external-txs branch from 4cba397 to d3ccb9a Aug 4, 2019

@sohkai sohkai merged commit c08bad3 into aragon:master Aug 4, 2019

4 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 51.84%
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.