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

Update "networks" in config.json to "deployAliases" #360

Merged
merged 20 commits into from
Feb 10, 2023

Conversation

ymekuria
Copy link
Collaborator

@ymekuria ymekuria commented Feb 8, 2023

Closes #217

This PR updates networks to deployAliases in the config.json and throughout the zkApp-cli for clarity. The config.js and deploy.js files were changed to support both cases if either networks or deployAliases are found in the config.json.

Copy link
Contributor

@jasongitmail jasongitmail left a comment

Choose a reason for hiding this comment

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

Left a comment of an approach that I think would clean it up a bit. Can we also use 'deploy alias' (instead of camel casing) for comments and text displayed to the user?

src/lib/config.js Outdated Show resolved Hide resolved
@@ -316,7 +326,7 @@ async function deploy({ alias, yes }) {
log(' Using the cached verification key');
}

let { fee } = config.networks[alias];
let { fee } = config[deployAliasesConfigName][alias];
Copy link
Contributor

@jasongitmail jasongitmail Feb 8, 2023

Choose a reason for hiding this comment

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

I think this would be easier to read if values were accessed at config.deployAliases consistently throughout the code. Then lines 45-48 would be updated to check if a deployAliases property exists, and if not, assigns the value of config?.networks to it.

i.e.
if (!config.hasOwnProperty('deployAliases')) config.deployAliases = config?.networks;

Naming is tricky here. We'd benefit from using a consistent pattern to name vars that distinguishes between property name (deployAliasName) and the object value (deployAlias).

Copy link
Collaborator Author

@ymekuria ymekuria Feb 8, 2023

Choose a reason for hiding this comment

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

I agree this approach is more readable, and I updated the code. The downside is if a user has an existing networks in their config.json, an additional deployAliases is added to the config.json after going through the zk config flow. Another option could be to copy the networks config into a new deployAliases config and delete the network config after. I'm hesitant to take this approach though.

Copy link
Contributor

Choose a reason for hiding this comment

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

The downside is if a user has an existing networks in their config.json, an additional deployAliases is added to the config.json after going through the zk config flow.
But only as a property on the temporary object. If we were writing it back to disk, that'd be a problem, but we don't so I think it's fine.

Copy link
Collaborator Author

@ymekuria ymekuria Feb 9, 2023

Choose a reason for hiding this comment

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

If a user has an existing networks config like this.

{
  "version": 1,
  "networks": {
    "networks-berkeley": {
      "url":   "https://proxy.berkeley.minaexplorer.com/graphql",
      "keyPath": "keys/networks-berkeley.json",
      "fee": ".1"
    }
  }
}

If they run zk config and add the deploy alias berkeley2, the resulting config.json with a networks and deploy aliases is below.

{
  "version": 1,
  "networks": {
    "networks-berkeley": {
      "url": "https://proxy.berkeley.minaexplorer.com/graphql",
      "keyPath": "keys/networks-berkeley.json",
      "fee": ".1"
    },
    "berkeley2": {
      "url": "https://proxy.berkeley.minaexplorer.com/graphql",
      "keyPath": "keys/berkeley2.json",
      "fee": ".1"
    }
  },
  "deployAliases": {
    "networks-berkeley": {
      "url": "https://proxy.berkeley.minaexplorer.com/graphql",
      "keyPath": "keys/networks-berkeley.json",
      "fee": ".1"
    },
    "berkeley2": {
      "url": "https://proxy.berkeley.minaexplorer.com/graphql",
      "keyPath": "keys/berkeley2.json",
      "fee": ".1"
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: How about when running zk config, we check if networks property exists and replace with deployAliases when writing it to the file, thereby transitioning the config. Then someday in the future (1yr+), we can clean up and remove this code and just assume deployAliases exists and let really, really old ones break. Or we could use the version number, but I kinda like the former.

wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should take the first approach. That is what I meant when I said this earlier but I didn't communicate it clearly.

Another option could be to copy the networks config into a new deployAliases config and delete the network config after.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me!

src/lib/deploy.js Show resolved Hide resolved
src/lib/deploy.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jasongitmail jasongitmail left a comment

Choose a reason for hiding this comment

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

This looks great!

Left some minor comments. I think there are some places where it can still use deployAliasName when referring to the string name. I marked a few, but there's more--worth a look over.

src/lib/config.js Outdated Show resolved Hide resolved
src/lib/config.js Outdated Show resolved Hide resolved
src/lib/config.js Outdated Show resolved Hide resolved
src/lib/config.js Outdated Show resolved Hide resolved
src/lib/deploy.js Outdated Show resolved Hide resolved
src/lib/config.js Outdated Show resolved Hide resolved
src/lib/config.js Outdated Show resolved Hide resolved
@ymekuria ymekuria merged commit afec10d into main Feb 10, 2023
@ymekuria ymekuria deleted the feature/deploy-alias-config branch March 14, 2023 15:09
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

Successfully merging this pull request may close these issues.

Update "networks" in config.json to "deployAliases"
2 participants