zkapp-cli icon indicating copy to clipboard operation
zkapp-cli copied to clipboard

RFC to show a warning when user must upgrade their zkapp-cli version

Open jasongitmail opened this issue 3 years ago • 6 comments
trafficstars

Current

During breaking upgrades to zkApps protocol (i.e. during QANet and Testnet) both the protocol and SnarkyJS need to advance to be compatible with one another. Developers who don't upgrade their zkapp-cli after the protocol receives such upgrades, can experience an error message about the "fee paying account not being found". The error message is unclear and leaves users without guidance for this situation.

Proposal

  • To address this, we'd like to check the latest version of zkapp-cli available on NPM and show a warning guiding user to upgrade their zkapp-cli via npm upgrade -g zkapp-cli && npm update snarkyjs. (Edit: Updated the command per comments below.)

Aspects to discuss:

  • While SnarkyJS and the zkApp CLI have a major version of 0, we can consider changes to the minor version of the zkapp-cli to represent breaking changes. When SnarkyJS and the zkApp have a major version of 1 or higher, we can consider changes to the major version of the zkapp-cli to be breaking, following semver.
  • Should this occur during every invocation of zk or only during zk deploy? Running zk deploy implies that the developer has access to the internet (which is not required for commands such as zk project). zk deploy is also the command where a user would encounter this error message, and never on other commands. For these two reasons, I suggest we implement this check as a first step of the zk deploy and NOT for any other commands within the zkApp CLI.

Behavior:

  • When zk deploy is run, fetch current version from NPM for zkapp-cli.
  • If version is deemed breaking from the current CLI version (see above), show warning message explaining and guiding dev to run npm update -g zkapp-cli.
  • Exit process and do not continue deployment.

jasongitmail avatar Sep 12 '22 21:09 jasongitmail

Thanks for writing this up! I agree re minor / major version and only doing this for zk deploy.

  • When zk deploy is run, fetch current version from NPM for zkapp-cli.
  • If version is deemed breaking from the current CLI version (see above), show warning message explaining and guiding dev to run npm update -g zkapp-cli.
  • Exit process and do not continue deployment.

In step 2, while there is a chance that updating the zkapp-CLI fixes a problem, in many cases it's actually snarkyjs that needs updating. Since the CLI takes snarkyjs from the user project (it has to), updating the CLI alone wouldn't fix it. So I propose to add that to the warning message and tell them to do something like npm update -g zkapp-cli && npm update snarkyjs.

That raises another question: In step 1, do we also need to check for the snarkyjs version? Or will "minor version change in snarkyjs" always imply "minor version change in the CLI", so checking the CLI version is enough?

Another question: Do we really want to exit the process (step 3 above) instead of just printing the warning and still trying to deploy? Given our heuristic ("minor version bump means breakage") is quite crude -- e.g., the version bump could mean breakage of some but not all zkApps, or it could mean that some other CLI aspect breaks but not the deploy command -- I would prefer that we still try to execute it, and ideally print the same warning again after we see it failed.

mitschabaude avatar Sep 13 '22 06:09 mitschabaude

npm update -g zkapp-cli && npm update snarkyjs.

+1 this is important. Updating the CLI would fix it for newly created projects only

I'm ok with not exiting the process too. The downside is that it will feel messier to the dev since they'll encounter two error messages, where the 2nd one is one that isn't useful at all. B/c if we're following the versioning pattern described for breaking changes, an error when deployment is attempted is always be expected to occur after our versioning warning error message is shown. So that's a bit of a downside--increasing the noise by adding one unhelpful error message. But no strong feelings on it

jasongitmail avatar Sep 13 '22 22:09 jasongitmail

This is great @jasongitmail.

So I propose to add that to the warning message and tell them to do something like npm update -g zkapp-cli && npm update snarkyjs.

+1 on guiding to update the zkapp-cli and SnarkyJS.

ymekuria avatar Sep 13 '22 23:09 ymekuria

I'm torn on if we should exit the process if the user has an old version of the CLI. I think error messages will be slightly messier, but I'm leaning towards not exiting the process and showing the warning if the deployment fails.

ymekuria avatar Sep 13 '22 23:09 ymekuria

After typing my response above and thinking about it more, I'm leaning back slightly toward exiting the process because: 1.) we will be following a pattern with our versioning that should allow us to have a quality signal for breaking changes, 2.) the failed deployment error message that a user would see by continuing adds noise and confusion to the situation for the dev. It turns our clear guidance into something less clear.

jasongitmail avatar Sep 14 '22 00:09 jasongitmail

I'm also fine with exiting the process. In this early phase, with snarkyjs being very unstable, it doesn't hurt if we soft-force users to always be on the latest version. In a more stable future, it's also fine because major upgrades will become rare.

To get a good signal, we'll have to make the cli version track compatibility of snarkyjs with the current network, bumping the version even if the cli didn't change. But that's ok I think!

mitschabaude avatar Sep 14 '22 06:09 mitschabaude