From d8935c25194199431d0ccb72f06790c6b582422a Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Tue, 29 Oct 2024 10:44:54 +0400 Subject: [PATCH] rename, fix and add tests --- .../condition/extensions/RuledCondition.sol | 33 ++++-- .../condition/extensions/ruled-condition.ts | 104 ++++++++++++++++-- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/contracts/src/permission/condition/extensions/RuledCondition.sol b/contracts/src/permission/condition/extensions/RuledCondition.sol index be83dada..0842dffd 100644 --- a/contracts/src/permission/condition/extensions/RuledCondition.sol +++ b/contracts/src/permission/condition/extensions/RuledCondition.sol @@ -76,13 +76,13 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { super.supportsInterface(_interfaceId); } - /// @notice Retrieves all the rules stored in this contract. - /// @return An array of `Rule` structs representing the defined rules. + /// @notice Retrieves the current rules stored in this contract. + /// @return An array of `Rule` structs representing the currently defined rules. function getRules() public view virtual returns (Rule[] memory) { return rules; } - /// @notice Updates the set of rules stored in the contract. + /// @notice Updates the set of rules. /// @dev This function deletes the current set of rules and replaces it with a new one. /// @param _rules An new array of `Rule` structs to replace the current set of rules. function _updateRules(Rule[] memory _rules) internal virtual { @@ -149,7 +149,7 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { return uint256(value) > 0; } - return _compare(value, Op(rule.op), comparedTo); + return _compare(value, comparedTo, Op(rule.op)); } /// @notice Evaluates logical operations. @@ -275,7 +275,12 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { return result; } - function _compare(uint256 _a, Op _op, uint256 _b) internal pure returns (bool) { + /// @notice Compares two values based on the specified operation. + /// @param _a The first value to compare. + /// @param _b The second value to compare. + /// @param _op The operation to use for comparison. + /// @return Returns `true` if the comparison holds true. + function _compare(uint256 _a, uint256 _b, Op _op) internal pure returns (bool) { if (_op == Op.EQ) return _a == _b; if (_op == Op.NEQ) return _a != _b; if (_op == Op.GT) return _a > _b; @@ -287,10 +292,10 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { /// @notice Encodes rule indices into a uint240 value. /// @param startingRuleIndex The index of the starting rule to evaluate. - /// @param successRuleIndex The index of the rule to evaluate if the condition is true. - /// @param failureRuleIndex The index of the rule to evaluate if the condition is false. + /// @param successRuleIndex The index of the rule to evaluate if the evaluation of `startingRuleIndex` was true. + /// @param failureRuleIndex The index of the rule to evaluate if the evaluation of `startingRuleIndex` was false. /// @return The encoded value combining all three inputs. - function encodeRuleValue( + function encodeIfElse( uint256 startingRuleIndex, uint256 successRuleIndex, uint256 failureRuleIndex @@ -298,7 +303,17 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { return uint240(startingRuleIndex + (successRuleIndex << 32) + (failureRuleIndex << 64)); } - /// @notice Decodes rule indices into three uint32. + /// @notice Encodes two rule indexes into a uint240 value. Useful for logical operators such as `AND/OR/XOR` and others. + /// @param ruleIndex1 The first index to evaluate. + /// @param ruleIndex2 The second index to evaluate. + function encodeLogicalOperator( + uint256 ruleIndex1, + uint256 ruleIndex2 + ) public pure returns (uint240) { + return uint240(ruleIndex1 + (ruleIndex2 << 32)); + } + + /// @notice Decodes rule indices into three uint32. /// @param _x The value to decode. /// @return a The first 32-bit segment. /// @return b The second 32-bit segment. diff --git a/contracts/test/permission/condition/extensions/ruled-condition.ts b/contracts/test/permission/condition/extensions/ruled-condition.ts index fa51d9f2..6a7772fe 100644 --- a/contracts/test/permission/condition/extensions/ruled-condition.ts +++ b/contracts/test/permission/condition/extensions/ruled-condition.ts @@ -13,6 +13,7 @@ import { LOGIC_OP_RULE_ID, DUMMY_PERMISSION_ID, Op, + RULE_VALUE_RULE_ID, } from '../../../utils/condition/condition'; import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; @@ -20,7 +21,7 @@ import {expect} from 'chai'; import {ethers} from 'hardhat'; describe('RuledCondition', async () => { - it('it should be able to update the condition rules', async () => { + it('should be able to update the condition rules', async () => { const {conditionMock} = await loadFixture(fixture); await conditionMock.updateRules([ @@ -40,11 +41,20 @@ describe('RuledCondition', async () => { expect(rules[0].permissionId).to.equal(DUMMY_PERMISSION_ID); }); - it('it should be able to eval simple rule (evaluation is true)', async () => { + it('should be able to eval simple rule (evaluation is true)', async () => { const {deployer, daoMock, conditionMock, subConditionA} = await loadFixture( fixture ); + await expect( + conditionMock.isGranted( + daoMock.address, + deployer.address, + DUMMY_PERMISSION_ID, + '0x' + ) + ).to.be.reverted; + // configure a simple rule in the condition await conditionMock.updateRules([ { @@ -68,7 +78,7 @@ describe('RuledCondition', async () => { ).to.be.true; }); - it('it should be able to eval simple rule (evaluation is false)', async () => { + it('should be able to eval simple rule (evaluation is false)', async () => { const {deployer, daoMock, conditionMock, subConditionA} = await loadFixture( fixture ); @@ -93,7 +103,7 @@ describe('RuledCondition', async () => { ).to.be.false; }); - it('it should be able to eval complex rule (evaluation is true)', async () => { + it('should be able to eval complex rule (evaluation is true)', async () => { const { deployer, daoMock, @@ -128,7 +138,7 @@ describe('RuledCondition', async () => { ).to.be.true; }); - it('it should be able to eval complex rule (evaluation is false)', async () => { + it('should be able to eval complex rule (evaluation is false)', async () => { const { deployer, daoMock, @@ -162,7 +172,50 @@ describe('RuledCondition', async () => { ).to.be.false; }); - it('it should be able to eval rule that checks blockNumber', async () => { + it(`evaluates 'if/else' on sub-conditions and only returns true if at least one of them returns true`, async () => { + const {deployer, daoMock, subConditionA, subConditionB, conditionMock} = + await loadFixture(fixture); + + // checks the block number is bigger or equal than 1 + await conditionMock.updateRules( + ifAelseB(conditionMock, subConditionA, subConditionB) + ); + + // since both sub-conditions return false, our condition also returns false. + expect( + await conditionMock.isGranted( + daoMock.address, + deployer.address, + DUMMY_PERMISSION_ID, + '0x' + ) + ).to.be.false; + + // This now must return true because `if` condition is true. + await subConditionA.setAnswer(true); + expect( + await conditionMock.isGranted( + daoMock.address, + deployer.address, + DUMMY_PERMISSION_ID, + '0x' + ) + ).to.be.true; + + // This now must return true because `else` condition is true. + await subConditionA.setAnswer(false); + await subConditionB.setAnswer(true); + expect( + await conditionMock.isGranted( + daoMock.address, + deployer.address, + DUMMY_PERMISSION_ID, + '0x' + ) + ).to.be.true; + }); + + it('should be able to eval rule that checks blockNumber', async () => { const {deployer, daoMock, conditionMock} = await loadFixture(fixture); // checks the block number is bigger or equal than 1 @@ -202,7 +255,7 @@ describe('RuledCondition', async () => { ).to.be.false; }); - it('it should be able to eval rule that checks timestamp', async () => { + it('should be able to eval rule that checks timestamp', async () => { const {deployer, daoMock, conditionMock} = await loadFixture(fixture); // checks the timestamp is bigger or equal than 1 @@ -252,6 +305,39 @@ type FixtureResult = { subConditionC: PermissionConditionMock; }; +function ifAelseB( + conditionMock: RuledConditionMock, + subConditionA: PermissionConditionMock, + subConditionB: PermissionConditionMock +) { + return [ + { + id: LOGIC_OP_RULE_ID, + op: Op.IF_ELSE, + value: conditionMock.encodeIfElse(1, 2, 3), + permissionId: DUMMY_PERMISSION_ID, + }, + { + id: CONDITION_RULE_ID, + op: Op.EQ, + value: subConditionA.address, + permissionId: DUMMY_PERMISSION_ID, + }, + { + id: RULE_VALUE_RULE_ID, + op: Op.RET, + value: 1, + permissionId: DUMMY_PERMISSION_ID, + }, + { + id: CONDITION_RULE_ID, + op: Op.EQ, + value: subConditionB.address, + permissionId: DUMMY_PERMISSION_ID, + }, + ]; +} + function C_or_B_and_A_Rule( conditionMock: RuledConditionMock, subConditionA: PermissionConditionMock, @@ -262,7 +348,7 @@ function C_or_B_and_A_Rule( { id: LOGIC_OP_RULE_ID, op: Op.OR, - value: conditionMock.encodeRuleValue(1, 2, 3), // indx 1 and idx 2 encoded + value: conditionMock.encodeLogicalOperator(1, 2), // indx 1 and indx 2 encoded permissionId: DUMMY_PERMISSION_ID, }, { @@ -274,7 +360,7 @@ function C_or_B_and_A_Rule( { id: LOGIC_OP_RULE_ID, op: Op.AND, - value: conditionMock.encodeRuleValue(3, 4, 5), // indx 3 and idx 4 encoded + value: conditionMock.encodeLogicalOperator(3, 4), // indx 3 and indx 4 encoded permissionId: DUMMY_PERMISSION_ID, }, {