feat: Enable deposit and match orders in a single tx #316
Conversation
…proved safety and readability
…ching functionality
…g in IexecEscrowTokenFacet
There was a problem hiding this comment.
Pull Request Overview
This PR implements atomic approve+deposit+match functionality for the IexecEscrow contract, allowing users to approve tokens, deposit them, and match orders in a single transaction through the receiveApproval callback mechanism. This provides a better UX by reducing gas costs and transaction complexity.
Key Changes:
- Extended
receiveApprovalto support optional order matching via encoded calldata - Added comprehensive test coverage for the new atomic operation flow
- Migrated several contracts from Solidity 0.6 to 0.8 (SafeMath removal)
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
utils/odb-tools.ts |
Added encodeOrders function to ABI-encode orders for callback data |
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts |
Comprehensive test suite for atomic approve+deposit+match flow |
contracts/facets/IexecEscrowTokenFacet.sol |
Enhanced receiveApproval with order matching and migrated to Solidity 0.8 |
contracts/interfaces/IexecEscrowToken.sol |
Added ApprovalReceivedAndMatched event definition |
contracts/interfaces/IexecERC20Common.sol |
Moved Transfer and Approval event definitions from IexecERC20 |
contracts/interfaces/IexecERC20.sol |
Removed duplicate event definitions |
contracts/facets/IexecERC20Core.sol |
Migrated to Solidity 0.8, replaced SafeMath with native operators |
contracts/facets/IexecERC20Facet.sol |
Migrated to Solidity 0.8, added underflow checks |
contracts/facets/IexecEscrowNativeFacet.sol |
Migrated to Solidity 0.8, replaced SafeMath with native operators |
contracts/facets/FacetBase.v8.sol |
Added onlyOwner modifier and owner() helper |
package-lock.json |
Removed duplicate hardhat dependency entry |
abis/**/*.json |
Updated ABI files to reflect event and interface changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
gfournieriExec
changed the base branch from
chore/solidity-v8
to
fix/format-sc
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Token Spender (endpoint for approveAndCallback calls to the proxy) | ||
| /*************************************************************************** | ||
| * Token Spender: Atomic Approve+Deposit+Match * |
There was a problem hiding this comment.
| * Token Spender: Atomic Approve+Deposit+Match * | |
| * Token Spender: Atomic Deposit+Match if used with RLC.approveAndCall * |
There was a problem hiding this comment.
There was a problem hiding this comment.
Up here. The comment is not correct we only have 2 of them, deposit and match.
|
|
||
| /** | ||
| * @notice Receives approval and optionally matches orders in one transaction | ||
| * @dev This is the magic function that enables approve+deposit+match atomicity |
There was a problem hiding this comment.
| * @dev This is the magic function that enables approve+deposit+match atomicity |
There was a problem hiding this comment.
| * bytes memory data = abi.encode(appOrder, datasetOrder, workerpoolOrder, requestOrder); | ||
| * | ||
| * // One transaction does it all | ||
| * RLC.approveAndCall(iexecProxy, dealCost, data); |
There was a problem hiding this comment.
| * RLC.approveAndCall(iexecProxy, dealCost, data); | |
| * RLC(token).approveAndCall(iexecProxy, dealCost, data); |
There was a problem hiding this comment.
| if (data.length > 0) { | ||
| dealId = _matchOrdersAfterDeposit(sender, data); | ||
| } | ||
| emit ApprovalReceivedAndMatched(sender, amount, dealId); |
There was a problem hiding this comment.
I don't think this is needed because we already have enough logs.
There was a problem hiding this comment.
| _deposit(sender, amount); | ||
| _mint(sender, amount); | ||
| bytes32 dealId = bytes32(0); | ||
| if (data.length > 0) { |
There was a problem hiding this comment.
We have two choices here:
data = encoded orders: the simpler option but limited for future updatesdata = matchOrders selector + encoded orders: this version seems a bit more complicated but it allows for easier future evolutions. We can encode the hole call and check the selector, if it ismatchOrdersthan do the call, if not revert. This way when we want to add a new operation the SDK needs just to encode the new call and we'll need to allow it. The behavior does not change from only encoding call arguments to encoding the full call.
if (data.length == 0) {
return true;
}
if (getSelector(data) == matchOrders.selector) {
matchOrders
return true;
}
revert;
| * @param data ABI-encoded orders | ||
| * @return dealId The deal ID of the matched deal | ||
| */ | ||
| function _matchOrdersAfterDeposit( |
There was a problem hiding this comment.
| function _matchOrdersAfterDeposit( | |
| function _decodeDataAndMatchOrders( |
There was a problem hiding this comment.
|
|
||
| // Call matchOrders on the IexecPoco1 facet through the diamond | ||
| // Using delegatecall pattern via address(this) | ||
| (bool success, bytes memory result) = address(this).call( |
There was a problem hiding this comment.
We need to check which is safer:
- call:
msg.senderis the PoCo - delegatecall:
msg.senderis the RLC
Option 2 is clearly safer but we need to check if it's a problem for the matching.
There was a problem hiding this comment.
| * @param requestOrder Request order struct | ||
| * @returns ABI-encoded orders | ||
| */ | ||
| export function encodeOrders( |
There was a problem hiding this comment.
We need to encode orders programatically so that we have an example of the client's code.
There was a problem hiding this comment.
i don't understand this com
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/solidity-v8 #316 +/- ##
=====================================================
+ Coverage 96.32% 96.37% +0.04%
=====================================================
Files 34 34
Lines 1117 1130 +13
Branches 209 227 +18
=====================================================
+ Hits 1076 1089 +13
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zguesmi
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…es and detailed documentation
…otes on order matching and deal cost calculation
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bytes32 dealId = bytes32(0); | ||
| if (data.length > 0) { | ||
| dealId = _decodeDataAndMatchOrders(sender, data); |
There was a problem hiding this comment.
The variable dealId is initialized to bytes32(0) but is never used after line 120. Consider removing this variable declaration entirely since the return value from _decodeDataAndMatchOrders is not used anywhere.
| bytes32 dealId = bytes32(0); | |
| if (data.length > 0) { | |
| dealId = _decodeDataAndMatchOrders(sender, data); | |
| if (data.length > 0) { | |
| _decodeDataAndMatchOrders(sender, data); |
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… in the Solidity API documentation
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async function signAndPrepareOrders(orders: IexecOrders): Promise<void> { | ||
| await signOrders(iexecWrapper.getDomain(), orders, ordersActors); | ||
| } | ||
|
|
||
| function encodeOrdersForCallback(orders: IexecOrders): string { | ||
| return encodeOrders(orders.app, orders.dataset, orders.workerpool, orders.requester); | ||
| } |
There was a problem hiding this comment.
Are those wrappers really necessary? We’re replacing one line with three.
|
|
||
| // Token Spender (endpoint for approveAndCallback calls to the proxy) | ||
| /*************************************************************************** | ||
| * Token Spender: Atomic Approve+Deposit+Match * |
There was a problem hiding this comment.
Up here. The comment is not correct we only have 2 of them, deposit and match.
| if (requestorder.requester != sender) revert("caller-must-be-requester"); | ||
|
|
||
| // Call matchOrders on the IexecPoco1 facet through the diamond | ||
| // Using delegatecall for safety: preserves msg.sender context |
There was a problem hiding this comment.
| // Using delegatecall for safety: preserves msg.sender context | |
| // Using delegatecall for safety: preserves msg.sender context (RLC address in this case) |
| // Decode the dealId from successful call | ||
| dealId = abi.decode(result, (bytes32)); | ||
|
|
||
| return dealId; |
There was a problem hiding this comment.
| // Decode the dealId from successful call | |
| dealId = abi.decode(result, (bytes32)); | |
| return dealId; |
| // Decode the dealId from successful call | ||
| dealId = abi.decode(result, (bytes32)); | ||
|
|
||
| return dealId; |
There was a problem hiding this comment.
| // Decode the dealId from successful call | |
| dealId = abi.decode(result, (bytes32)); | |
| return dealId; |
3 participants
No description provided.