diff --git a/docs/modules/ROOT/pages/api-core.adoc b/docs/modules/ROOT/pages/api-core.adoc index 929359f92..f23b2d5de 100644 --- a/docs/modules/ROOT/pages/api-core.adoc +++ b/docs/modules/ROOT/pages/api-core.adoc @@ -117,7 +117,8 @@ If any errors are found, the command will exit with a non-zero exit code and pri *Options:* * `--contract ` The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. -* `--reference ` The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the `--contract` option. If not specified, uses the `@custom:oz-upgrades-from` annotation in the contract that is being validated. +* `--reference ` Can only be used when the `--contract` option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. +* `--requireReference` Can only be used when the `--contract` option is also provided. If specified, requires either the `--reference` option to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. * `--unsafeAllow ""` - Selectively disable one or more validation errors. Comma-separated list with one or more of the following: `state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto` * `--unsafeAllowRenames` - Configure storage layout check to allow variable renaming. * `--unsafeSkipStorageCheck` - Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort. @@ -161,11 +162,12 @@ Note that this function does not throw validation errors directly. Instead, you * `buildInfoDir` - the path to the build info directory which contains JSON files with Solidity compiler input and output. Defaults to `artifacts/build-info` for Hardhat projects or `out/build-info` for Foundry projects. If your project uses a custom output directory, you must specify its build info directory here. * `contract` - The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. -* `reference` - The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with `contract`. If not specified, uses the `@custom:oz-upgrades-from` annotation in the contract that is being validated. +* `reference` - Can only be used when the `contract` argument is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the `@custom:oz-upgrades-from` annotation if it is defined in the contract that is being validated. * `opts` - an object with the following options as defined in xref:api-hardhat-upgrades.adoc#common-options[Common Options]: ** `unsafeAllow` ** `unsafeAllowRenames` ** `unsafeSkipStorageCheck` +** `requireReference` - Can only be used when the `contract` argument is also provided. If specified, requires either the `reference` argument to be provided or the contract to have a `@custom:oz-upgrades-from` annotation. *Returns:* diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 5140dcf87..7b1ad7285 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- CLI: Add `--requireReference` option. ([#900](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/900)) + ## 1.30.1 (2023-10-11) - Fix Hardhat compile error when using Solidity 0.5.x. ([#892](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/892)) diff --git a/packages/core/contracts/test/cli/Validate.sol b/packages/core/contracts/test/cli/Validate.sol index a9edea19e..0610f3e11 100644 --- a/packages/core/contracts/test/cli/Validate.sol +++ b/packages/core/contracts/test/cli/Validate.sol @@ -139,4 +139,16 @@ contract BothAnnotationsUnsafe { function sd() public { selfdestruct(payable(msg.sender)); } +} + +contract StorageV2_Bad_NoAnnotation { + uint256 public x; + uint256 public y; + uint256[49] private __gap; +} + +contract StorageV2_Ok_NoAnnotation { + uint256 public x; + uint256 public y; + uint256[48] private __gap; } \ No newline at end of file diff --git a/packages/core/src/cli/cli.test.ts b/packages/core/src/cli/cli.test.ts index ce5c0de30..303bab822 100644 --- a/packages/core/src/cli/cli.test.ts +++ b/packages/core/src/cli/cli.test.ts @@ -167,6 +167,73 @@ test('validate - reference not found', async t => { t.true(error?.message.includes('Could not find contract NonExistent.'), error?.message); }); +test('validate - requireReference - no contract option', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV1`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --requireReference`)); + t.true( + error?.message.includes('The --requireReference option can only be used along with the --contract option.'), + error?.message, + ); +}); + +test('validate - requireReference - no reference, no upgradesFrom', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV1`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync(execAsync(`${CLI} validate ${temp} --contract StorageV1 --requireReference`)); + t.true(error?.message.includes('does not specify what contract it upgrades from'), error?.message); +}); + +test('validate - requireReference - no reference, has upgradesFrom - safe', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesSafe`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const output = (await execAsync(`${CLI} validate ${temp} --contract BecomesSafe --requireReference`)).stdout; + t.snapshot(output); +}); + +test('validate - requireReference - no reference, has upgradesFrom - unsafe', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesBadLayout`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync( + execAsync(`${CLI} validate ${temp} --contract BecomesBadLayout --requireReference`), + ); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + +test('validate - requireReference - has reference - unsafe', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV2_Bad_NoAnnotation`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const error = await t.throwsAsync( + execAsync(`${CLI} validate ${temp} --contract StorageV2_Bad_NoAnnotation --reference StorageV1 --requireReference`), + ); + const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`]; + t.snapshot(expectation.join('\n')); +}); + +test('validate - requireReference - has reference - safe', async t => { + const temp = await getTempDir(t); + const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:StorageV2_Ok_NoAnnotation`); + await fs.writeFile(path.join(temp, 'validate.json'), JSON.stringify(buildInfo)); + + const output = ( + await execAsync( + `${CLI} validate ${temp} --contract StorageV2_Ok_NoAnnotation --reference StorageV1 --requireReference`, + ) + ).stdout; + t.snapshot(output); +}); + test('validate - no upgradeable', async t => { const temp = await getTempDir(t); const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Storage088.sol:Storage088`); diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md index c6ed50425..5281d45ae 100644 --- a/packages/core/src/cli/cli.test.ts.md +++ b/packages/core/src/cli/cli.test.ts.md @@ -17,7 +17,8 @@ Generated by [AVA](https://avajs.dev). ␊ Options:␊ --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ - --reference The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being validated.␊ + --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ + --requireReference Can only be used when the --contract option is also provided. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ @@ -36,7 +37,8 @@ Generated by [AVA](https://avajs.dev). ␊ Options:␊ --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated.␊ - --reference The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being validated.␊ + --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated.␊ + --requireReference Can only be used when the --contract option is also provided. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation.␊ --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: state-variable-assignment, state-variable-immutable, external-library-linking, struct-definition, enum-definition, constructor, delegatecall, selfdestruct, missing-public-upgradeto␊ --unsafeAllowRenames Configure storage layout check to allow variable renaming.␊ --unsafeSkipStorageCheck Skips checking for storage layout compatibility errors. This is a dangerous option meant to be used as a last resort.␊ @@ -215,6 +217,51 @@ Generated by [AVA](https://avajs.dev). ␊ Stderr: ` +## validate - requireReference - no reference, has upgradesFrom - safe + +> Snapshot 1 + + ` ✔ contracts/test/cli/Validate.sol:BecomesSafe (upgrades from contracts/test/cli/Validate.sol:MultipleUnsafe)␊ + ␊ + SUCCESS (1 upgradeable contracts detected, 1 passed, 0 failed)␊ + ` + +## validate - requireReference - no reference, has upgradesFrom - unsafe + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/Validate.sol:BecomesBadLayout (upgrades from contracts/test/cli/Validate.sol:BecomesSafe)␊ + ␊ + BecomesSafe: Deleted \`x\`␊ + > Keep the variable even if unused␊ + ␊ + FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊ + ␊ + Stderr: ` + +## validate - requireReference - has reference - unsafe + +> Snapshot 1 + + `Stdout: ✘ contracts/test/cli/Validate.sol:StorageV2_Bad_NoAnnotation (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊ + ␊ + contracts/test/cli/Validate.sol:147: Layout changed for \`__gap\` (uint256[49] -> uint256[49])␊ + - Slot changed from 1 to 2␊ + > Set __gap array to size 48␊ + ␊ + FAILED (1 upgradeable contract detected, 0 passed, 1 failed)␊ + ␊ + Stderr: ` + +## validate - requireReference - has reference - safe + +> Snapshot 1 + + ` ✔ contracts/test/cli/Validate.sol:StorageV2_Ok_NoAnnotation (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊ + ␊ + SUCCESS (1 upgradeable contracts detected, 1 passed, 0 failed)␊ + ` + ## validate - no upgradeable > Snapshot 1 diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap index cb96c5ccb..fa011da00 100644 Binary files a/packages/core/src/cli/cli.test.ts.snap and b/packages/core/src/cli/cli.test.ts.snap differ diff --git a/packages/core/src/cli/validate.ts b/packages/core/src/cli/validate.ts index 1b7773bfa..9ce8ae53d 100644 --- a/packages/core/src/cli/validate.ts +++ b/packages/core/src/cli/validate.ts @@ -1,9 +1,9 @@ import minimist from 'minimist'; import { ValidateUpgradeSafetyOptions, validateUpgradeSafety } from '.'; -import { withValidationDefaults } from '..'; import { ValidationError, errorKinds } from '../validate/run'; import debug from '../utils/debug'; +import { withCliDefaults } from './validate/validate-upgrade-safety'; const USAGE = 'Usage: npx @openzeppelin/upgrades-core validate [] []'; const DETAILS = ` @@ -14,7 +14,8 @@ Arguments: Options: --contract The name or fully qualified name of the contract to validate. If not specified, all upgradeable contracts in the build info directory will be validated. - --reference The name or fully qualified name of the reference contract to use for storage layout comparisons. Can only be used along with the --contract option. If not specified, uses the @custom:oz-upgrades-from annotation in the contract that is being validated. + --reference Can only be used when the --contract option is also provided. The name or fully qualified name of the reference contract to use for storage layout comparisons. If not specified, uses the @custom:oz-upgrades-from annotation if it is defined in the contract that is being validated. + --requireReference Can only be used when the --contract option is also provided. If specified, requires either the --reference option to be provided or the contract to have a @custom:oz-upgrades-from annotation. --unsafeAllow "" Selectively disable one or more validation errors. Comma-separated list with one or more of the following: ${errorKinds.join( ', ', )} @@ -45,6 +46,7 @@ function parseArgs(args: string[]) { 'unsafeSkipStorageCheck', 'unsafeAllowCustomTypes', 'unsafeAllowLinkedLibraries', + 'requireReference', ], string: ['unsafeAllow', 'contract', 'reference'], alias: { h: 'help' }, @@ -87,10 +89,15 @@ export function getFunctionArgs(parsedArgs: minimist.ParsedArgs, extraArgs: stri const buildInfoDir = extraArgs.length === 1 ? undefined : extraArgs[1]; const contract = getAndValidateString(parsedArgs, 'contract'); const reference = getAndValidateString(parsedArgs, 'reference'); - if (reference !== undefined && contract === undefined) { - throw new Error('The --reference option can only be used along with the --contract option.'); - } const opts = withDefaults(parsedArgs); + + if (contract === undefined) { + if (reference !== undefined) { + throw new Error('The --reference option can only be used along with the --contract option.'); + } else if (opts.requireReference) { + throw new Error('The --requireReference option can only be used along with the --contract option.'); + } + } return { buildInfoDir, contract, reference, opts }; } } @@ -117,6 +124,7 @@ function validateOptions(parsedArgs: minimist.ParsedArgs) { 'unsafeAllow', 'contract', 'reference', + 'requireReference', ].includes(key), ); if (invalidArgs.length > 0) { @@ -152,7 +160,8 @@ export function withDefaults(parsedArgs: minimist.ParsedArgs): Required, specifiedContracts?: SpecifiedContracts, ) { const upgradeableContractReports: UpgradeableContractReport[] = []; @@ -83,7 +83,11 @@ export function getContractReports( sourceContracts, specifiedContracts?.reference, ); - if (specifiedContracts !== undefined || upgradeabilityAssessment.upgradeable) { + if (opts.requireReference && upgradeabilityAssessment.referenceContract === undefined) { + throw new Error( + `The contract ${sourceContract.fullyQualifiedName} does not specify what contract it upgrades from. Add the \`@custom:oz-upgrades-from \` annotation to the contract, or include the reference contract name when running the validate command or function.`, + ); + } else if (specifiedContracts !== undefined || upgradeabilityAssessment.upgradeable) { const reference = upgradeabilityAssessment.referenceContract; const kind = upgradeabilityAssessment.uups ? 'uups' : 'transparent'; const report = getUpgradeableContractReport(sourceContract, reference, { ...opts, kind: kind }); diff --git a/packages/core/src/cli/validate/validate-upgrade-safety.test.ts b/packages/core/src/cli/validate/validate-upgrade-safety.test.ts index 0489592a1..149385ffb 100644 --- a/packages/core/src/cli/validate/validate-upgrade-safety.test.ts +++ b/packages/core/src/cli/validate/validate-upgrade-safety.test.ts @@ -6,7 +6,7 @@ import path from 'path'; import os from 'os'; import { artifacts } from 'hardhat'; -import { validateUpgradeSafety } from './validate-upgrade-safety'; +import { findSpecifiedContracts, validateUpgradeSafety, withCliDefaults } from './validate-upgrade-safety'; import { ReferenceContractNotFound } from './find-contract'; test.before(async () => { @@ -97,3 +97,15 @@ test('invalid annotation args - upgrades', async t => { ); t.true(error?.message.includes('Found 1, expected 0')); }); + +test('findSpecifiedContracts - requireReference option without contract', async t => { + try { + findSpecifiedContracts([], withCliDefaults({ requireReference: true })); + } catch (e: any) { + t.true( + e.message.includes( + 'The requireReference option can only be specified when the contract option is also specified.', + ), + ); + } +}); diff --git a/packages/core/src/cli/validate/validate-upgrade-safety.ts b/packages/core/src/cli/validate/validate-upgrade-safety.ts index 29ef56d9d..afcdb28f9 100644 --- a/packages/core/src/cli/validate/validate-upgrade-safety.ts +++ b/packages/core/src/cli/validate/validate-upgrade-safety.ts @@ -1,4 +1,4 @@ -import { ValidationOptions } from '../..'; +import { ValidationOptions, withValidationDefaults } from '../..'; import { getBuildInfoFiles } from './build-info-file'; import { getContractReports } from './contract-report'; @@ -9,7 +9,9 @@ import { SourceContract, validateBuildInfoContracts } from './validations'; /** * Validation options for upgrade safety checks. */ -export type ValidateUpgradeSafetyOptions = Omit; +export type ValidateUpgradeSafetyOptions = Omit & { + requireReference?: boolean; +}; export type SpecifiedContracts = { contract: SourceContract; @@ -32,17 +34,20 @@ export async function validateUpgradeSafety( reference?: string, opts: ValidateUpgradeSafetyOptions = {}, ): Promise { + const allOpts = withCliDefaults(opts); + const buildInfoFiles = await getBuildInfoFiles(buildInfoDir); const sourceContracts = validateBuildInfoContracts(buildInfoFiles); - const specifiedContracts = findSpecifiedContracts(sourceContracts, contract, reference); + const specifiedContracts = findSpecifiedContracts(sourceContracts, allOpts, contract, reference); - const contractReports = getContractReports(sourceContracts, opts, specifiedContracts); + const contractReports = getContractReports(sourceContracts, allOpts, specifiedContracts); return getProjectReport(contractReports); } -function findSpecifiedContracts( +export function findSpecifiedContracts( sourceContracts: SourceContract[], + opts: Required, contractName?: string, referenceName?: string, ): SpecifiedContracts | undefined { @@ -53,7 +58,16 @@ function findSpecifiedContracts( }; } else if (referenceName !== undefined) { throw new Error(`The reference option can only be specified when the contract option is also specified.`); + } else if (opts.requireReference) { + throw new Error(`The requireReference option can only be specified when the contract option is also specified.`); } else { return undefined; } } + +export function withCliDefaults(opts: ValidateUpgradeSafetyOptions): Required { + return { + ...withValidationDefaults(opts), + requireReference: opts.requireReference ?? false, + }; +} diff --git a/packages/core/src/cli/validate/validations.test.ts b/packages/core/src/cli/validate/validations.test.ts index feaed8d07..413538903 100644 --- a/packages/core/src/cli/validate/validations.test.ts +++ b/packages/core/src/cli/validate/validations.test.ts @@ -3,6 +3,7 @@ import _test, { ExecutionContext, TestFn } from 'ava'; import { artifacts } from 'hardhat'; import { validateBuildInfoContracts } from './validations'; import { UpgradeableContractReport, getContractReports } from './contract-report'; +import { withCliDefaults } from './validate-upgrade-safety'; interface Context { reports: UpgradeableContractReport[]; @@ -19,7 +20,7 @@ test.before(async t => { t.fail(); } else { const sourceContracts = validateBuildInfoContracts([buildInfo]); - t.context.reports = getContractReports(sourceContracts, {}); + t.context.reports = getContractReports(sourceContracts, withCliDefaults({})); } });