sort condition children so that complex types come first

gnosisguild · Jul 9, 2023 · 23ab29f · 23ab29f
1 parent f24f8bf
commit 23ab29f
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 7 deletions.
diff --git a/packages/sdk/src/conditions/checkConditionIntegrity.ts b/packages/sdk/src/conditions/checkConditionIntegrity.ts
@@ -15,6 +15,10 @@ export const checkRootConditionIntegrity = (condition: Condition): void => {
   checkConditionIntegrityRecursive(condition)
 }
 
+/**
+ * Validates that logical condition children have consistent types.
+ * Since the children conditions address the very same value it does not make sense for them to declare incompatible param types.
+ * */
 const checkConsistentChildrenTypes = (condition: Condition): ParameterType => {
   if (condition.paramType !== ParameterType.None) {
     return condition.paramType
@@ -24,12 +28,34 @@ const checkConsistentChildrenTypes = (condition: Condition): ParameterType => {
   condition.children?.forEach((child) => {
     const childType = checkConsistentChildrenTypes(child)
     if (childType === ParameterType.None) return
-    if (expectedType && childType !== expectedType) {
+    if (!expectedType) {
+      expectedType = childType
+      return
+    }
+
+    if (childType === expectedType) return
+
+    if (
+      childType === ParameterType.Dynamic &&
+      (expectedType === ParameterType.Calldata ||
+        expectedType === ParameterType.AbiEncoded)
+    ) {
+      return
+    }
+
+    if (
+      (childType === ParameterType.Calldata ||
+        childType === ParameterType.AbiEncoded) &&
+      expectedType === ParameterType.Dynamic
+    ) {
       throw new Error(
-        `Inconsistent children types (\`${ParameterType[expectedType]}\` and \`${ParameterType[childType]}\`)`
+        `Mixed children types: \`${ParameterType[childType]}\` must appear before \`${ParameterType[expectedType]}\``
       )
     }
-    expectedType = childType
+
+    throw new Error(
+      `Inconsistent children types (\`${ParameterType[expectedType]}\` and \`${ParameterType[childType]}\`)`
+    )
   })
 
   return expectedType || ParameterType.None
@@ -50,6 +76,8 @@ const checkParamTypeIntegrity = (condition: Condition): void => {
       ParameterType.Dynamic,
       ParameterType.Tuple,
       ParameterType.Array,
+      ParameterType.Calldata,
+      ParameterType.AbiEncoded,
     ],
 
     [Operator.And]: [ParameterType.None],

diff --git a/packages/sdk/src/conditions/normalizeCondition.ts b/packages/sdk/src/conditions/normalizeCondition.ts
@@ -145,7 +145,18 @@ const normalizeChildrenOrder = (condition: Condition): Condition => {
     )
     // sort is in-place
     pairs.sort(([a], [b]) => (a.lt(b) ? -1 : 1))
-    const orderedChildren = pairs.map(([, child]) => child)
+    let orderedChildren = pairs.map(([, child]) => child)
+
+    // in case of mixed-type children (dynamic & calldata/abiEncoded), those with children must come first
+    const moveToFront = orderedChildren.filter(
+      (child) =>
+        child.paramType === ParameterType.Calldata ||
+        child.paramType === ParameterType.AbiEncoded
+    )
+    orderedChildren = [
+      ...moveToFront,
+      ...orderedChildren.filter((c) => !moveToFront.includes(c)),
+    ]
 
     return {
       ...condition,

diff --git a/packages/sdk/src/presets/authoring/conditions/matches.ts b/packages/sdk/src/presets/authoring/conditions/matches.ts
@@ -133,8 +133,8 @@ const calldataMatchesScopings =
       }
 
       return and(
-        bitmask({ mask: selector, value: selector }),
-        () => matchesCondition
+        () => matchesCondition,
+        bitmask({ mask: selector, value: selector })
       )(ParamType.from("bytes"))
     }
 
@@ -170,7 +170,7 @@ const calldataMatchesPresetFunction =
     })
 
     return (
-      condition ? and(selectorCondition, () => condition) : selectorCondition
+      condition ? and(() => condition, selectorCondition) : selectorCondition
     )(ParamType.from("bytes"))
   }
 

diff --git a/packages/sdk/test/checkConditionIntegrity.test.ts b/packages/sdk/test/checkConditionIntegrity.test.ts
@@ -52,6 +52,32 @@ describe("checkConditionIntegrity()", () => {
       })
     ).to.throw("Inconsistent children types (`Static` and `Dynamic`)")
   })
+
+  it("should not throw for And with Calldata and Dynamic children", () => {
+    expect(() =>
+      checkConditionIntegrity({
+        paramType: ParameterType.None,
+        operator: Operator.And,
+        children: [
+          { paramType: ParameterType.Calldata, operator: Operator.Pass },
+          { paramType: ParameterType.Dynamic, operator: Operator.Pass },
+        ],
+      })
+    ).to.not.throw()
+  })
+
+  it("should throw for And with Calldata and Dynamic children if Calldata does not come first", () => {
+    expect(() =>
+      checkConditionIntegrity({
+        paramType: ParameterType.None,
+        operator: Operator.And,
+        children: [
+          { paramType: ParameterType.Dynamic, operator: Operator.Pass },
+          { paramType: ParameterType.Calldata, operator: Operator.Pass },
+        ],
+      })
+    ).to.throw("Mixed children types: `Calldata` must appear before `Dynamic`")
+  })
 })
 
 describe("checkRootConditionIntegrity()", () => {