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

alchemy should prevent setting wrong permission for scheme registrar #929

Closed
orenyodfat opened this issue Jul 17, 2019 · 9 comments
Closed
Assignees

Comments

@orenyodfat
Copy link
Contributor

scheme registrar permission should be >=2 .
this in order to prevent issues like #918 .

@jellegerbrandy
Copy link
Contributor

We have a form for adding schemes and for ediging schems, that are giving the user the liberty to set the permissions as they want.

To implement these kind of restrictions in alchemy, we'd need sort of like a registry of schems with default permissions, either set them automaticlly (remove it from the form?) or warn the user? And what do we do with schemes that we do not know? And what about errors in the parameteers. Should we also forbid, say, proposal times that are longer then 10 years? Etc. etc. (Not trying to be polemic, jus tsaying you opened a big can of worms :-))

But in any case, thie schemeregistrar from issue #918 was probably not added in alchemy in the frist place, but with a script. So implementing these checks in alchemy would not have avoided the error anyway.

So given that (a) this is a whole new rabbit hole for implementation in alchemy and (b) putting the checks in alchemy instead of on the contract level will only avoid the problem in some cases, but not all, it looks to me that tis should be resolved on the contract level (if it needs to be resolved at all)

@orenyodfat
Copy link
Contributor Author

orenyodfat commented Jul 17, 2019

But in any case, thie schemeregistrar from issue #918 was probably not added in alchemy in the frist place, but with a script. So implementing these checks in alchemy would not have avoided the error anyway.

does this proposal does not came from alchemy ?

sure , some of the issues can be solved at the contract level ...see daostack/arc#647

adding additional checks in alchemy which prevent the standard user for doing trivial mistakes (like downgrade scheme registrar permission) might be nice to have.

@jellegerbrandy
Copy link
Contributor

does this proposal does not came from alchemy ?

Ok I misunderstood what was the issue here, I though tthe DAO was created with the wrong permissions on the SchemeRegistrar.

Unless we want to work on an ad hoc basis ("user does something stupid, we implement a warning or make it impossible in alcemy"), we'd need a list of "trivial mistakes" and some policy for handling them. Perhaps we should have a call about this? Also bc it is not clear to me how to handle this. What if a DAO wants to do something stupid?.

@jellegerbrandy
Copy link
Contributor

@A-Zak A-Zak closed this as completed Aug 19, 2019
@A-Zak A-Zak reopened this Aug 19, 2019
@tibetsprague
Copy link
Contributor

so I'm unclear on the specific things we want to do here. The only concrete task i see is not allowing people to downgrade the permissions on the Scheme Registrar/Plugin manager.

Secondly, are we saying that when we are adding a new plugin/scheme we look up that scheme and try to determine what it is and then have default permissions for that? how would we do that?

@dkent600
Copy link
Contributor

dkent600 commented Mar 3, 2020

@tibetsprague I think @orenyodfat is asking us to not allow a set of permissions to be submitted that leaves a scheme unusable. In other words, certain permissions are required, depending on the scheme. The link that @jellegerbrandy provided above shows how arc.js used to define the required ("minimum") permissions for each scheme.

@dkent600
Copy link
Contributor

dkent600 commented Mar 3, 2020

@tibetsprague As for:

Secondly, are we saying that when we are adding a new plugin/scheme we look up that scheme and try to determine what it is and then have default permissions for that? how would we do that?

This seems reasonable to me, and we can already identify Arc schemes that are not fully supported by Alchemy by ISchemeState.name. I think non-Arc schemes may be kinda screwed until we have a better story for required scheme interfaces, or a different story for plugging schemes into Alchemy.

@tibetsprague
Copy link
Contributor

@orenyodfat do you have an updated list of which schemes require which minimum permissions? Also is the list of minimum requirements the same as what we should default the permissions to? For example when registering a SchemeRegistrar scheme obviously it needs permission 2 to register schemes so we should just always include that one. are there others that we should include as by default selected but let people unselect them? In the old Arc.js code that @dkent600 mentioned it defaults to selecting all permissions for SchemeRegistrar

@dkent600
Copy link
Contributor

done

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

No branches or pull requests

5 participants