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

Enforce Module permissions modifications to be routed through supply keeper #4762

Closed
4 tasks
colin-axner opened this issue Jul 22, 2019 · 7 comments
Closed
4 tasks
Assignees
Milestone

Comments

@colin-axner
Copy link
Contributor

Summary

Add functions to supply keeper such as AddPermission and RemovePermission.

Problem Definition

Currently there is little structure around how adding/removing permissions for a module account. A module account has AddPermission and RemovePermission functions that aren't exported. The only way we can do contextual validation on the permissions called in AddPermission is to use the ValidatePermissions function in supply keeper.

Proposal

Add AddPermission and RemovePermission to supply keeper that calls ValidatePermissions before calling the account's add/remove permission functions


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez alexanderbez added this to the v0.37.0 milestone Jul 22, 2019
@fedekunze
Copy link
Collaborator

fedekunze commented Jul 23, 2019

I believe @rigelrozanski had a very compelling argument to only grant permissions at initialization. If we support adding permissions what prevents for eg a module account to add the Minter Permission and start minting coins ?

@colin-axner
Copy link
Contributor Author

So in this case all possible permissions are declared at initialization, ie a module account cannot be granted Minter Permission unless it is added as a possible permission initially.

Perhaps one way of making this stricter is unidirectional permission modifications. Give module accounts the full permissions declared at initialization and only allow for removal of those permissions and no additions. One example of usage of this is in coinswap if governance decided to disable a trading pair, the subaccounts minting permission could be removed.

@alexanderbez
Copy link
Contributor

I believe @rigelrozanski had a very compelling argument to only grant permissions at initialization. If we support adding permissions what prevents for eg a module account to add the Minter Permission and start minting coins ?

It would have to go through governance. There may very well be situations where we want to grant permissions to existing module accounts or remove them (although I haven't thought of any).

@rigelrozanski
Copy link
Contributor

YO

so this is what it is - a module can be granted the permission to modify permissions. Note that this isn't referring to a module-account permission, but a new class of permission for a module to make these modifications - it would ideally just live in supply and never get explicitly passed to other modules but if it did this would be defined at application initialization.

In our situation I reckon it would look like the supply module is passes this special new permission to a Proposal Type which is registered with governance at app initialization.

@colin-axner
Copy link
Contributor Author

Sidenote: When implementing #4679, I had a misunderstanding of how permissions were managed. AddPermission and RemovePermission can be removed from the code, since they do not actually modify permissions tracked by the Supply Keeper. They just modify the ModuleAccount instance being referenced.

@rigelrozanski
Copy link
Contributor

Open up a PR?

@alexanderbez
Copy link
Contributor

Closed by #4798

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

4 participants