Remove Release contract #168

facuspagnuolo · 2018-06-13T03:48:35Z

Fixes #69

I removed the Release contract and its JS wrappers.
I refactored a the rest of JS wrappers architecture in order to represent the real hierarchy we have of directory contracts.

fiiiu

Partial Review: solidity code and modified tests LGTM, plz see minor comments.

fiiiu · 2018-06-13T16:23:54Z

contracts/application/AppDirectory.sol

@@ -7,8 +7,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 /**
 * @title AppDirectory
 * @dev Implementation directory with a standard library as a fallback provider.
- * If the implementation is not found in the directory, it will search in the
- * standard library.
+ * @dev If the implementation is not found in the directory, it will search in the standard library.


Two minor style things here:

The @dev in lines after the first was deliberately removed in a prior round of editing. @ElOpio plz confirm that we do not want them there.

I believe the max-len for lines is 120 (as per this), but again, someone from #static should check.

Regarding @dev there is a good explanation about it here: OpenZeppelin/openzeppelin-contracts#995 (comment)

I don't understand the comments about max-len. These are all below 120. @fiiiu Can you clarify what change you're suggesting, if any?

Sorry I was unclear, I was simply asking whether the max-len is indeed 120, because these comments were recently edited (and split in several lines) by @ElOpio, who is on top of the style guides..

If it is indeed 120, it's good as it is (in the PR).

Oh! But I did suggest removing the second @dev.

fiiiu · 2018-06-13T16:25:26Z

contracts/application/AppDirectory.sol

    stdlib = _stdlib;
  }

  /**
   * @dev Returns the implementation address for a given contract name.
-   * If the implementation is not found in the directory, it will search in the
-   * standard library.
+   * @dev If the implementation is not found in the directory, it will search in the standard library.


fiiiu · 2018-06-13T16:26:07Z

contracts/application/AppDirectory.sol

   * @param contractName Name of the contract.
-   * @return Address where the contract is implemented, or 0 if it is not
-   * found.
+   * @return Address where the contract is implemented, or 0 if it is not found.


See max-len comment above, also other instances below.

fiiiu · 2018-06-13T16:35:43Z

contracts/application/versioning/ImplementationDirectory.sol

+   */
+  function changeDeveloper(address newDeveloper) external onlyDeveloper {
+    require(newDeveloper != address(0));
+    emit DeveloperChanged(developer, newDeveloper);


emit after the change?

I followed what we did in AdminUpgradeabilityProxy

zos-lib/contracts/upgradeability/AdminUpgradeabilityProxy.sol

Lines 71 to 75 in 47edcb5

function changeAdmin(address newAdmin) external ifAdmin {

require(newAdmin != address(0));

emit AdminChanged(_admin(), newAdmin);

_setAdmin(newAdmin);

}

should we change that too?

fiiiu · 2018-06-13T17:03:21Z

contracts/application/versioning/ImplementationDirectory.sol

+   * @dev It sets the the developer of this implementation directory.
+   */
+  function ImplementationDirectory(address _developer) public {
+    developer = _developer;


In Release, the developer was set to msg.sender, thus the owner and the developer were the same. Here, they can be independently modified.

Maybe the gist of this PR includes separating these two, but it's not clear from the issue (#69), and I'm yet to see a place where this is actually used. I believe this was relevant in the context of an app's stdlib.. but still, are we adding functionality or we just need an alias? (In an AppDirectory, there will now be: 1) its owner, 2) its developer, 3) its stdlib owner, and 4) its stdlib developer.. )

Exactly, that was intentional...
IMO, having just an alias doesn't make much sense. We are now making possible to have an owner responsible to set/unset implementations and additionally a developer, that in a future could receive payouts for a stdlib. However, it's just an idea, we can make these two the same address

facuspagnuolo · 2018-06-14T19:57:15Z

please @fiiiu review again 🙏

frangio

I'm a bit uncomfortable with the duplication of code in the PackageWith... classes (and their long names). Can we merge them into a common abstraction parameterized by the type of package that they use?

I haven't reviewed the tests. I think this PR is in going in the right direction by abstracting the directory logic of a package into the newVersion function. I would be interested in exploring a bit further the possibility to reduce code duplication across the different package types.

frangio · 2018-06-14T20:05:55Z

src/app/App.js

@@ -37,7 +37,7 @@ export default class App {
    this.txParams = txParams
  }

-  address() {
+  get address() {
    return this._app.address


What is the type of this._app here? We need some inline documentation in these files.

_app is the PackagedApp contract
We have another issue to tackle inline doc for the lib

frangio · 2018-06-14T20:08:22Z

src/app/App.js

-    log.info(`Setting stdlib ${stdlibAddress}...`)
-    await this.currentDirectory().setStdlib(stdlibAddress, this.txParams)
-    return stdlibAddress
+    return this.currentDirectory().setStdlib(stdlibAddress)


Doesn't this return a tx receipt? Shouldn't we be hiding that?

Ignore this. I was wrong.

No, the directory is a wrapped AppDirectory here, it will return the address directly

frangio · 2018-06-14T20:16:30Z

src/package/PackageWithFreezableDirectories.js

+
+  static async fetch(address, txParams = {}) {
+    const provider = new PackageProvider(txParams)
+    return await provider.fetchForFrezzableDirectories(address)


Typo here: Frezzable

fiiiu

Solidity code and modified tests LGTM!

fiiiu · 2018-06-14T20:13:16Z

contracts/application/AppDirectory.sol

@@ -7,8 +7,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 /**
 * @title AppDirectory
 * @dev Implementation directory with a standard library as a fallback provider.
- * If the implementation is not found in the directory, it will search in the
- * standard library.
+ * @dev If the implementation is not found in the directory, it will search in the standard library.


Oh! But I did suggest removing the second @dev.

fiiiu · 2018-06-14T20:13:28Z

contracts/application/AppDirectory.sol

    stdlib = _stdlib;
  }

  /**
   * @dev Returns the implementation address for a given contract name.
-   * If the implementation is not found in the directory, it will search in the
-   * standard library.
+   * @dev If the implementation is not found in the directory, it will search in the standard library.


frangio · 2018-06-14T20:33:04Z

src/app/App.js

@@ -92,11 +79,11 @@ export default class App {
      ? await this._createProxy(contractName)
      : await this._createProxyAndCall(contractClass, contractName, initMethodName, initArgs)

-    log.info(` TX receipt received: ${receipt.transactionHash}`)


For the record the convention afaict was to log.info("Doing x") and then log.info(" Did x") with the space in the beginning to hint at its relation to the previous log message. (I opened #172 to replace all this by something nicer.)

…tories hierarchy

facuspagnuolo · 2018-06-15T16:28:39Z

@frangio thanks for reviewing, I addressed your comments, please review again 🙏

frangio · 2018-06-16T15:50:39Z

@facuspagnuolo It's not necessary to use npx because npm runs its scripts with the proper $PATH so as to find the local modules first.

facuspagnuolo · 2018-06-16T21:51:27Z

I know @frangio, I was just trying to see if Travis stops complaining about not finding dependencies

frangio · 2018-06-17T23:30:51Z

I'm going to take a shot at fixing the broken build.

frangio · 2018-06-18T00:39:59Z

I deleted all caches on Travis and it is working now. Very weird.

facuspagnuolo · 2018-06-18T13:19:46Z

Thanks @frangio !

frangio

As we discussed offline, there are a few places where the JS wrappers reimplement logic that should be delegated to the smart contract.

I found this PR to be too large. I did my best reviewing and I think the pull request is ok, but I can't be too confident about it. There are too many changes to focus on given the limited time that I have and my lack of knowledged of the codebase. Can we try to aim for smaller PRs next time? 🙂

frangio · 2018-06-18T14:49:24Z

src/app/App.js

@@ -37,7 +37,7 @@ export default class App {
    this.txParams = txParams
  }

-  address() {
+  get address() {
    return this._app.address


This is backwards-incompatible, for the record.

frangio · 2018-06-18T15:00:16Z

src/app/App.js

@@ -50,40 +50,27 @@ export default class App {
  }

  async getImplementation(contractName) {
-    const directory = this.currentDirectory()
-    return directory.getImplementation(contractName)
+    return this.package.getImplementation(this.version, contractName)


This should be delegated to the smart contract instead of duplicating the logic.

frangio · 2018-06-18T15:08:42Z

src/package/Package.js

-    return new Release(releaseAddress)
+  async getImplementation(version, contractName) {
+    const implementationDirectory = await this.getImplementationDirectory(version)
+    return implementationDirectory.getImplementation(contractName)


This should be delegated to the smart contract as well.

frangio · 2018-06-18T15:35:18Z

src/app/App.js

+  async newVersion(version, stdlibAddress = 0x0) {
+    const directory = await this.package.newVersion(version, stdlibAddress)
+    await this._app.setVersion(version, this.txParams)
+    log.info(`Version set`)


This message could be a bit more informative like Version ${version} set in app.

frangio · 2018-06-18T15:42:25Z

src/app/App.js

+    const directory = await this.package.newVersion(version, stdlibAddress)
+    await this._app.setVersion(version, this.txParams)
+    log.info(`Version set`)
+    this.directories[version] = directory


Do we need this.directories at all? It's apparently only used in currentDirectory, which duplicates the smart contract logic of BaseApp#getProvider.

frangio · 2018-06-18T16:01:42Z

src/directory/AppDirectoryProvider.js

+    this.txParams = txParams
+  }
+
+  async fetch(address) {


Why is this async if there is nothing asynchronous in its body? For comparison, PackageProvider#fetch isn't async.

frangio · 2018-06-18T16:04:28Z

src/directory/FreezableImplementationDirectory.js

+
+  async freeze() {
+    this.log.info('Freezing implementation directory...')
+    await this.directory.freeze(this.txParams)


Should log "Frozen" after the await.

frangio · 2018-06-18T16:09:58Z

src/directory/ImplementationDirectoryDeployer.js

-      log.info('Registering implementation in release...')
-      await this.release.setImplementation(contractAlias, implementation.address, this.txParams)
+      log.info('Registering implementation in implementation directory...')
+      await this.directory.setImplementation(contractAlias, implementation.address, this.txParams)


Should log something after the await as well.

frangio · 2018-06-18T16:10:47Z

src/index.js

+  FreezableImplementationDirectory,
+  AppDirectory,
+  PackageWithAppDirectories,
+  PackageWithNonFreezableDirectories,


I think we need to simplify the external interface. We can discuss it in another issue.

frangio · 2018-06-18T16:15:37Z

src/utils/Contracts.js

@@ -14,7 +14,7 @@ const DEFAULT_COVERAGE_TX_PARAMS = {

 export default {
  getFromLocal(contractName) {
-    const buildDir = `${process.cwd()}/build/contracts/`
+    const buildDir = `${process.cwd()}/build/contracts`


Let's try to keep these random changes out of random PRs. 😅

facuspagnuolo · 2018-06-18T20:05:09Z

Thanks a lot for the review @frangio! Comments addressed. Regarding the Package interface, I do agree. I pushed another PR to reduce the interface the user should care about. We are now exporting just one object, and the user is free to choose which type of directory will be using, otherwise, AppDirectory will be used by default

frangio · 2018-06-18T21:02:02Z

test/src/app/App.test.js

@@ -32,12 +32,11 @@ contract('App', function ([_, owner]) {

    it('initializes all app properties', async function () {
      this.app.version.should.eq(initialVersion);
-      this.app.directories.should.have.key(initialVersion);
+      this.app.directory.should.not.be.null


This type of assertion doesn't work because if this.app.directory is ever null, it will fail with "cannot access property should of null".

I've used should.exist(this.app.directory) before, but you need to const should = require('chai').should() at the beginning of the file, or we could export it from the setup and do const should = require('../../setup');.

Additionally, this probably only checks for null but we don't want it to be undefined either.

facuspagnuolo requested review from frangio and fiiiu June 13, 2018 03:54

facuspagnuolo added the status:review PR waiting for review label Jun 13, 2018

fiiiu reviewed Jun 13, 2018

View reviewed changes

zeppelinos deleted a comment from coveralls Jun 13, 2018

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch 3 times, most recently from 7383b4a to 650e7e4 Compare June 14, 2018 16:31

frangio reviewed Jun 14, 2018

View reviewed changes

fiiiu reviewed Jun 14, 2018

View reviewed changes

frangio reviewed Jun 14, 2018

View reviewed changes

frangio closed this Jun 14, 2018

frangio reopened this Jun 14, 2018

facuspagnuolo added 6 commits June 15, 2018 13:26

Remove Release contract

3f5f360

Refactor directory wrapper to represent the real implementation direc…

c172403

…tories hierarchy

Refactor package wrapper allowing to work with any directory type

45bc5ee

Refactor App wrapper to work with new directory wrapper models

daa8e98

Fix dir path used to fetch local contracts

1ec1b3a

Define address function as a getter for JS wrappers

7751751

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from ecfccbf to b6bce97 Compare June 15, 2018 16:27

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch 2 times, most recently from b6bce97 to 7751751 Compare June 15, 2018 22:21

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from 0432aea to f3d3d20 Compare June 16, 2018 21:49

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from f3d3d20 to 51e0899 Compare June 17, 2018 22:53

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from ddb0708 to 6fe5ccc Compare June 18, 2018 13:30

Refactor code duplication from package wrappers

14a7960

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from 6fe5ccc to 14a7960 Compare June 18, 2018 13:45

frangio changed the title ~~Add developer to implementation directories~~ Remove Release contract Jun 18, 2018

frangio reviewed Jun 18, 2018

View reviewed changes

Final tweaks on package wrappers

34c46a4

facuspagnuolo force-pushed the feature/add_developer_to_implementation_directories branch from 5cc5a4f to 34c46a4 Compare June 18, 2018 19:46

facuspagnuolo mentioned this pull request Jun 18, 2018

Reduce Package wrapper interface #177

Merged

frangio approved these changes Jun 18, 2018

View reviewed changes

facuspagnuolo merged commit fb74556 into development Jun 19, 2018

facuspagnuolo deleted the feature/add_developer_to_implementation_directories branch June 19, 2018 16:13

	function changeAdmin(address newAdmin) external ifAdmin {
	require(newAdmin != address(0));
	emit AdminChanged(_admin(), newAdmin);
	_setAdmin(newAdmin);
	}

Remove Release contract #168

Remove Release contract #168

Conversation

facuspagnuolo commented Jun 13, 2018 • edited Loading

fiiiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

facuspagnuolo commented Jun 14, 2018

frangio left a comment • edited Loading

Choose a reason for hiding this comment

frangio Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiiiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Jun 15, 2018

frangio commented Jun 16, 2018

facuspagnuolo commented Jun 16, 2018

frangio commented Jun 17, 2018

frangio commented Jun 18, 2018

facuspagnuolo commented Jun 18, 2018

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

facuspagnuolo commented Jun 13, 2018 •

edited

Loading

frangio Jun 13, 2018 •

edited

Loading

facuspagnuolo Jun 14, 2018 •

edited

Loading

frangio left a comment •

edited

Loading

frangio Jun 14, 2018 •

edited

Loading

frangio Jun 18, 2018 •

edited

Loading

facuspagnuolo commented Jun 18, 2018 •

edited

Loading