feat: receive approval generic #323
Conversation
…proved safety and readability
…ching functionality
…g in IexecEscrowTokenFacet
…rns and parameter details
…es and detailed documentation
…otes on order matching and deal cost calculation
…tion selector and enhance documentation
…on with improved error handling
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the receiveApproval function to support a generalized operation execution pattern. Instead of being hardcoded to only handle order matching, it now uses function selectors to dispatch different operations, making the implementation more extensible.
Key changes:
- Refactored
receiveApprovalto extract function selectors from calldata and dispatch to operation-specific validators - Updated
encodeOrdersto include thematchOrdersfunction selector in the encoded data - Added validation logic separation with
_validateMatchOrdersand execution logic in_executeOperation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| utils/odb-tools.ts | Updated encodeOrders to prepend the matchOrders function selector to encoded parameters |
| contracts/facets/IexecEscrowTokenFacet.sol | Refactored receiveApproval to use generalized operation dispatch pattern with selector-based routing |
| test/byContract/IexecEscrow/IexecEscrowToken.receiveApproval.test.ts | Updated test descriptions and added test for unsupported operation selector validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
utils/odb-tools.ts
Outdated
| const iface = new ethers.Interface([ | ||
| `function matchOrders(${appOrderType} apporder, ${datasetOrderType} datasetorder, ${workerpoolOrderType} workerpoolorder, ${requestOrderType} requestorder)`, | ||
| ]); | ||
| const matchOrdersSelector = iface.getFunction('matchOrders')!.selector; |
There was a problem hiding this comment.
Using the non-null assertion operator (!) is unsafe here. If getFunction returns undefined (e.g., due to a typo in the function name), this will throw a runtime error. Consider adding an explicit null check or using optional chaining with a fallback error message.
| const matchOrdersSelector = iface.getFunction('matchOrders')!.selector; | |
| const matchOrdersFn = iface.getFunction('matchOrders'); | |
| if (!matchOrdersFn) { | |
| throw new Error("Function 'matchOrders' not found in interface"); | |
| } | |
| const matchOrdersSelector = matchOrdersFn.selector; |
| * @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 { |
There was a problem hiding this comment.
The function is marked as pure but it performs abi.decode on calldata. While this is technically allowed in newer Solidity versions, the function should be marked as view for clarity and consistency, as it reads from calldata which is external data.
| function _validateMatchOrders(address sender, bytes calldata data) internal pure { | |
| function _validateMatchOrders(address sender, bytes calldata data) internal view { |
There was a problem hiding this comment.
Function state mutability can be restricted to puresolidity(2018)
| (, , , IexecLibOrders_v5.RequestOrder memory requestorder) = abi.decode( | ||
| data[4:], |
There was a problem hiding this comment.
The comment on line 179-181 states 'We only need to check requestorder.requester', but this approach still decodes all four order structs (consuming gas for all of them). Consider adding a note about the gas trade-off or exploring if there's a more efficient way to decode only the request order if optimization is important.
gfournierPro
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/solidity-v8 #323 +/- ##
=====================================================
+ Coverage 96.36% 96.37% +0.01%
=====================================================
Files 33 33
Lines 1127 1132 +5
Branches 227 228 +1
=====================================================
+ Hits 1086 1091 +5
Misses 41 41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gas cost from 852719 -> 849967 = - 2 752
The comment on line 179-181 states 'We only need to check requestorder.requester', but this approach still decodes all four order structs (consuming gas for all of them). Consider adding a note about the gas trade-off or exploring if there's a more efficient way to decode only the request order if optimization is important.
when we will support sponsor MatchOrder the check won't be needed