Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Preserve App, Package and Directory addresses #120

Merged
merged 7 commits into from
Sep 25, 2018

Conversation

jbcarpanelli
Copy link
Contributor

Fixes #35

  • Implement an app, package and directory address tracker in lib and cli.
  • Pick up already deployed contracts addresses on future push runs.

Now, only one method (fetchOrDeploy) is provided by both LibProject and
AppProject. This method will fetch the App, Package and/or Provider if possible,
and if not, will try to deploy them
@jbcarpanelli jbcarpanelli force-pushed the feature/save-partial-deployments branch from 297aab3 to c44a0e2 Compare September 25, 2018 12:40
Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

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

LGTM, left some small comments

project.directory = directory
project.package = thepackage
return project
static async fetchOrDeploy(name = 'main', version = '0.1.0', txParams = {}, { appAddress = undefined, packageAddress = undefined }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the default values for name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say so, it simplifies usage of the Project class directly

return project
} catch(deployError) {
throw new DeployError(deployError.message, { thepackage, directory, app })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about splitting this method a bit to sth like:

  static async fetchOrDeploy(name = 'main', version = '0.1.0', txParams = {}, { appAddress = undefined, packageAddress = undefined }) {
    try {
      const app = await this.fetchOrDeployApp(...)
      const thepackage = await this.fetchOrDeployPackage(...)
      const directory = await this.fetchOrDeployDirectory(...)
      return this.buildProject(...)
    } catch(deployError) {
      throw new DeployError(deployError.message, { thepackage, directory, app })
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've discussed this offline. It's not a good idea to merge deploy and fetch methods in Package and App, as it would add complexity.
On the other hand, separating the logic to new static methods inside AppProject/LibProject (or BasePackageProject) just to make the code prettier is not reason enough to do so. In case we would want to provide this methods through zos-lib, we can add them, but this exceeds this PR's scope.

return project
} catch(deployError) {
throw new DeployError(deployError.message, { thepackage, directory })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to split this one too, it is not that large tho

this.unsetContracts()
])
}

async pushVersion() {
_checkVersion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@spalladino
Copy link
Contributor

@jcarpanelli feel free to merge after applying @facuspagnuolo's suggestions

@jbcarpanelli jbcarpanelli merged commit c407f72 into master Sep 25, 2018
@jbcarpanelli jbcarpanelli deleted the feature/save-partial-deployments branch September 25, 2018 20:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants