videoDAC / livepeer-aragon

Aragon App for interacting with the Livepeer Protocol. Includes Delegator functionality and basic Transcoder functionality.
https://rinkeby.aragon.org/#/livepeerdelegator.aragonid.eth/0x4a7335f3ecb43b685526c1b39043bf696c78c641
8 stars 2 forks source link

Use Agent app instead of inheriting from it #22

Closed izqui closed 5 years ago

izqui commented 5 years ago

Would it be possible for LivepeerAragonApp to have a reference to an Agent instance that it uses for actually executing the actions instead of inheriting from a modified Agent contract? The LivepeerAragonApp could have permissions in an Agent instance to execute actions and run scripts, which shouldn't require a lot of changes in the code. This would be similar to how Finance has a reference to a Vault instance that it uses to hold the funds.

As it is currently implemented, I don't see a lot of benefit on inheriting from Agent instead of performing the calls directly. On the other hand, if it is the Agent app the one that is executing the calls, users could interact with the Agent directly (using the upcoming Frame integration) to do actions that the LivepeerAragonApp contract doesn't implement.

chrishobcroft commented 5 years ago

Thank you for this feedback @izqui - I will let @willjgriff respond specifically with regards the architecture and implementation.

I'm interested to know what impact the different approaches have for:

a) the ongoing development and maintenance of the Livepeer Aragon App

b) end-users of the Livepeer Aragon App - in terms of things like e.g. gas costs, usability, and security

Thanks

willjgriff commented 5 years ago

Hi @izqui, sorry for the slow reply, I’ve been away, but thanks for highlighting this. I think you’re absolutely right, I’m not sure how I overlooked this approach as I’m aware of the advantages of composition over inheritance. Naturally I would much rather not modify any of the Aragon apps that are used. I guess I was in a one contract per app mindset and was still familiarising myself with Aragon permissions. I will remodel it over the next few days.

One thought though, is it expected there will only be one Agent per DAO which is used by many apps? Because if this is the case then the only concern I have would be for whoever manages the permissions for the Agent managing them carefully since they must be set with params to prevent apps from having more access than they should. ASFAIK there isn’t any UI for updating and viewing permissions with params for easy verification, is there likely to be in the future? Otherwise I wonder if it would be safer to install one Agent per app.

izqui commented 5 years ago

@willjgriff is it expected there will only be one Agent per DAO which is used by many apps?

It depends on the organization, but there could definitely be multiple Agent app instances installed for different purposes.

must be set with params to prevent apps from having more access than they should

Yes, although if it is another app that has permissions to execute actions in the Agent, the scope of what it can do is also restricted by the app itself. So it really comes down to who can upgrade the app, which in all default templates it is the same entity (the root authority Voting app).

ASFAIK there isn’t any UI for updating and viewing permissions with params for easy verification, is there likely to be in the future?

Indeed, there is no UI for permission parameters at the moment. It is not part of the current roadmap for Aragon One, but showing whether a permission has params or not in the Permissions UI could be done fairly easily, also parsing and displaying simple parameters shouldn't be hard.

willjgriff commented 5 years ago

if it is another app that has permissions to execute actions in the Agent, the scope of what it can do is also restricted by the app itself.

Of course, I guess I'm thinking of a situation where an insecure app is installed which shares references/dependencies with other apps, then there's risk it could effect other apps if permissions aren't set correctly.

showing whether a permission has params or not in the Permissions UI could be done fairly easily, also parsing and displaying simple parameters shouldn't be hard.

Nice, I think this would be useful.

willjgriff commented 5 years ago

if it is another app that has permissions to execute actions in the Agent, the scope of what it can do is also restricted by the app itself.

I've realised the Livepeer app uses the forward function for which the permission can't be restricted by function sigs like the execute() function can. So if we're to continue to use forward() in this way (to execute multiple transactions at once, in this case approve() and bond()) and the agent is reused, we will have to verify and trust the apps that are granted the permission to execute forward() anyway.

chrishobcroft commented 5 years ago

The proposed changes have been made to this app in the latest code - @willjgriff please can you confirm?

@izqui - are you happy for us to close the issue?

Also, I have a few pieces of feedback on this new architecture, from the perspectives of an installer of the app and an end user:

The installation process is more expensive and complex

Obviously in a world where there are multiple apps operating via the same Agent, then the additional gas cost of installing Agent can start to be shared, and the benefit of the cleaner architecture perhaps increasingly outweighs the additional complexity.

The address of the Livepeer app ≠ the address of the entity interacting on the protocol

These are all perhaps minor points, but I do think it's worth acknowledging them, as most DAOs will begin by installing only 1 app (+1 agent), and so the overheads and complexity will be felt more acutely by end users.

izqui commented 5 years ago

I think this makes sense and we can close the issue!

In my opinion the extra cost of deploying an additional app is worth it even if only for not having the Agent functionality duplicated across multiple apps. This fragmentation could be problematic in case that an issue is found in Agent and it would need to be propagated across all its 'forks'. Also it will be more and more common for organizations to already have an Agent installed (some Aragon 0.8 templates already come with Agent), so only the Livepeer app would need to be installed and granted permissions on the Agent instance.

I'm not worried about having two different addresses, as users will probably not be exposed to the Livepeer app address unless they are using the app from the CLI.

chrishobcroft commented 5 years ago

Yes, I agree, and if Agent does end up shipping with the core DAO templates, then this can simplify the incremental build process for adding individual apps.

I'm not worried about having two different addresses, as users will probably not be exposed to the Livepeer app address unless they are using the app from the CLI.

I am also not worried, but for the sake of completeness, the Livepeer app address is exposed to the user of the web app in the browser's address bar, and also in the DAO's Settings.

Closing - thanks for the discussion!