adding workerregistry and workorderregistry for the proxy model #72
Conversation
Signed-off-by: Hadrien Croubois <[email protected]>
Amxx
requested review from
danintel,
EugeneYYY,
manju956,
manojgop,
Ram-srini and
TomBarnes
as code owners
| bytes32 _requesterID, | ||
| bytes memory _workOrderRequest, | ||
| address _verifier, | ||
| bytes32 _salt) |
There was a problem hiding this comment.
_verifier and _salt is not mentioned in EEA spec in section 6.2.1 Submitting a New Work Order. Could you add function comment with the details of parameters
There was a problem hiding this comment.
We have a PR on the specs that explains that
There was a problem hiding this comment.
Could you share the link of the PR for reference
There was a problem hiding this comment.
There was a problem hiding this comment.
I can't access this link. Seems it's a private repo
There was a problem hiding this comment.
Actually, adding "salt" here is for the uniqueness
There was a problem hiding this comment.
you need to request eea admin to get the access, I think Eugene can help you on that
There was a problem hiding this comment.
Sure. I'll check with @EugeneYYY . Existing spec has workOrderId as input parameter in workOrderSubmit function. So was workOrderId used for uniqueness instead of salt ?
function workOrderSubmit( bytes32 workOrderId, bytes32 workerId, bytes32 requesterId, string workOrderRequest) public returns uint32 errorCode
There was a problem hiding this comment.
The PR removes workOrderId has an input parameter. It is not clearly defined how this should be computed beforehand and it causes issues: what if the value provided is already used ? How to prevent race conditions ?
The solution (which is the PR) is for the requester to provide the parameters (and a random salt). The contract will then generate the workOrderId itself, which is very unlikely to collide.
Without the salt a colision would happen if a requester asks the same computation twice (same parameters) ... if we add a random salt generated on the requester's side we remove this issue.
There was a problem hiding this comment.
Why do we have a secret spec? Why cannot the spec be publicly accessible?
| wo.requesterID = _requesterID; | ||
| wo.request = _workOrderRequest; | ||
| wo.verifier = _verifier; | ||
|
|
There was a problem hiding this comment.
Shouldn't we persist the new workOrder in storage using m_workorders[workOrderID] = wo;
There was a problem hiding this comment.
That is already what's happening. Look at line 80!
I'm using storage, not memory so all changes to wo are 'saved'
There was a problem hiding this comment.
workOrderId is calculated inside this function. Section 6.2.1 in EEA spec mentions workOrderId as an input parameter for workOrderSubmit function
function workOrderSubmit( bytes32 workOrderId, bytes32 workerId, bytes32 requesterId, string workOrderRequest) public returns uint32 errorCode
There was a problem hiding this comment.
See my reply above about: workerorder should not be an input but produced by the smartcontract (need to avoid colisions)
| emit workOrderSubmitted( | ||
| workOrderID, | ||
| _workerID, | ||
| _requesterID); |
There was a problem hiding this comment.
I assume there are changes made to spec. Publicly released spec mentions workOrderNew event is emitted by workOrderSubmit .
event workOrderNew (bytes32 indexed workOrderId, bytes32 indexed workerId, bytes32 indexed requesterId, string workOrderRequest, uint32 errorCode, address senderAddress, byte4 version)
There was a problem hiding this comment.
Yes, part of the spec change was about improving name consistency
| { | ||
| uint256 constant public TIMEOUT = 1 days; | ||
|
|
||
| enum WorkOrderStatus |
There was a problem hiding this comment.
Work order status has pending and processing as per the spec -https://entethalliance.github.io/trusted-computing/spec.html#work-order-status-payload
Where are those status are captured?
There was a problem hiding this comment.
These status assume a communication/report from the worker. This is part of the direct model but not in the proxy model. Smart contracts only know about the current state of "consensus" and the next step it expects. Smart contracts cannot say anything about any ongoing work.
|
|
||
| event workOrderTimedout( | ||
| bytes32 indexed workOrderID); | ||
|
|
There was a problem hiding this comment.
EEA spec talk about 4 events for work order. 1. New Work Order Event 2. Work Order Done Event 3. Encryption Key Set Event 4. Encryption Key Start Event. Please use same name as in spec eg. newWorkOrderEvent, workOrderDoneEvnet, etc
Are you planning to add encryption ket set and encryption key start event?
| _requesterID, | ||
| _workOrderRequest, | ||
| _verifier, | ||
| _salt |
There was a problem hiding this comment.
Please add comments about salt parameter description.
| return 0; | ||
| } | ||
|
|
||
| function workOrderTimeout( |
There was a problem hiding this comment.
Please add the description about this function. This function is not there in EEA spec.
| WorkOrder storage wo = m_workorders[_workOrderID]; | ||
| return ( | ||
| wo.status, | ||
| wo.timestamp, |
There was a problem hiding this comment.
Please add description about timestamp and verifier parameters in the function comments.
| mapping(bytes32 => Worker) private m_workers; | ||
|
|
||
| // workerType → organizationId → appTypeId → WorkerID[] | ||
| mapping(uint256 => mapping(bytes32 => mapping(bytes32 => bytes32[]))) m_workersDB; |
There was a problem hiding this comment.
Why do we need this complex map structure? Can't we use one in simple map as it line 39?
Does it for lookup optimization?
There was a problem hiding this comment.
what key-value do you propose and how would you maintain it?
| uint256 newLookupTag, | ||
| bytes32[] memory ids | ||
| ) { | ||
| bytes32[] storage matchs = m_workersDB[workerType][organizationId][appTypeId]; |
There was a problem hiding this comment.
The Worker must match all input parameters (AND mode) to be included in the list and All input parameters are optional and can be provided in any combination to select Workers. Does it covers all the conditions of lookup function?
There was a problem hiding this comment.
I don't know what to tell you but to read the code. Also, optional input parameters are not a thing in solidity, so you need to be creative about it (cf line 64 to 78).
|
|
||
| pragma solidity ^0.5.0; | ||
|
|
||
| contract IERC1271 |
There was a problem hiding this comment.
Please add the description of IERC1271 in the comments.
|
|
||
| pragma solidity ^0.5.0; | ||
|
|
||
| contract IERC734 |
There was a problem hiding this comment.
Please add description about IERC734 in comments.
|
|
||
| pragma solidity ^0.5.0; | ||
|
|
||
| library Set |
There was a problem hiding this comment.
Where is Set solidity module being used?
| // WorkerID → Worker | ||
| mapping(bytes32 => Worker) private m_workers; | ||
|
|
||
| // workerType → organizationId → appTypeId → WorkerID[] |
There was a problem hiding this comment.
s/appTypeId/appTypeIds/
(I see no appTypeId here, only appTypeIds)
| uint256 public constant MANAGEMENT_KEY = 1; | ||
| // 2: ACTION keys, which perform actions in this identities name (signing, logins, transactions, etc.) | ||
| uint256 public constant ACTION_KEY = 2; | ||
| // 3: CLAIM signer keys, used to sign claims on other identities which need to be revokable. |
|
Apparently this PR was abondonded and a new PR filed, PR #123 |
Amxx
requested review from
dcmiddle,
karthikamurthy,
pankajgoyal2 and
SriharshaSubbakrishna
as code owners
Signed-off-by: Hadrien Croubois [email protected]