Package registry PoC #131

facuspagnuolo · 2018-09-18T11:55:06Z

Fixes #132

spalladino · 2018-09-21T20:03:17Z

Looks great @facuspagnuolo! I've yet to understand better the standards around ENS to comment in-depth about the PR, but the architecture looks good. I like having a base registrar, with different "interfaces" depending on the access control for zOS or aOS (though maybe we could switch from Ownable to RBAC down the road).

I have one question pending regarding ENS: can we be sure that, under the new registrar to be deployed next year, there is no way for the domain backing the package registry to be lost? For instance, if we register thebestpackageregistry.eth, and we lose it a year after due to a higher bid, or someone forgetting to pay for the renewal, we are dealing with quite a single point of failure. From what I understand though, domain name assignments are to be permanent, but I wanted to check.

Moving on to a different subject, which is the Registry contract. I different ways of working here:

1- We have an APMRegistry and a ZOSRegistry, both entirely different contracts, but they are both backed by the same Registrar, tied to the same domain. This would allow both organizations, at first, to share the domain where packages are registered, and then move forward by integrating the subsequent contracts.

2- We aim at a single Registry contract that covers the use cases for both. We can extend the APMRegistry contract to have registerRepo methods, that will allow any kind of contract to be registered, including zOS Packages. This would remove the need for having two different contracts writing to the same Registrar, and we could actually upgrade the APMRegistry to this SharedRegistry contract.

Either way, this opens the door for arbitrary contracts to be registered in the registry, not just valid Repos or Packages. We should keep a centralised control to remove malicious registry entries during the beta phase.

Another topic is the Repo itself, which tracks different versions of a lib, and is the aOS equivalent of zOS' Package. Regardless of its validations, its heavily tied to semver, and we should decide whether we want zOS Package to adopt that naming scheme as well.

