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

Core: Registry proposal #214

Closed
wants to merge 2 commits into from
Closed

Core: Registry proposal #214

wants to merge 2 commits into from

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Nov 9, 2020

I've created quickly a draft implementation of the solution I thought could work for the new GovernRegistry. Would be great to get some feedback here and to start the discussion about.

closes #196

@nivida nivida added P3 🏖 'It would be great to do this in the future' issues T0: Core 🌞 Core issues S3: Maintenance 👨‍💼 Make it shine labels Nov 9, 2020
@CLAassistant
Copy link

CLAassistant commented Nov 9, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current mental model of Govern is a bit unclear with the current changes. I think we still need to give more definition to what we consider it is a DAO in the context of Govern.

From the current changes it's clear that:

  1. A DAO will be the owner of several registration entities identified by a unique name.
  2. Those entities are the links between an executor and a queue.
  3. A DAO has metadata and an owner

It's unclear to me:

  1. What's the difference between a DAO and an executor instance.
  2. What's the relation between register a pair (executor, queue) and assign it an owner.
  3. What happened if a pair has no owner.

@@ -36,4 +35,11 @@ contract GovernRegistry is IERC3000Registry {
function _setMetadata(IERC3000Executor _executor, bytes memory _metadata) internal {
emit SetMetadata(_executor, _metadata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe now the metadata should be related to the owner address instead of the executor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, agreed. I thought already before it is strange that anyone can change the metadata of a registered Executor.

@nivida
Copy link
Contributor Author

nivida commented Nov 10, 2020

My current mental model of Govern is a bit unclear with the current changes. I think we still need to give more definition to what we consider it is a DAO in the context of Govern.

From the current changes it's clear that:

  1. A DAO will be the owner of several registration entities identified by a unique name.
  2. Those entities are the links between an executor and a queue.
  3. A DAO has metadata and an owner

It's unclear to me:

  1. What's the difference between a DAO and an executor instance.
  2. What's the relation between register a pair (executor, queue) and assign it an owner.
  3. What happened if a pair has no owner.

Some really good points! Do we want to have a call with @izqui to discuss them first before we focus more on the code?

@nivida nivida added the Blocked/Frozen Label to mark a PR as blocked or frozen label Nov 11, 2020
@nivida nivida mentioned this pull request Nov 13, 2020
6 tasks
@izqui
Copy link
Contributor

izqui commented Dec 16, 2020

@nivida what's the status of this?

@nivida
Copy link
Contributor Author

nivida commented Jan 27, 2021

@izqui We decided back then to organize a call with you to discuss it closer. I will introduce the new devs to Govern and will definitely come back to you in case we have a call about it or any other kind of communication we would like to hear your feedback. Thanks for asking back!

@nivida nivida closed this Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked/Frozen Label to mark a PR as blocked or frozen P3 🏖 'It would be great to do this in the future' issues S3: Maintenance 👨‍💼 Make it shine T0: Core 🌞 Core issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core: Registry improvement
4 participants