OpenZeppelin / openzeppelin-sdk

OpenZeppelin SDK repository for CLI and upgrades.js. No longer actively developed.
MIT License
431 stars 200 forks source link

Option --no-interactive should not mean `false` #1544

Open kigawas opened 4 years ago

kigawas commented 4 years ago

It is assumed in a pile of places that interactive: false is equal to false, but it's not always correct.

I'll post some codes here.

// cli/src/commands/push.ts
async function runActionIfNeeded(contracts: string[], network: string, options: any): Promise<void> {
  if (!options.interactive) return; // why return?
  await action({ ...options, dontExitProcess: true, skipTelemetry: true, contracts });
}

async function promptForDeployDependencies(
  deployDependencies: boolean,
  network: string,
  interactive: boolean,
): Promise<{ deployDependencies: boolean | undefined }> {
  if (await ZWeb3.isGanacheNode()) return { deployDependencies: true };
  if (Dependency.hasDependenciesForDeploy(network)) {
    const opts = { deployDependencies };
    const props = getCommandProps(network);
    return promptIfNeeded({ opts, props }, interactive);
  }
  return { deployDependencies: undefined }; // why not true? even it's false, why undefined?
}

The code above causes the bug in #1538 , when --no-interactive enabled, it'll skip pushing dependencies.

frangio commented 4 years ago

Thanks for reporting @kigawas. This is definitely the case. Initially the reason was that under the interactivity umbrella we introduced many breaking changes to the behavior of the CLI, and --no-interactive was a way to opt out of the whole package. At this point this is only a historical artifact and it has unintuitive consequences that are harming usability.