1- If we modify Package to work only under semver (which I'd not recommend to do now, but later down the road), then we could have a similar public interface to Repo, even if we don't enforce the same validations.

2- Otherwise, we accept the fact that, during the initial beta period, there will be two different kind of animals registered in the same registry, which have different interfaces.

Note that (2) means that a lib developer who wants to develop for both aOS and zOS would need to register two different packages under two different names, at least during this beta phase. On the other hand, even if we do implement a shared interface, then the expected contractAddress linked for a version of the Repo/Package would also need to be different, as it'd be an ImplementationProvider for zOS, and an AragonApp for aOS, until we figure out a common interface at that level for both OSs (which is not that far-fetched, actually).

spalladino

Looks good! Left a handful of questions. My biggest concern is having an API too large for Package, just for historical reasons (acommodating for specific aOS and zOS needs).

spalladino · 2018-09-25T14:52:36Z

package_manager/contracts/PR/zeppelin/NewZosPackage.sol

+   * @param version Name of the version.
+   * @param provider ImplementationProvider associated with the version.
+   */
+  function addVersion(uint16[3] version, ImplementationProvider provider) public onlyOwner {


I'd remove this method altogether, I think we can work fine with newVersion directly.

spalladino · 2018-09-25T14:55:52Z

package_manager/contracts/PR/zeppelin/NewZosPackage.sol

+   * @param version Name of the version.
+   * @return The implementation provider of the version.
+   */
+  function getVersion(uint16[3] version) public view returns (ImplementationProvider) {


I'd remove this method, and use getContract directly, having the client cast it to an ImplementationProvider

spalladino · 2018-09-25T14:57:09Z

package_manager/contracts/PR/RepoPackage.sol

+    return len > 0 ? len - 1 : 0;
+  }
+
+  function isValidBump(uint16[3] _oldVersion, uint16[3] _newVersion) public pure returns (bool) {


We would be removing this from the shared implementation, right?

I think so, we should discuss that

spalladino · 2018-09-25T14:59:15Z

package_manager/contracts/PR/RepoPackage.sol

+  }
+
+  function getLatest() public view returns (uint16[3], address, bytes) {
+    return getByVersionId(versions.length - 1);


I'm worried about this one. I believe we should allow for pushing a version 1.2.4, as a fix to an existing 1.2.3, even if there is a 2.0 running around. Picking up the last version in the array returns the last one uploaded, not the last one in terms of semver.

spalladino · 2018-09-25T15:00:03Z

package_manager/contracts/PR/RepoPackage.sol

+    return (version.semanticVersion, version.contractAddress, version.contentURI);
+  }
+
+  function getContractByVersionId(uint256 _versionId) public view returns (address) {


Can we remove this method, and just have clients work with getByVersionId?

Yes, I'm worried that users will need to know the version ID for a given version number, we can expose a method to tell that tho

spalladino · 2018-09-25T15:01:41Z

package_manager/contracts/PR/RepoPackage.sol

+/**
+ * @title RepoPackage
+ */
+contract RepoPackage {


As a general comment, I feel like the API is a bit convoluted. For instance, are the getByVersionId getters needed, or could we remove them? Same for getLatestForContractAddress. I know that these methods are in the original AragonPackage, can we reach out to them and check if they do need all of them?

Totally agree, the main idea of this PoC was to validate that Aragon's interface could be compatible with ours, we should now work together towards a common and better and minimum interface :)

spalladino · 2018-09-25T15:02:41Z

package_manager/contracts/PR/zeppelin/NewZosPackage.sol

+ * @dev Collection of contracts grouped into versions.
+ * Contracts with the same name can have different implementation addresses in different versions.
+ */
+contract NewZosPackage is Ownable, RepoPackage {


Should we start moving to RBAC?

Agree but I think that's not part of the PoC, do you agree?

spalladino · 2018-09-25T15:16:22Z

package_manager/contracts/PR/Registrar.sol

+    rootNode = _rootNode;
+  }
+
+  function createName(bytes32 _label, address _owner) public returns (bytes32) {


Let's keep these methods internal instead of public

spalladino · 2018-09-25T15:17:45Z

package_manager/contracts/PR/zeppelin/ZeppelinRegistrar.sol

+    Registrar.initialize(_registry, _rootNode);
+  }
+
+  function createName(bytes32 _label, address _owner) public onlyOwner returns (bytes32) {


I just noted I don't properly understand how access control to the registrar should be. Who should be the owner of the contract? How are we handling who can push new packages?

I was simlpy proving that we can reuse exactly the same implementation used by Aragon with another permission control flow. In this scenario, the owner will be the only one allowed which doesn't make much sense. IMO, we can start with something public which means so anyone can register a package for ZeppelinOS

frangio · 2018-09-25T19:13:53Z

package_manager/contracts/PR/RepoPackage.sol

+      require(isValidBump(zeroVersion, _newSemanticVersion));
+    }
+
+    uint256 versionId = versions.push(Version(_newSemanticVersion, contractAddress, _contentURI)) - 1;


I'm not sure how serious this is, but we're relying on transaction ordering to identify versions, and this might be unreliable because of reorgs.

Can we assess the severity of this and consider alternatives?

One rather efficient alternative would be the concatenation of the version components. This should be unique anyway.

Good point, I hadn't thought of that. I was actually thinking about removing versionId altogether, given that there is already the semanticVersionHash that could be used, which is similar to the concatenation you mention.

Similar but only probabilistically unique!

frangio · 2018-09-25T19:16:00Z

package_manager/contracts/PR/zeppelin/NewZosPackage.sol

+   * @param version Name of the added version.
+   * @param provider ImplementationProvider associated with the version.
+   */
+  event VersionAdded(uint16[3] version, ImplementationProvider indexed provider);


Why is it the responsibility of this contract to emit an event with the provider? Should the base RepoPackage be emitting this info along with the URI?

Agree, this should not be part of ZosPackage.

Sure, we should move that to RepoPackage. Didn't want to do that in the PoC just to make sure there was an implementation satisfying both specs :)

frangio · 2018-09-25T19:18:26Z

package_manager/contracts/PR/RepoPackage.sol

+  event NewVersion(uint256 versionId, uint16[3] semanticVersion);
+
+  struct Version {
+    uint16[3] semanticVersion;


I'd consider using three uint64. I know it sounds like a lot but I don't see a reason for using smaller numbers.

Actually using uint64 allows for alternative version identifiers, such as time-based versions (20180925 does not fit in 16 bits), so +1 to this change.

I like that, this should not be a problem for Aragon since it is an array right? I mean, we won't need to perform any migrations or so, right?

luisivan · 2018-09-26T10:10:06Z

Nice work @facuspagnuolo! I like the simplification work you have done.
I have a question: where is the Registry contract? I guess it is still on the works, but right now there would be no way to push new Repos/Packages to the registry. I see a mention of ZOSRegistry but didn't find it anywhere.

Other points I wanted to touch upon:

Rich governance

I see most of the logic has been taken from APM, but without the auth modifiers for newVersion and newRepo. I think it is important to have rich governance built in into the package manager, and that's why we made APM a DAO from the start. A decentralized package manager run by a single entity doesn't make sense, since that'd be centralized and there are already plenty of centralized package managers out there.
I think the package manager being a DAO is key for rich governance, since it makes it dead easy.
For example, in Zeppelin's case, if the package manager was a DAO, you could deploy a registry controlled by ZEP holders very easily in X steps:

Deploy the package manager DAO
Deploy the Voting app in Aragon, and give permission to the ZEP holders to cast votes
Give permission to the Voting app to call newVersion and newRepo

With just those 3 steps, we give ZEP token holders right to vote on new versions and repos inside the registry. You could get fancier and create new Token Manager instances for different groups of stakeholders, different Voting apps with different consensus thresholds for different repos, etc. This wouldn't be possible if the package manager wasn't a DAO. We recently wrote a blog post about this.

Upgradeability of the repos

Right now, the contracts for the repo and the registry are not upgradeable, right? I think they should be (no need to argue why upgradeability is important between us haha). Also one of the cool things you get for free if the package manager is a DAO is that you can upgrade all repos in a registry. Otherwise, you'd need to upgrade each one individually. But if it's a DAO, you can just upgrade the reference to the Repo app in the kernel, and all repos in a registry are automatically upgraded.

`Repo` vs `Package` naming

Although Package is more intuitive (you push new versions of a package) the technically correct term would be Repo (you push a package to a repo). I guess that calling it Package means that you push new versions to a package, but the new versions pushed packages themselves, so you cannot push a package to a package. That's why I think that, although not as sexy, Repo is the most accurate term. It's also used by package managers in Linux, etc. That being said Package feels more friendly, so I don't have hard feelings, but wanted to point out that question about naming.

That's it! Solid work on this, looking forward to your thoughts.

facuspagnuolo · 2018-09-27T09:15:03Z

Hey @luisivan! thanks a lot for your feedback! 😄

Regarding the Registry contract, it would be simple to have some similar contract to the ENSRegistry that acts as a registry for us. But that would mean that either you guys duplicate the effort of publishing Repos in this new Registry given that you're already using ENS, or that you stop using ENS to register Repos and start using the new Registry. Based on what I spoke with @izqui you guys are interested in keeping the ENS integration, so we decided to focus simply on what will be the common interface of Package/Repos that fit for both projects.

Governance

I really like the idea of decentrilizing the governance of the Package/Repos, do you think it should be part of the minimum interface?

Upgradeability of the Repos

AFAIK, given that a Aragon's Repo are an AragonApp I think those are upgradeable. Regarding Zeppelin's Package I haven't tested that in this PoC, but it is on our plans. I do think both should be upgradeable, but I think it shouldn't be part of the standard, do you agree?

`Repo` vs `Package` naming

I like your thoughts on this topic :)
I'll discuss this with the rest of the team and let you guys know

luisivan · 2018-10-01T14:00:44Z

Got it, makes sense, thanks for the clarification @facuspagnuolo!

Regarding governance and upgradeability, I think it doesn't need to be a part of the interface, but a part of any of the implementations.

The minimum viable path is to work on a common interface and spec, which I think we are doing a good job at. But ideally we would also work on a canonical implementation that we can all use and build upon. For the spec and interface, we don't need to get into governance or upgradeability, but for the implementation, I think it's very necessary.

facuspagnuolo added 4 commits September 17, 2018 13:45

Init package manager project

b6e3716

Add ethereumjs-abi, bignumber and babel-polyfill

72c938a

Create ENS registry contract

fb10946

Create ENS resolver contract supporting ERC137 and ERC181

730d08a

facuspagnuolo added the status:in progress Pull Request is a work in progress label Sep 18, 2018

facuspagnuolo force-pushed the master branch from 1090a32 to ff47999 Compare September 18, 2018 11:56

facuspagnuolo changed the title ~~[WIP] Package manager implementation~~ [WIP] Package manager PoC Sep 18, 2018

Extend ENS resolver implementation to support content resolver interface

31c220d

facuspagnuolo force-pushed the package_manager branch 2 times, most recently from 443f3f0 to 7b42164 Compare September 21, 2018 10:27

facuspagnuolo added 3 commits September 21, 2018 14:59

Create ENS registrar contract

b3a9f90

Add ENS prefix to ENS implementation contracts

0878092

Add zeppelin and aragon registrars poc

c078540

facuspagnuolo force-pushed the package_manager branch from 7b42164 to c078540 Compare September 21, 2018 17:19

facuspagnuolo changed the title ~~[WIP] Package manager PoC~~ Package manager PoC Sep 21, 2018

facuspagnuolo added 4 commits September 24, 2018 12:20

Fix AragonRegistrar

e54920b

Add zos package implementation

79bd9ae

Add aragon repo implementation

c4c9b47

Build common package/repo implementation for both projects

dfd4422

facuspagnuolo force-pushed the package_manager branch from 40e5a5d to dfd4422 Compare September 24, 2018 16:44

facuspagnuolo changed the title ~~Package manager PoC~~ Package registry PoC Sep 25, 2018

facuspagnuolo removed the status:in progress Pull Request is a work in progress label Sep 25, 2018

facuspagnuolo requested a review from spalladino September 25, 2018 09:38

spalladino reviewed Sep 25, 2018

View reviewed changes

frangio reviewed Sep 25, 2018

View reviewed changes

facuspagnuolo mentioned this pull request Sep 26, 2018

Adapt Package to follow a standard interface as discussed with Aragon OpenZeppelin/openzeppelin-sdk#138

Closed

Package registry PoC #131

Are you sure you want to change the base?

Package registry PoC #131

Uh oh!

Conversation

facuspagnuolo commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spalladino commented Sep 21, 2018

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facuspagnuolo Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luisivan commented Sep 26, 2018

Rich governance

Upgradeability of the repos

Repo vs Package naming

Uh oh!

facuspagnuolo commented Sep 27, 2018

Governance

Upgradeability of the Repos

Repo vs Package naming

Uh oh!

luisivan commented Oct 1, 2018

Uh oh!

Uh oh!

facuspagnuolo commented Sep 18, 2018 •

edited

Loading

facuspagnuolo Sep 27, 2018 •

edited

Loading

`Repo` vs `Package` naming

`Repo` vs `Package` naming