feat: add signed integer constraints and OR predicate composition#11
feat: add signed integer constraints and OR predicate composition#11fichiokaku wants to merge 5 commits into
Conversation
Community feedback on ERC-8211 identified two gaps in the constraint system: - Unsigned-only GTE/LTE constraints are incorrect for int256 values (two's complement makes negative numbers appear greater than positives in raw bytes32 comparison). - No way to express OR logic without deploying helper contracts.
8a79688 to
92bade9
Compare
| EQ, // Equal to (unsigned / bitwise) | ||
| GTE, // Greater than or equal to (unsigned) | ||
| LTE, // Less than or equal to (unsigned) | ||
| IN, // In range (unsigned) |
There was a problem hiding this comment.
EQ, and IN can be suitable for signed and unsigned as well. Please update the code comment here
| } else { | ||
| revert InvalidConstraintType(); | ||
| uint256 len = constraints.length; | ||
| for (uint256 i; i < len; i++) { |
There was a problem hiding this comment.
For gas optimization, we can also try to use unchecked {} block where the length will be always few and will never exceeds the bound
| if (c.constraintType == ConstraintType.OR) { | ||
| Constraint[] memory subs = abi.decode(c.referenceData, (Constraint[])); | ||
| bool anyMet; | ||
| for (uint256 j; j < subs.length; j++) { |
There was a problem hiding this comment.
For gas optimization, we can also try to use unchecked {} block where the length will be always few and will never exceeds the bound
| /// @dev Validate the constraints => compare the value with the reference data | ||
| /// @dev Validate the constraints => compare the value with the reference data. | ||
| /// Each constraints[i] is checked against the i-th 32-byte word of rawValue (AND semantics). | ||
| /// Use ConstraintType.OR to express OR semantics within a single word position. |
There was a problem hiding this comment.
Given the OR operand can be just 1 level. Should be support deep recursive levels as well.
I mean more than on level
[
{
type: "OR",
constraints: [
{
type: "OR",
constraints: [...] // and so on
},
{ ... }
]
}
]
vr16x
left a comment
There was a problem hiding this comment.
Overall, everything seems good and test coverage also good.
Just left few comments on gas optimizations and extended OR operand support
Summary
Community feedback on ERC-8211 surfaced two gaps in the constraint system that this PR addresses, plus a few refinements that came out of code review.
Problem 1 — Unsigned-only comparisons break for
int256GTEandLTEcompare rawbytes32values, which gives wrong results across the sign boundary:int256(-1)encodes as0xffff…ff, making it appear greater than any positive value under an unsigned comparison. Signed integer comparisons require explicit two's-complement casting.Fix: added
GTE_SIGNEDandLTE_SIGNEDtoConstraintType. These cast both sides throughint256(uint256(...))before comparing, which correctly handles two's-complement representation.Problem 2 — No OR composition without helper contracts
All constraints in an
InputParamare AND-composed. Users who need "value must satisfy at least one of these conditions" previously had to deploy a helper contract to express that.Fix: added
ConstraintType.OR. ItsreferenceDataisabi.encode(Constraint[])— a dynamic array of alternative leaf constraints. At least one must pass for the OR to succeed. OR is intentionally single-level (no nesting): a leaf constraint inside an OR cannot itself be another OR. This keeps what the user is signing flat and displayable; theInvalidConstraintTyperevert path is covered by a dedicated test so future refactors can't silently re-enable nesting.Changes
contracts/types/ComposabilityDataTypes.sol— addedGTE_SIGNED,LTE_SIGNED,ORtoConstraintType. Corrected the type-suitability comments onEQ(bitwise; works for signed/unsigned/addresses/bytes32) andIN(works for unsigned ranges and same-sign signed ranges).contracts/ComposableExecutionLib.sol— refactored_validateConstraintsinto a loop +_checkConstraint(bytes32, Constraint memory) returns (bool)helper. OR is handled inline in_validateConstraints(decoded once, evaluated against the same 32-byte word as the outer entry). Gas: both loops useunchecked { ++i; }on bounded counters. Docs: added an inline NatSpec block that contrasts AND (static 32/64-byte payloads) vs OR (dynamicabi.encode(Constraint[])) with a concrete encoding example.test/mock/DummyContract.sol— addedgetSignedValue() returns (int256)for STATIC_CALL-path signed tests.test/unit/ComposableExecution_ConstraintsReverts.sol— 6 new test functions, each fanned out across all four account execution paths (MockAccount,MockAccountFallback,MockAccountCaller,MockAccountDelegateCaller):test_inputs_With_GteSigned_Constraints— includes a case proving plainGTEincorrectly acceptsint256(-1) >= 0whileGTE_SIGNEDcorrectly rejects ittest_inputs_With_LteSigned_Constraints— boundary, above-bound (including positive vs negative bound), and below-bound casestest_inputs_With_GteSigned_StaticCall_Constraints— signed constraints through the STATIC_CALL fetchertest_inputs_With_Or_Constraints—EQ(0) OR GTE(100)across passing and failing valuestest_inputs_With_Or_Signed_Constraints—LTE_SIGNED(-100) OR GTE_SIGNED(0)mixing signed types inside ORtest_Nested_Or_Reverts_With_InvalidConstraintType— locks in the "no nesting" design decisionReview feedback addressed
uncheckedon bounded loops: applied to the outer constraint loop and the OR sub-array loop.forge testpasses: 28/28 (existing + 6 new).