Check for missing calls to transitive parent initializers - WIP

packages/core/contracts/test/ValidationsInitializer.sol

@@ -323,6 +323,61 @@ contract Child_Has_PrivateInitializer_Bad is Parent__OnlyInitializingModifier {
 
 // ==== Transitive initialization ====
 
+abstract contract TransitiveGrandparent1 is Initializable {
+  uint x;
+  function __TransitiveGrandparent1_init() onlyInitializing internal {
+    x = 1;
+  }
+}
+
+abstract contract TransitiveGrandparent2 is Initializable {
+  uint y;
+  function __TransitiveGrandparent2_init() onlyInitializing internal {
+    y = 1;
+  }
+}
+
+contract TransitiveParent_Ok is TransitiveGrandparent1, TransitiveGrandparent2 {
+  function initializeParent() initializer public {
+    __TransitiveGrandparent1_init();
+    __TransitiveGrandparent2_init();
+  }
+}
+
+contract TransitiveParent_Bad is TransitiveGrandparent1, TransitiveGrandparent2 {
+  function initializeParent() initializer public {
+    __TransitiveGrandparent1_init();
+    // Does not call __TransitiveGrandparent2_init, and this contract is not abstract, so it is required
+  }
+}
+
+contract TransitiveChild_Bad_Parent is TransitiveParent_Bad { // this contract is ok but the parent is not
+  function initialize() initializer public {
+    initializeParent();
+  }
+}
+
+contract TransitiveChild_Bad_Order is TransitiveParent_Bad { // grandparent should be initialized first
+  function initialize() initializer public {
+    initializeParent();
+    __TransitiveGrandparent2_init();
+  }
+}
+
+contract TransitiveChild_Bad_Order2 is TransitiveParent_Bad { // this contract is ok but the parent is not
+  function initialize() initializer public {
+    __TransitiveGrandparent2_init();
+    initializeParent();
+  }
+}
+
+contract TransitiveDuplicate_Bad is TransitiveGrandparent1, TransitiveParent_Ok {
+  function initialize() initializer public {
+    __TransitiveGrandparent1_init();
+    initializeParent();
+  }
+}
+
 contract Ownable_Ok is Initializable, ERC20Upgradeable, OwnableUpgradeable {
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {

packages/core/src/validate-initializers.test.ts

@@ -37,17 +37,27 @@ function testAccepts(name: string, kind: ValidationOptions['kind']) {
   testOverride(name, kind, {}, undefined);
 }
 
-function testRejects(name: string, kind: ValidationOptions['kind'], expectedErrorContains: string) {
-  testOverride(name, kind, {}, expectedErrorContains);
+function testRejects(
+  name: string,
+  kind: ValidationOptions['kind'],
+  expectedError?: {
+    contains: string;
+    count: number;
+  },
+) {
+  testOverride(name, kind, {}, expectedError);
 }
 
 function testOverride(
   name: string,
   kind: ValidationOptions['kind'],
   opts: ValidationOptions,
-  expectErrorContains?: string,
+  expectedError?: {
+    contains: string;
+    count: number;
+  },
 ) {
-  const expectValid = expectErrorContains === undefined;
+  const expectValid = expectedError === undefined;
 
   const optKeys = Object.keys(opts);
   const describeOpts = optKeys.length > 0 ? '(' + optKeys.join(', ') + ')' : '';
@@ -59,83 +69,112 @@ function testOverride(
       t.notThrows(assertUpgSafe);
     } else {
       const error = t.throws(assertUpgSafe) as ValidationErrors;
-      t.true(error.message.includes(expectErrorContains), error.message);
+      t.true(
+        error.errors.length === expectedError.count,
+        `Expected ${expectedError.count} errors, got ${error.errors.length}:\n${error.message}`,
+      );
+      t.true(error.message.includes(expectedError.contains), error.message);
     }
   });
 }
 
 testAccepts('Child_Of_NoInitializer_Ok', 'transparent');
 
 testAccepts('Child_Of_InitializerModifier_Ok', 'transparent');
-testRejects(
-  'Child_Of_InitializerModifier_Bad',
-  'transparent',
-  'Contract is missing initializer calls for one or more parent contracts: `Parent_InitializerModifier`',
-);
+testRejects('Child_Of_InitializerModifier_Bad', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `Parent_InitializerModifier`',
+  count: 1,
+});
 testAccepts('Child_Of_InitializerModifier_UsesSuper_Ok', 'transparent');
 
 testAccepts('Child_Of_ReinitializerModifier_Ok', 'transparent');
-testRejects(
-  'Child_Of_ReinitializerModifier_Bad',
-  'transparent',
-  'Contract is missing initializer calls for one or more parent contracts: `Parent_ReinitializerModifier`',
-);
+testRejects('Child_Of_ReinitializerModifier_Bad', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `Parent_ReinitializerModifier`',
+  count: 1,
+});
 
 testAccepts('Child_Of_OnlyInitializingModifier_Ok', 'transparent');
-testRejects(
-  'Child_Of_OnlyInitializingModifier_Bad',
-  'transparent',
-  'Contract is missing initializer calls for one or more parent contracts: `Parent__OnlyInitializingModifier`',
-);
+testRejects('Child_Of_OnlyInitializingModifier_Bad', 'transparent', {
+  contains:
+    'Contract is missing initializer calls for one or more parent contracts: `Parent__OnlyInitializingModifier`',
+  count: 1,
+});
+
+testRejects('MissingInitializer_Bad', 'transparent', {
+  contains: 'Contract is missing an initializer',
 
-testRejects('MissingInitializer_Bad', 'transparent', 'Contract is missing an initializer');
+  count: 1,
+});
 testAccepts('MissingInitializer_UnsafeAllow_Contract', 'transparent');
 testOverride('MissingInitializer_Bad', 'transparent', { unsafeAllow: ['missing-initializer'] });
 
 testAccepts('InitializationOrder_Ok', 'transparent');
 testAccepts('InitializationOrder_Ok_2', 'transparent');
 
