Revocation registry #108
Revocation registry #108
Conversation
|
Hello @guilherme-funchal ! |
guilherme-funchal
force-pushed
the
Revocation_SERPRO
branch
from
7ed0201 to
3f540d2
Compare
|
Hello Renata, I think I solved the problem. Could you please check if everything is correct? |
|
Thank you, we started reviewing the code. |
|
I put a developer from my team to look at the problem. |
guilherme-funchal
force-pushed
the
Revocation_SERPRO
branch
from
f047c2e to
c147188
Compare
| mapping(bytes32 id => RevocationRegistryDefinitionRecord revocationRegistryDefinitionRecord) private _revRegDefs; | ||
|
|
||
| /** | ||
| * Checks that the schema exist |
There was a problem hiding this comment.
Typo in the comment. Must be credential definition instead schema
| _revRegDefs[_revRegDefId].metadata.currentAccumulator.length != 0 && | ||
| keccak256(abi.encodePacked(_revRegDefs[_revRegDefId].metadata.currentAccumulator)) != | ||
| keccak256(abi.encodePacked(initialRevRegEntry.prevAccumulator)) | ||
| ) revert AccumulatorMismatch(initialRevRegEntry.prevAccumulator); |
There was a problem hiding this comment.
From the gas point of view, I think it will be more efficient just to compare each element in the array rather doing hashing.
if (a.length != b.length) {
return false;
}
for (uint256 i = 0; i < a.length; i++) {
if (a[i] != b[i]) {
return false;
}
}
return true;
```
| /** | ||
| * Checks wether Credential Definition Exists | ||
| */ | ||
| modifier _credDefExists(bytes32 credDefId) { |
There was a problem hiding this comment.
this modifier looks like a duplicate of _credentialDefinitionExists
| ) external override { | ||
| _createRevocationRegistryEntry(identity, msg.sender, revRegDefId, issuerId, revRegEntry); | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe createRevocationRegistryEntrymethod also require adding endorsing (Signed) version.
| if ( | ||
| keccak256(abi.encodePacked(_revRegDefs[revRegDefId].metadata.issuerId)) != | ||
| keccak256(abi.encodePacked(issuerId)) | ||
| ) revert NotRevocationRegistryDefinitionIssuer(issuerId); |
There was a problem hiding this comment.
You can just use StringUtils.equals method to compare issuerId
| revocationRegistryEntryStruct | ||
| ) | ||
|
|
||
| console.log(`Revocation Registry Entry created for Revocation Registry Definition id ${revocationRegistryId}. Receipt: ${JSON.stringify(receipt)}`) |
There was a problem hiding this comment.
Please add registry resolving step
| storage[slots['1']] = padLeft(data.universalDidResolverAddress, 64) | ||
| // address of Role control contact stored in slot 3 | ||
| storage[slots['2']] = padLeft(data.roleControlContractAddress, 64) | ||
| // address of schema registry contact stored in slot 2 |
There was a problem hiding this comment.
typo: schema -> credentialDefinition
| // Set all `issuer` indexes to 0 (not revoked) | ||
| for issue in delta.issued { | ||
| revocation_list[issue as usize] = RevocationState::Active as u32 | ||
| } |
There was a problem hiding this comment.
I think this loop does nothing because let mut revocation_list: Vec<u32> = vec![0; rev_reg_def.value.max_cred_num.try_into().unwrap()]; already create an array where all elements are 0
| return Ok(None); | ||
| } | ||
|
|
||
| //TODO: sets? vectors? |
There was a problem hiding this comment.
Agree with using hashset
| Some(previous_delta) => { | ||
| previous_delta.validate(revocation_registry_status_list.len() as u32 - 1)?; | ||
| // Check whether the revocationStatusList entry is not included in the previous delta issued indices | ||
| for (index, entry) in (0u32..).zip(revocation_registry_status_list.iter()) { |
There was a problem hiding this comment.
It seems, can be replaced with: revocation_registry_status_list.iter().enumerate()
|
Thank @guilherme-funchal for the great contribution. |
|
Will you make the corrections or do you want me to try to do them? |
|
@guilherme-funchal It would be better if you or your team process them. |
|
We have already started reviewing your observations and will submit them as soon as possible. |
Revocation support Signed-off-by: Guilherme Funchal da Silva <[email protected]>
Signed-off-by: Guilherme Funchal da Silva <[email protected]>
guilherme-funchal
force-pushed
the
Revocation_SERPRO
branch
from
0373995 to
311847f
Compare
|
I just saw some more of your comments that we haven't addressed yet, if you can wait for the new commit. |
As part of our POC using the indy-besu VDR and credo-ts (with the credo-rest extension), we needed and therefore are trying to develop a revocation solution, as it seems it is missing in the current version of this repository.
We have only started testing it, and probably there will be modifications and improvements, but if you could take the time to give us your feedback and suggestions, it would surely be appreciated!
We tried to model our implementation having the credo-ts implementation of anoncreds in mind, and the credo-vdr interaction was based on on the indy-vdr package. We have also attempted to keep within the standards used in this repository.
Our initial design idea consists of a smart contract (RevocationRegistry.sol) that stores, for a given RevocationRegistryDefinitionId, an object following this specification. It also stores onchain a reference to the issuerId and the current accumulator for comparison when trying to update the registry.
The Revocation Status List would be built by blockchain events (RevocationRegistryEntry) emitted for a given RevocationRegistryId. The RevocationRegistryEntry contains the indexes of newly issued/revoked credentials, the new accumulator, the previous accumulator for comparison, and a timestamp.
For building the complete RevocationStatusList, one would fetch and accumulate all events up to a given timestamp, this is done offchain.
We updated the genesis generation scripts, the 'flow-with-did-ethr' demo and created some basic tests for now.
We are also working on the Rust VDR implementation, including the wasm integration with credo, with some superficial tests for now.