-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add view assets functions #262
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
Conversation
… functionality and proxy updates
… in proxy upgrade script
…xecPocoAccessorsFacet
… mapping in deploy script
…pt for IexecPocoAccessorsFacet
…y for new facet integration
…loy-and-update-accessor-facet script
…tions in deploy-and-update-accessor-facet script
… deploy-and-update-accessor-facet script
…ccessors interface
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 adds new view functions to access asset information (datasets, apps, and workerpools) in the Iexec protocol. The implementation provides a standardized way to retrieve metadata about these assets through the main contract interface.
- Adds three new view functions:
viewDataset
,viewApp
, andviewWorkerpool
- Defines corresponding data structures (
DatasetInfo
,AppInfo
,WorkerpoolInfo
) in the core library - Creates interface definitions for the three asset types with their respective accessor methods
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
contracts/facets/IexecPocoAccessorsFacet.sol | Implements the three view functions that aggregate asset data by calling interface methods |
contracts/interfaces/IexecPocoAccessors.sol | Adds function signatures for the new view asset functions |
contracts/interfaces/IexecAccessors.sol | Adds the same function signatures to the main accessor interface |
contracts/libs/IexecLibCore_v5.sol | Defines data structures for DatasetInfo, AppInfo, and WorkerpoolInfo |
contracts/registries/datasets/IDataset.v8.sol | New interface defining dataset accessor methods |
contracts/registries/apps/IApp.v8.sol | New interface defining app accessor methods |
contracts/registries/workerpools/IWorkerpool.v8.sol | Updates workerpool interface to add missing view modifiers and new methods |
test/byContract/iexecPocoAccessors/IexecPocoAccessors.test.ts | Adds comprehensive tests for all three new view functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 83.66% 84.56% +0.90%
==========================================
Files 38 37 -1
Lines 1218 1218
Branches 227 227
==========================================
+ Hits 1019 1030 +11
+ Misses 199 188 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Clean Job
Somes questions/reflections
import {FacetBase} from "./FacetBase.v8.sol"; | ||
import {IexecLibCore_v5} from "../libs/IexecLibCore_v5.sol"; | ||
import {IexecLibOrders_v5} from "../libs/IexecLibOrders_v5.sol"; | ||
import {IDataset} from "../registries/datasets/IDataset.v8.sol"; |
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.
Should we indicate that this is version 8 in the file name?
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.
good question, matched what was already done with IWorkpool file can change it
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.
Let's keep them ISO yes. Otherwise we'll need to modify IexecPoco1Facet
and IexecPocoBoostFacet
.
struct DatasetInfo { | ||
address owner; | ||
string m_datasetName; | ||
bytes m_datasetMultiaddr; | ||
bytes32 m_datasetChecksum; | ||
} | ||
|
||
struct AppInfo { | ||
address owner; | ||
string m_appName; | ||
string m_appType; | ||
bytes m_appMultiaddr; | ||
bytes32 m_appChecksum; | ||
bytes m_appMREnclave; | ||
} | ||
|
||
struct WorkerpoolInfo { | ||
address owner; | ||
string m_workerpoolDescription; | ||
uint256 m_workerStakeRatioPolicy; | ||
uint256 m_schedulerRewardRatioPolicy; | ||
} |
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.
Another option could be to have those structs directly in the asset contract interface. I’m not sure which solution is better.
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.
could be an option but this would imply having v6 and v8 interfaces in asset contract interface
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.
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.
Ok to put them inside LibCore since viewDataset
is in the PoCo's code not in registries.
|
||
it('viewWorkerpool', async function () { | ||
const workerpoolInfo = await iexecPoco.viewWorkerpool(workerpoolAddress); | ||
expect(workerpoolInfo.owner).to.equal(scheduler.address); |
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 owner of the workerpool is the scheduler 🤔 ?
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.
Co-authored-by: Zied Guesmi <26070035+zguesmi@users.noreply.github.com>
No description provided.