-testRejects(
-  'InitializationOrder_WrongOrder_Bad',
-  'transparent',
-  'Contract has an incorrect order of parent initializer calls. Expected initializers to be called for parent contracts in the following order: A, B, C',
-);
+testRejects('InitializationOrder_WrongOrder_Bad', 'transparent', {
+  contains: 'Expected initializers to be called for parent contracts in the following order: A, B, C',
+  count: 1,
+});
 testAccepts('InitializationOrder_WrongOrder_UnsafeAllow_Contract', 'transparent');
 testAccepts('InitializationOrder_WrongOrder_UnsafeAllow_Function', 'transparent');
 testOverride('InitializationOrder_WrongOrder_Bad', 'transparent', { unsafeAllow: ['incorrect-initializer-order'] });
 
-testRejects(
-  'InitializationOrder_MissingCall_Bad',
-  'transparent',
-  'Contract is missing initializer calls for one or more parent contracts: `C`',
-);
+testRejects('InitializationOrder_MissingCall_Bad', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `C`',
+  count: 1,
+});
 testAccepts('InitializationOrder_MissingCall_UnsafeAllow_Contract', 'transparent');
 testAccepts('InitializationOrder_MissingCall_UnsafeAllow_Function', 'transparent');
 testOverride('InitializationOrder_MissingCall_Bad', 'transparent', { unsafeAllow: ['missing-initializer-call'] });
 
-testRejects(
-  'InitializationOrder_Duplicate_Bad',
-  'transparent',
-  'Contract has duplicate calls to parent initializer `__B_init` for contract `B`',
-);
+testRejects('InitializationOrder_Duplicate_Bad', 'transparent', {
+  contains: 'Contract has duplicate calls to parent initializer `__B_init` for contract `B`',
+  count: 1,
+});
 testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Contract', 'transparent');
 testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Function', 'transparent');
 testAccepts('InitializationOrder_Duplicate_UnsafeAllow_Call', 'transparent');
 testOverride('InitializationOrder_Duplicate_Bad', 'transparent', { unsafeAllow: ['duplicate-initializer-call'] });
-testRejects(
-  'InitializationOrder_UnsafeAllowDuplicate_But_WrongOrder',
-  'transparent',
-  'Contract has an incorrect order of parent initializer calls. Expected initializers to be called for parent contracts in the following order: A, B, C',
-);
+testRejects('InitializationOrder_UnsafeAllowDuplicate_But_WrongOrder', 'transparent', {
+  contains: 'Expected initializers to be called for parent contracts in the following order: A, B, C',
+  count: 1,
+});
 
 testAccepts('Child_Of_ParentPrivateInitializer_Ok', 'transparent');
 testAccepts('Child_Of_ParentPublicInitializer_Ok', 'transparent');
-testRejects('Child_Has_PrivateInitializer_Bad', 'transparent', 'Contract is missing an initializer');
+testRejects('Child_Has_PrivateInitializer_Bad', 'transparent', {
+  contains: 'Contract is missing an initializer',
+  count: 1,
+});
+
+testAccepts('TransitiveParent_Ok', 'transparent');
+testRejects('TransitiveParent_Bad', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`',
+  count: 1,
+});
+testRejects('TransitiveChild_Bad_Parent', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`',
+  count: 3, // should be 2 if we ignore wrong order.  the errors are: missing for child, missing for parent
+});
+testRejects('TransitiveChild_Bad_Order', 'transparent', {
+  contains:
+    'Expected initializers to be called for parent contracts in the following order: TransitiveGrandparent2, TransitiveParent_Bad',
+  count: 2,
+}); // should have 2 errors: 'Expected initializers to be called for parent contracts in the following order: TransitiveGrandparent2, TransitiveParent_Bad', 'Contract is missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`'
+// but 1 if we ignore wrong order
+testRejects('TransitiveChild_Bad_Order2', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `TransitiveGrandparent2`',
+  count: 1,
+});
+testRejects('TransitiveDuplicate_Bad', 'transparent', {
+  contains: 'Contract has duplicate calls to parent',
+  count: 1,
+});
+// should allow this if we ignore duplicate calls transitively
 
 testAccepts('Ownable_Ok', 'transparent');
 testAccepts('Ownable2Step_Ok', 'transparent');
-testRejects(
-  'Ownable2Step_Bad',
-  'transparent',
-  'Contract is missing initializer calls for one or more parent contracts: `OwnableUpgradeable`',
-);
+testRejects('Ownable2Step_Bad', 'transparent', {
+  contains: 'Contract is missing initializer calls for one or more parent contracts: `OwnableUpgradeable`',
+  count: 1,
+});

packages/core/src/validate/report.ts

@@ -91,7 +91,9 @@ const errorInfo: ErrorDescriptions<ValidationError> = {
   },
   'incorrect-initializer-order': {
     msg: e =>
-      `Contract has an incorrect order of parent initializer calls. Expected initializers to be called for parent contracts in the following order: ${e.expectedLinearization.join(', ')}`,
+      `Contract has an incorrect order of parent initializer calls.
+- Expected initializers to be called for parent contracts in the following order: ${e.expectedLinearization.join(', ')}
+- Found order: ${e.foundOrder.join(', ')}`,
     hint: () => `Call parent initializers in the order they are inherited`,
     link: 'https://zpl.in/upgrades/error-001',
   },