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(contracts): Add WalletScheme contract and tests #758

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(contracts): Add WalletScheme contract and tests #758

wants to merge 1 commit into from

Conversation

AugustoL
Copy link

@AugustoL AugustoL commented Jun 1, 2020

Adds the wallet scheme contract, it is based in the genericScheme but it doesnt make the genericCall
to the controller, instead it can execute multiple staticCalls to any contract. (It does not allow
execute staticCalls on the scheme itself and controller).

I know that this "breaks" the current design of the DAOstack insfrastructure, although we (DXdao devs) would like to go in this direction by replacing the avatar role into one of this schemes, have different layers of security (wallets with ++ funds, higher requirements to execute proposals).

Interested in your review @orenyodfat and @leviadam .

Adds the wallet scheme contract, it is based in the genericScheme but it doesnt make the genericCall
to the controller, instead it can execte multiple staticCalls to any contract. (It does not allow
execute staticCalls on the scheme itself and controller).
@orenyodfat
Copy link
Contributor

@AugustoL could you please explain more about the motivation behind this scheme .
In relation to security concerns :

  • This scheme has no constraint re the target contract it sends the funds to(which are part of the proposal) so in a sense you are getting more general porpose scheme though you are paying with more vulnerability.

@AugustoL
Copy link
Author

AugustoL commented Jun 1, 2020

@AugustoL could you please explain more about the motivation behind this scheme .
In relation to security concerns :

  • This scheme has no constraint re the target contract it sends the funds to(which are part of the proposal) so in a sense you are getting more general porpose scheme though you are paying with more vulnerability.

Hola @orenyodfat, effectively. The idea of the WalletScheme is to have the flexibility to make static calls directly to any contract.
The idea is to have multiple wallets with different level of security (weekly withdrawal limit, minimum execution time, required reputation, etc).

By not calling the controller we wont be able to use the global constraints, right? maybe we would have to add this configuration in the wallet scheme itself, right?

@orenyodfat
Copy link
Contributor

orenyodfat commented Jun 1, 2020

@AugustoL could you please explain more about the motivation behind this scheme .
In relation to security concerns :

  • This scheme has no constraint re the target contract it sends the funds to(which are part of the proposal) so in a sense you are getting more general porpose scheme though you are paying with more vulnerability.

Hola @orenyodfat, effectively. The idea of the WalletScheme is to have the flexibility to make static calls directly to any contract.
The idea is to have multiple wallets with different level of security (weekly withdrawal limit, minimum execution time, required reputation, etc).

By not calling the controller we wont be able to use the global constraints, right? maybe we would have to add this configuration in the wallet scheme itself, right?

Yes. no global constraint on this scheme.

Few options for that :

  1. multiple standard genericSchemes. each for each wallet.
  2. use ContributionReward proposals to send funds from the avatar to each wallet.
  3. have a generic scheme with white listed target contracts.

Please note that option 3 is new (same as the one you implemented) and will require support on the higher layers (migration/subgraph/arc.js/alchemy).
option 1&2 are already supported by alchemy .

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

Successfully merging this pull request may close these issues.

2 participants