-
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
isDatasetCompatibleWithDeal
public view function inIexecPoco1Facet
- Moved
Matching
struct 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 <175728472+Copilot@users.noreply.github.com>
We should check if we want to return it false (Pierre) or if we want revert( JB) |
Co-authored-by: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com>
…and IexecPoco1Facet
…acets in upgrade script
Co-authored-by: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com>
Co-authored-by: gfournieriExec <100280020+gfournieriExec@users.noreply.github.com>
…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 <26070035+zguesmi@users.noreply.github.com>
…in IexecPoco1Facet
Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com>
Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com>
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), |
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, |
Copilot uses AI. Check for mistakes.
SignatureVerifier, | ||
IexecPocoCommon | ||
{ | ||
contract IexecPoco1Facet is IexecPoco1, FacetBase, IexecEscrow, SignatureVerifier, IexecPocoCommon { |
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 | |
{ |
Copilot uses AI. Check for mistakes.
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 <26070035+zguesmi@users.noreply.github.com>
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 theMatching
struct fromIexecPoco1Facet
and moved it to theIexecPoco1
interface.To test
ARBITRUM_FORK=true npx hardhat node --no-deploy