Include build info dir in report if different

OpenZeppelin · Aug 16, 2024 · 977eb26 · 977eb26
1 parent a6f1f80
commit 977eb26
Show file tree

Hide file tree

Showing 9 changed files with 71 additions and 19 deletions.
diff --git a/packages/core/src/cli/cli.test.ts b/packages/core/src/cli/cli.test.ts
@@ -94,6 +94,21 @@ test('validate - single contract, reference', async t => {
   t.snapshot(expectation.join('\n'));
 });
 
+test('validate - single contract, reference with same build info dir', async t => {
+  const temp = await getTempDir(t);
+  const buildInfoDir = path.join(temp, 'build-info');
+  await fs.mkdir(buildInfoDir);
+
+  const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesBadLayout`);
+  await fs.writeFile(path.join(buildInfoDir, 'validate.json'), JSON.stringify(buildInfo));
+
+  const error = await t.throwsAsync(
+    execAsync(`${CLI} validate ${buildInfoDir} --contract BecomesBadLayout --reference build-info:StorageV1`),
+  );
+  const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`];
+  t.snapshot(expectation.join('\n'));
+});
+
 test('validate - single contract, reference, fully qualified names', async t => {
   const temp = await getTempDir(t);
   const buildInfo = await artifacts.getBuildInfo(`contracts/test/cli/Validate.sol:BecomesBadLayout`);
@@ -307,8 +322,9 @@ test('validate - references other build info dir by command - ok', async t => {
   const referenceBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV1.sol:MyContract`);
   await fs.writeFile(path.join(referenceDir, 'validate.json'), JSON.stringify(referenceBuildInfo));
 
-  const updatedDir = await getTempDir(t);
-  t.assert(updatedDir !== referenceDir);
+  const updatedDir = path.join(temp, 'build-info');
+  await fs.mkdir(updatedDir);
+
   const updatedBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV2_Ok.sol:MyContract`);
   await fs.writeFile(path.join(updatedDir, 'validate.json'), JSON.stringify(updatedBuildInfo));
 
@@ -326,8 +342,9 @@ test('validate - references other build info dir by command - bad', async t => {
   const referenceBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV1.sol:MyContract`);
   await fs.writeFile(path.join(referenceDir, 'validate.json'), JSON.stringify(referenceBuildInfo));
 
-  const updatedDir = await getTempDir(t);
-  t.assert(updatedDir !== referenceDir);
+  const updatedDir = path.join(temp, 'build-info');
+  await fs.mkdir(updatedDir);
+
   const updatedBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV2_Bad.sol:MyContract`);
   await fs.writeFile(path.join(updatedDir, 'validate.json'), JSON.stringify(updatedBuildInfo));
 
@@ -348,8 +365,9 @@ test('validate - references other build info dir by annotation - ok', async t =>
   const referenceBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV1.sol:MyContract`);
   await fs.writeFile(path.join(referenceDir, 'validate.json'), JSON.stringify(referenceBuildInfo));
 
-  const updatedDir = await getTempDir(t);
-  t.assert(updatedDir !== referenceDir);
+  const updatedDir = path.join(temp, 'build-info');
+  await fs.mkdir(updatedDir);
+
   const updatedBuildInfo = await artifacts.getBuildInfo(
     `contracts/test/cli/ValidateBuildInfoV2_Annotation_Ok.sol:MyContract`,
   );
@@ -369,7 +387,9 @@ test('validate - references other build info dir by annotation - bad', async t =
   const referenceBuildInfo = await artifacts.getBuildInfo(`contracts/test/cli/ValidateBuildInfoV1.sol:MyContract`);
   await fs.writeFile(path.join(referenceDir, 'validate.json'), JSON.stringify(referenceBuildInfo));
 
-  const updatedDir = await getTempDir(t);
+  const updatedDir = path.join(temp, 'build-info');
+  await fs.mkdir(updatedDir);
+
   t.assert(updatedDir !== referenceDir);
   const updatedBuildInfo = await artifacts.getBuildInfo(
     `contracts/test/cli/ValidateBuildInfoV2_Annotation_Bad.sol:MyContract`,
@@ -382,3 +402,5 @@ test('validate - references other build info dir by annotation - bad', async t =
   const expectation: string[] = [`Stdout: ${(error as any).stdout}`, `Stderr: ${(error as any).stderr}`];
   t.snapshot(expectation.join('\n'));
 });
+
+// TODO test that --contract does not have build info dir name
diff --git a/packages/core/src/cli/cli.test.ts.md b/packages/core/src/cli/cli.test.ts.md
@@ -189,6 +189,22 @@ Generated by [AVA](https://avajs.dev).
 
 ## validate - single contract, reference
 
+> Snapshot 1
+
+    `Stdout:  ✘  contracts/test/cli/Validate.sol:BecomesBadLayout (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊
+    ␊
+          StorageV1: Deleted \`x\`␊
+            > Keep the variable even if unused␊
+          ␊
+          StorageV1: Deleted \`__gap\`␊
+            > Keep the variable even if unused␊
+    ␊
+    FAILED␊
+    ␊
+    Stderr: `
+
+## validate - single contract, reference with same build info dir
+
 > Snapshot 1
 
     `Stdout:  ✘  contracts/test/cli/Validate.sol:BecomesBadLayout (upgrades from contracts/test/cli/Validate.sol:StorageV1)␊
@@ -313,7 +329,7 @@ Generated by [AVA](https://avajs.dev).
 
     {
       stderr: '',
-      stdout: ` ✔  contracts/test/cli/ValidateBuildInfoV2_Ok.sol:MyContract (upgrades from contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
+      stdout: ` ✔  contracts/test/cli/ValidateBuildInfoV2_Ok.sol:MyContract (upgrades from build-info-v1:contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
       ␊
       SUCCESS␊
       `,
@@ -323,7 +339,7 @@ Generated by [AVA](https://avajs.dev).
 
 > Snapshot 1
 
-    `Stdout:  ✘  contracts/test/cli/ValidateBuildInfoV2_Bad.sol:MyContract (upgrades from contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
+    `Stdout:  ✘  contracts/test/cli/ValidateBuildInfoV2_Bad.sol:MyContract (upgrades from build-info-v1:contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
     ␊
           MyContract: Deleted \`y\`␊
             > Keep the variable even if unused␊
@@ -342,7 +358,7 @@ Generated by [AVA](https://avajs.dev).
 
     {
       stderr: '',
-      stdout: ` ✔  contracts/test/cli/ValidateBuildInfoV2_Annotation_Ok.sol:MyContract (upgrades from contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
+      stdout: ` ✔  contracts/test/cli/ValidateBuildInfoV2_Annotation_Ok.sol:MyContract (upgrades from build-info-v1:contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
       ␊
       SUCCESS␊
       `,
@@ -352,7 +368,7 @@ Generated by [AVA](https://avajs.dev).
 
 > Snapshot 1
 
-    `Stdout:  ✘  contracts/test/cli/ValidateBuildInfoV2_Annotation_Bad.sol:MyContract (upgrades from contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
+    `Stdout:  ✘  contracts/test/cli/ValidateBuildInfoV2_Annotation_Bad.sol:MyContract (upgrades from build-info-v1:contracts/test/cli/ValidateBuildInfoV1.sol:MyContract)␊
     ␊
           MyContract: Deleted \`y\`␊
             > Keep the variable even if unused␊

diff --git a/packages/core/src/cli/cli.test.ts.snap b/packages/core/src/cli/cli.test.ts.snap
diff --git a/packages/core/src/cli/validate/build-info-file.test.ts b/packages/core/src/cli/validate/build-info-file.test.ts
@@ -406,7 +406,7 @@ test.serial('individual output selections - has layout', async t => {
   await fs.mkdir(dir, { recursive: true });
   await fs.writeFile(`${dir}/build-info.json`, JSON.stringify(BUILD_INFO_INDIVIDUAL_HAS_LAYOUT));
 
-  t.assert(((await getBuildInfoDirWithFiles(dir)).files).length === 1);
+  t.assert((await getBuildInfoDirWithFiles(dir)).files.length === 1);
 });
 
 test.serial('individual output selections - partial layout', async t => {

diff --git a/packages/core/src/cli/validate/build-info-file.ts b/packages/core/src/cli/validate/build-info-file.ts
@@ -49,6 +49,11 @@ export interface BuildInfoFile {
    * The Solidity compiler output JSON object.
    */
   output: SolcOutput;
+
+  /**
+   * Short name of the build info dir containing this file.
+   */
+  dirShortName: string;
 }
 
 export interface BuildInfoDirWithFiles {
@@ -67,7 +72,7 @@ export async function getBuildInfoDirWithFiles(buildInfoDir?: string): Promise<B
   const shortName = path.basename(dir);
   const jsonFiles = await getJsonFiles(dir);
 
-  return { dirShortName: shortName, files: await readBuildInfo(jsonFiles) };
+  return { dirShortName: shortName, files: await readBuildInfo(jsonFiles, shortName) }; // TODO remove redundant dirShortName
 }
 
 async function findDir(buildInfoDir?: string): Promise<string> {
@@ -130,7 +135,7 @@ async function getJsonFiles(dir: string): Promise<string[]> {
   return jsonFiles.map(file => path.join(dir, file));
 }
 
-async function readBuildInfo(buildInfoFilePaths: string[]) {
+async function readBuildInfo(buildInfoFilePaths: string[], dirShortName: string) {
   const buildInfoFiles: BuildInfoFile[] = [];
 
   for (const buildInfoFilePath of buildInfoFilePaths) {
@@ -150,6 +155,7 @@ async function readBuildInfo(buildInfoFilePaths: string[]) {
         input: buildInfoJson.input,
         output: buildInfoJson.output,
         solcVersion: buildInfoJson.solcVersion,
+        dirShortName: dirShortName,
       });
     }
   }

diff --git a/packages/core/src/cli/validate/contract-report.ts b/packages/core/src/cli/validate/contract-report.ts
@@ -130,7 +130,11 @@ function getUpgradeableContractReport(
     const referenceVersion = getContractVersion(referenceContract.validationData, referenceContract.fullyQualifiedName);
     const referenceLayout = getStorageLayout(referenceContract.validationData, referenceVersion);
 
-    reference = referenceContract.fullyQualifiedName;
+    if (referenceContract.buildInfoDirShortName !== contract.buildInfoDirShortName) {
+      reference = `${referenceContract.buildInfoDirShortName}:${referenceContract.fullyQualifiedName}`;
+    } else {
+      reference = referenceContract.fullyQualifiedName;
+    }
     storageLayoutReport = getStorageUpgradeReport(referenceLayout, layout, withValidationDefaults(opts));
   }
 

diff --git a/packages/core/src/cli/validate/find-contract.ts b/packages/core/src/cli/validate/find-contract.ts
@@ -64,5 +64,8 @@ export function findContract(
 
 function isMatchFound(contractName: string, foundContract: SourceContract, buildInfoDirShortName: string): boolean {
   const searchPrefix = buildInfoDirShortName === '' ? '' : `${buildInfoDirShortName}:`;
-  return `${searchPrefix}${foundContract.fullyQualifiedName}` === contractName || `${searchPrefix}${foundContract.name}` === contractName;
-}
+  return (
+    `${searchPrefix}${foundContract.fullyQualifiedName}` === contractName ||
+    `${searchPrefix}${foundContract.name}` === contractName
+  );
+}
diff --git a/packages/core/src/cli/validate/validate-upgrade-safety.ts b/packages/core/src/cli/validate/validate-upgrade-safety.ts
@@ -82,8 +82,7 @@ export function findSpecifiedContracts(
   if (contractName !== undefined) {
     return {
       contract: findContract(contractName, undefined, buildInfoDictionary, true), // only search main build info dir for the specified contract
-      reference:
-        referenceName !== undefined ? findContract(referenceName, undefined, buildInfoDictionary) : undefined,
+      reference: referenceName !== undefined ? findContract(referenceName, undefined, buildInfoDictionary) : undefined,
     };
   } else if (referenceName !== undefined) {
     throw new Error(`The reference option can only be specified when the contract option is also specified.`);

diff --git a/packages/core/src/cli/validate/validations.ts b/packages/core/src/cli/validate/validations.ts
@@ -13,6 +13,7 @@ export interface SourceContract {
   name: string;
   fullyQualifiedName: string;
   validationData: ValidationRunData;
+  buildInfoDirShortName: string;
 }
 
 export function validateBuildInfoContracts(buildInfoFiles: BuildInfoFile[]): SourceContract[] {
@@ -48,6 +49,7 @@ function addContractsFromBuildInfo(
         name: contractDef.name,
         fullyQualifiedName,
         validationData: validationData,
+        buildInfoDirShortName: buildInfoFile.dirShortName,
       });
     }
   }