-
Notifications
You must be signed in to change notification settings - Fork 14
feat: implement dataset compatibility check with a deal in IexecPoco1Facet
#266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement dataset compatibility check with a deal in IexecPoco1Facet
#266
Conversation
Added a new public view function `isDatasetCompatibleWithDeal` to verify the compatibility of a dataset order with a deal. This function includes multiple checks such as deal existence, dataset order owner signature, and restrictions on app, workerpool, and requester. Removed the `Matching` struct from `IexecPoco1Facet` and moved it to the `IexecPoco1` interface.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
+ Coverage 83.66% 84.83% +1.17%
==========================================
Files 38 37 -1
Lines 1218 1240 +22
Branches 227 235 +8
==========================================
+ Hits 1019 1052 +33
+ Misses 199 188 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ility Removed redundant comment in `IexecPoco1Facet` and added comprehensive tests for the new `isDatasetCompatibleWithDeal` function. The tests cover various scenarios including compatibility checks, invalid signatures, and restrictions on dataset orders.
Introduced a new script to deploy and update the IexecPocoAccessorsFacet and IexecPoco1Facet. The script handles the deployment of new facets, removal of old facets, and updates the diamond proxy accordingly. It includes checks for required deployment options and logs the process for better traceability.
Refactored tests for the `isDatasetCompatibleWithDeal` function to improve clarity and coverage. Removed the creation of an incompatible dataset order and added checks for non-existent deals and deals that already have datasets. Updated test cases to ensure accurate validation of dataset order compatibility, including scenarios for incompatible app restrictions and tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new dataset compatibility check feature by adding the isDatasetCompatibleWithDeal function to verify if a dataset order can be used with an existing deal. The function validates multiple compatibility criteria including deal existence, dataset order signatures, and restriction matching.
Key changes:
- Added
isDatasetCompatibleWithDealpublic view function inIexecPoco1Facet - Moved
Matchingstruct from facet implementation to interface for better organization - Updated deployment script to handle both IexecPocoAccessorsFacet and IexecPoco1Facet upgrades
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| contracts/facets/IexecPoco1Facet.sol | Adds isDatasetCompatibleWithDeal function and removes Matching struct |
| contracts/interfaces/IexecPoco1.v8.sol | Adds Matching struct and isDatasetCompatibleWithDeal function signature |
| test/byContract/IexecPoco/IexecPoco1.test.ts | Comprehensive test suite for the new compatibility function |
| scripts/upgrades/deploy-and-update-some-facet.ts | Updates deployment script to handle IexecPoco1Facet upgrade |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
We should check if we want to return it false (Pierre) or if we want revert( JB) |
Co-authored-by: gfournieriExec <[email protected]>
Co-authored-by: gfournieriExec <[email protected]>
…e and update tests
|
Even if the spec changes, I think we should make those changes in another PR and iterate from there. |
Co-authored-by: Zied Guesmi <[email protected]>
…in IexecPoco1Facet
Co-authored-by: Zied Guesmi <[email protected]>
Co-authored-by: Zied Guesmi <[email protected]>
IexecPoco1Facet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.log('Deploying new IexecPocoAccessorsFacet...'); | ||
| const iexecPocoAccessorsFacetFactory = new IexecPocoAccessorsFacet__factory(iexecLibOrders); | ||
| const iexecPocoAccessorsFacet = await factoryDeployer.deployContract( | ||
| new IexecPocoAccessorsFacet__factory(iexecLibOrders), |
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IexecPocoAccessorsFacet__factory is instantiated twice - once assigned to iexecPocoAccessorsFacetFactory and again in the deployContract call. Use the existing iexecPocoAccessorsFacetFactory variable instead of creating a new instance.
| new IexecPocoAccessorsFacet__factory(iexecLibOrders), | |
| iexecPocoAccessorsFacetFactory, |
| SignatureVerifier, | ||
| IexecPocoCommon | ||
| { | ||
| contract IexecPoco1Facet is IexecPoco1, FacetBase, IexecEscrow, SignatureVerifier, IexecPocoCommon { |
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The contract declaration formatting was changed from multi-line to single-line. While this works, the multi-line format is more readable for contracts with multiple inheritance, especially when the line becomes long.
| contract IexecPoco1Facet is IexecPoco1, FacetBase, IexecEscrow, SignatureVerifier, IexecPocoCommon { | |
| contract IexecPoco1Facet is | |
| IexecPoco1, | |
| FacetBase, | |
| IexecEscrow, | |
| SignatureVerifier, | |
| IexecPocoCommon | |
| { |
zguesmi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
Some minor comments but all good.
Co-authored-by: Zied Guesmi <[email protected]>
Added a new public view function
isDatasetCompatibleWithDealto verify the compatibility of a dataset order with a deal. This function includes multiple checks such as deal existence, dataset order owner signature, and restrictions on app, workerpool, and requester. Removed theMatchingstruct fromIexecPoco1Facetand moved it to theIexecPoco1interface.To test
ARBITRUM_FORK=true npx hardhat node --no-deploy