diff --git a/contracts/facets/IexecEscrowTokenFacet.sol b/contracts/facets/IexecEscrowTokenFacet.sol index 3e801f29..be49e28c 100644 --- a/contracts/facets/IexecEscrowTokenFacet.sol +++ b/contracts/facets/IexecEscrowTokenFacet.sol @@ -70,25 +70,33 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 ***************************************************************************/ /** - * @notice Receives approval, deposit and optionally matches orders in one transaction + * @notice Receives approval, deposit and optionally executes an operation in one transaction * * Usage patterns: * 1. Simple deposit: RLC.approveAndCall(escrow, amount, "") - * 2. Deposit + match: RLC.approveAndCall(escrow, amount, encodedOrders) + * 2. Deposit + operation: RLC.approveAndCall(escrow, amount, encodedOperation) * - * The `data` parameter should be ABI-encoded orders if matching is desired: - * abi.encode(appOrder, datasetOrder, workerpoolOrder, requestOrder) + * The `data` parameter should include a function selector (first 4 bytes) to identify + * the operation, followed by ABI-encoded parameters. Supported operations: + * - matchOrders: Validates sender is requester, then matches orders * - * @dev Important notes: - * - Match orders sponsoring is NOT supported. The requester (sender) always pays for the deal. - * - Clients must compute the exact deal cost and deposit the right amount for the deal to be matched. + * @dev Implementation details: + * - Deposits tokens first, then executes the operation if data is provided + * - Extracts function selector from data to determine which operation + * - Each operation has a validator (_validateMatchOrders, etc.) for preconditions + * - After validation, _executeOperation performs the delegatecall + * - Error handling is generalized: bubbles up revert reasons or returns 'operation-failed' + * - Future operations can be added by implementing a validator and adding a selector case + * + * @dev matchOrders specific notes: + * - Sponsoring is NOT supported. The requester (sender) always pays for the deal. + * - Clients must compute the exact deal cost and deposit the right amount. * The deal cost = (appPrice + datasetPrice + workerpoolPrice) * volume. - * - If insufficient funds are deposited, the match will fail. * - * @param sender The address that approved tokens (must be requester if matching) + * @param sender The address that approved tokens * @param amount Amount of tokens approved and to be deposited * @param token Address of the token (must be RLC) - * @param data Optional: ABI-encoded orders for matching + * @param data Optional: Function selector + ABI-encoded parameters for operation * @return success True if operation succeeded * * @@ -97,10 +105,16 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 * // Compute deal cost * uint256 dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; * - * // Encode orders - * bytes memory data = abi.encode(appOrder, datasetOrder, workerpoolOrder, requestOrder); + * // Encode matchOrders operation with selector + * bytes memory data = abi.encodeWithSelector( + * IexecPoco1.matchOrders.selector, + * appOrder, + * datasetOrder, + * workerpoolOrder, + * requestOrder + * ); * - * // One transaction does it all + * // One transaction does it all: approve, deposit, and match * RLC(token).approveAndCall(iexecProxy, dealCost, data); * ``` */ @@ -114,54 +128,28 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 require(token == address($.m_baseToken), "wrong-token"); _deposit(sender, amount); _mint(sender, amount); + if (data.length > 0) { - _decodeDataAndMatchOrders(sender, data); + _executeOperation(sender, data); } return true; } - /****************************************************************************** - * Token Spender: Atomic Deposit+Match if used with RLC.approveAndCall * - *****************************************************************************/ - - /** - * @dev Internal function to match orders after deposit - * @param sender The user who deposited (must be the requester) - * @param data ABI-encoded orders - */ - function _decodeDataAndMatchOrders(address sender, bytes calldata data) internal { - // Decode the orders from calldata - ( - IexecLibOrders_v5.AppOrder memory apporder, - IexecLibOrders_v5.DatasetOrder memory datasetorder, - IexecLibOrders_v5.WorkerpoolOrder memory workerpoolorder, - IexecLibOrders_v5.RequestOrder memory requestorder - ) = abi.decode( - data, - ( - IexecLibOrders_v5.AppOrder, - IexecLibOrders_v5.DatasetOrder, - IexecLibOrders_v5.WorkerpoolOrder, - IexecLibOrders_v5.RequestOrder - ) - ); + function _executeOperation(address sender, bytes calldata data) internal { + // Extract the function selector (first 4 bytes) + bytes4 selector = bytes4(data[:4]); - // Validate that sender is the requester - if (requestorder.requester != sender) revert("caller-must-be-requester"); + // Validate operation-specific preconditions before execution + if (selector == IexecPoco1.matchOrders.selector) { + _validateMatchOrders(sender, data); + } else { + revert("unsupported-operation"); + } - // Call matchOrders on the IexecPoco1 facet through the diamond - // Using delegatecall for safety: preserves msg.sender context (RLC address in this case) - // Note: matchOrders doesn't use msg.sender, but delegatecall is safer - // in case the implementation changes in the future - (bool success, bytes memory result) = address(this).delegatecall( - abi.encodeWithSelector( - IexecPoco1.matchOrders.selector, - apporder, - datasetorder, - workerpoolorder, - requestorder - ) - ); + // Execute the operation via delegatecall + // This preserves msg.sender context and allows the operation to access + // the diamond's storage and functions + (bool success, bytes memory result) = address(this).delegatecall(data); // Handle failure and bubble up revert reason if (!success) { @@ -172,11 +160,39 @@ contract IexecEscrowTokenFacet is IexecEscrowToken, IexecTokenSpender, IexecERC2 revert(add(result, 32), returndata_size) } } else { - revert("receive-approval-failed"); + revert("operation-failed"); } } } + /****************************************************************************** + * Token Spender: Atomic Deposit+Match if used with RLC.approveAndCall * + *****************************************************************************/ + + /** + * @dev Validates matchOrders preconditions + * @param sender The user who deposited (must be the requester) + * @param data ABI-encoded matchOrders call with orders + */ + function _validateMatchOrders(address sender, bytes calldata data) internal pure { + // Decode only the request order to validate the requester + // Full decoding: (AppOrder, DatasetOrder, WorkerpoolOrder, RequestOrder) + // We only need to check requestorder.requester + (, , , IexecLibOrders_v5.RequestOrder memory requestorder) = abi.decode( + data[4:], + ( + IexecLibOrders_v5.AppOrder, + IexecLibOrders_v5.DatasetOrder, + IexecLibOrders_v5.WorkerpoolOrder, + IexecLibOrders_v5.RequestOrder + ) + ); + + // Validate that sender is the requester + // This ensures the caller is authorized to create this deal + if (requestorder.requester != sender) revert("caller-must-be-requester"); + } + function _deposit(address from, uint256 amount) internal { PocoStorageLib.PocoStorage storage $ = PocoStorageLib.getPocoStorage(); require($.m_baseToken.transferFrom(from, address(this), amount), "failed-transferFrom"); diff --git a/docs/solidity/index.md b/docs/solidity/index.md index b10c3933..f0cc2c32 100644 --- a/docs/solidity/index.md +++ b/docs/solidity/index.md @@ -252,35 +252,43 @@ function recover() external returns (uint256) function receiveApproval(address sender, uint256 amount, address token, bytes data) external returns (bool) ``` -Receives approval, deposit and optionally matches orders in one transaction +Receives approval, deposit and optionally executes an operation in one transaction Usage patterns: 1. Simple deposit: RLC.approveAndCall(escrow, amount, "") -2. Deposit + match: RLC.approveAndCall(escrow, amount, encodedOrders) +2. Deposit + operation: RLC.approveAndCall(escrow, amount, encodedOperation) -The `data` parameter should be ABI-encoded orders if matching is desired: -abi.encode(appOrder, datasetOrder, workerpoolOrder, requestOrder) +The `data` parameter should include a function selector (first 4 bytes) to identify +the operation, followed by ABI-encoded parameters. Supported operations: +- matchOrders: Validates sender is requester, then matches orders -_Important notes: -- Match orders sponsoring is NOT supported. The requester (sender) always pays for the deal. -- Clients must compute the exact deal cost and deposit the right amount for the deal to be matched. - The deal cost = (appPrice + datasetPrice + workerpoolPrice) * volume. -- If insufficient funds are deposited, the match will fail._ +_Implementation details: +- Deposits tokens first, then executes the operation if data is provided +- Extracts function selector from data to determine which operation +- Each operation has a validator (_validateMatchOrders, etc.) for preconditions +- After validation, _executeOperation performs the delegatecall +- Error handling is generalized: bubbles up revert reasons or returns 'operation-failed' +- Future operations can be added by implementing a validator and adding a selector case + +matchOrders specific notes: +- Sponsoring is NOT supported. The requester (sender) always pays for the deal. +- Clients must compute the exact deal cost and deposit the right amount. + The deal cost = (appPrice + datasetPrice + workerpoolPrice) * volume._ #### Parameters | Name | Type | Description | | ---- | ---- | ----------- | -| sender | address | The address that approved tokens (must be requester if matching) | +| sender | address | The address that approved tokens | | amount | uint256 | Amount of tokens approved and to be deposited | | token | address | Address of the token (must be RLC) | -| data | bytes | Optional: ABI-encoded orders for matching | +| data | bytes | Optional: Function selector + ABI-encoded parameters for operation | #### Return Values | Name | Type | Description | | ---- | ---- | ----------- | -| [0] | bool | success True if operation succeeded @custom:example ```solidity // Compute deal cost uint256 dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; // Encode orders bytes memory data = abi.encode(appOrder, datasetOrder, workerpoolOrder, requestOrder); // One transaction does it all RLC(token).approveAndCall(iexecProxy, dealCost, data); ``` | +| [0] | bool | success True if operation succeeded @custom:example ```solidity // Compute deal cost uint256 dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; // Encode matchOrders operation with selector bytes memory data = abi.encodeWithSelector( IexecPoco1.matchOrders.selector, appOrder, datasetOrder, workerpoolOrder, requestOrder ); // One transaction does it all: approve, deposit, and match RLC(token).approveAndCall(iexecProxy, dealCost, data); ``` | ## IexecOrderManagementFacet diff --git a/test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts b/test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts index 7b7e7474..21bd3463 100644 --- a/test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts +++ b/test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts @@ -144,8 +144,8 @@ describe('IexecEscrowToken-receiveApproval', () => { }); }); - describe('receiveApproval with Order Matching', () => { - it('Should approve, deposit and match orders with all assets', async () => { + describe('receiveApproval with generalized operation execution', () => { + it('Should approve, deposit and execute matchOrders operation with all assets', async () => { const orders = buildOrders({ assets: ordersAssets, prices: ordersPrices, @@ -167,6 +167,7 @@ describe('IexecEscrowToken-receiveApproval', () => { const initialBalance = await iexecPoco.balanceOf(requester.address); const initialTotalSupply = await iexecPoco.totalSupply(); + // Encode the matchOrders operation with its selector const encodedOrders = encodeOrdersForCallback(orders); const tx = await rlcInstanceAsRequester.approveAndCall( @@ -344,6 +345,19 @@ describe('IexecEscrowToken-receiveApproval', () => { ).to.be.revertedWith('IexecEscrow: Transfer amount exceeds balance'); }); + it('Should revert with unsupported-operation for unknown function selector', async () => { + const dealCost = 1000n; + // Create calldata with an unsupported function selector (not matchOrders) + // Using a random selector that doesn't exist + const unsupportedSelector = '0x12345678'; + const dummyData = ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [42]); + const invalidData = unsupportedSelector + dummyData.slice(2); + + await expect( + rlcInstanceAsRequester.approveAndCall(proxyAddress, dealCost, invalidData), + ).to.be.revertedWith('unsupported-operation'); + }); + it('Should not match orders with invalid calldata', async () => { const dealCost = (appPrice + datasetPrice + workerpoolPrice) * volume; const invalidData = '0x1234'; // Too short to be valid @@ -446,8 +460,10 @@ describe('IexecEscrowToken-receiveApproval', () => { expect(await iexecPoco.frozenOf(requester.address)).to.equal(0n); }); - it('Should revert with receive-approval-failed when delegatecall fails silently', async () => { + it('Should revert with operation-failed when delegatecall fails silently', async () => { // Deploy the mock helper contract that fails silently + // This tests that the generalized _executeOperation properly handles + // delegatecall failures and bubbles up errors const mockFacet = await new ReceiveApprovalTestHelper__factory() .connect(iexecAdmin) .deploy() @@ -495,7 +511,7 @@ describe('IexecEscrowToken-receiveApproval', () => { depositAmount, encodedOrders, ); - await expect(tx).to.be.revertedWith('receive-approval-failed'); + await expect(tx).to.be.revertedWith('operation-failed'); // Restore original facet await diamondCut.diamondCut( diff --git a/utils/odb-tools.ts b/utils/odb-tools.ts index d620628b..2f50ca07 100644 --- a/utils/odb-tools.ts +++ b/utils/odb-tools.ts @@ -3,6 +3,7 @@ import { TypedDataDomain, TypedDataEncoder, TypedDataField, ethers } from 'ethers'; import hre from 'hardhat'; +import { IexecPoco1Facet__factory } from '../typechain'; interface WalletInfo { privateKey?: string; @@ -169,13 +170,18 @@ export function hashStruct( } /** - * Encode orders for callback data in receiveApproval. - * Uses typechain-generated struct definitions to ensure type consistency. + * Encode orders with matchOrders selector for receiveApproval callback. + * + * The encoded data includes the function selector as the first 4 bytes, which allows + * the generalized receiveApproval implementation to: + * 1. Extract the selector to identify the operation (matchOrders in this case) + * 2. Call the appropriate validator (_validateMatchOrders for permission checks) + * * @param appOrder App order struct * @param datasetOrder Dataset order struct * @param workerpoolOrder Workerpool order struct * @param requestOrder Request order struct - * @returns ABI-encoded orders + * @returns ABI-encoded calldata with matchOrders selector + encoded order structs */ export function encodeOrders( appOrder: Record, @@ -195,8 +201,13 @@ export function encodeOrders( const requestOrderType = 'tuple(address app, uint256 appmaxprice, address dataset, uint256 datasetmaxprice, address workerpool, uint256 workerpoolmaxprice, address requester, uint256 volume, bytes32 tag, uint256 category, uint256 trust, address beneficiary, address callback, string params, bytes32 salt, bytes sign)'; - return ethers.AbiCoder.defaultAbiCoder().encode( + // Encode the function parameters (without selector) + const encodedParams = ethers.AbiCoder.defaultAbiCoder().encode( [appOrderType, datasetOrderType, workerpoolOrderType, requestOrderType], [appOrder, datasetOrder, workerpoolOrder, requestOrder], ); + const matchOrdersSelector = + IexecPoco1Facet__factory.createInterface().getFunction('matchOrders')!.selector; + + return matchOrdersSelector + encodedParams.slice(2); }