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. |
Signed-off-by: Guilherme Funchal da Silva <guilherme.funchal@gmail.com>
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. |
|
Hello @flaviocgarcia78 ,
|
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
flaviocgarcia78
force-pushed
the
Revocation_SERPRO
branch
2 times, most recently
from
70ec1f5 to
39f268d
Compare
|
Hello @flaviocgarcia78 , |
Hello Renata, we are reviewing the commits to address your comments. I will let you know when they are OK. |
|
Hello @Toktar @Artemkaaas , I commited a new refactor of the previous code, checking every test to run smoothly, as well as the python demo using uniffi. One thing that I couldn't get working was the wasm demo, but I saw that there are some other errors on wasm demo, so I would suggest that we could fix that in another PR. I believe that I could cover all the comments, but a review should make sure of it. |
|
Hi @Toktar, is there any adjustment needed? We have implemented the integration with ACA-PY for this feature and would like to include it in the community package Pull Request refence. |
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Thiago Romano <thiagoromano.s@gmail.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
…docs Signed-off-by: artem.ivanov <artem.ivanov@dsr-corporation.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Thiago Romano <thiagoromano.s@gmail.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
…ignore) Signed-off-by: Thiago Romano <thiagoromano.s@gmail.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Thiago Romano <thiagoromano.s@gmail.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Thiago Romano <thiagoromano.s@gmail.com> Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
…o 0.29.4 Implemented recommended validations on VDR library level as discussed. Updated UniFFI to version 0.29.4 to enable disabling of 'disable_java_cleaner' (improving compatibility with Android SDK 0.26.0). Adjusted Python wrapper to reflect UniFFI updates. Added new integration test scenarios for validation coverage. Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
flaviocgarcia78
force-pushed
the
Revocation_SERPRO
branch
from
d8f11aa to
8f019f6
Compare
Signed-off-by: flaviocgarcia78 <flavio.garcia@serpro.gov.br>
|
Hi everyone, Best regards, |
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
|
Hello @flaviocgarcia78, New changes are in the technical review. Thanks for your patience and feel free to ping me if there are any questions |
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
|
Hello Renata, Thank you for your review and helpful suggestions. Additionally, we’ve been considering extending the besu-vdr module to support a multi-ledger approach. We believe this could enhance interoperability between different ledger networks within the same infrastructure. Best regards, |
|
@flaviocgarcia78 I would say that write operations must be validated on the contract level as much, but without growing the storage too much. For example, I think we should store the identity address of revocation registry definition owner and verify sender of revocation registry against it (it looks like we still do not perform this check in the latest version). But we cannot store all entries as they quite big in size. |
@Artemkaaas, |
|
@flaviocgarcia78 AFAIR it checks existence and ownership of DID but do not check that actor is ownership of revocation registry. For the creating rest anoncreds entities it is sufficient check, but for posting revocation registry we also need an addition one to ensure entry sender is owner of registry definition. |
Thanks @Artemkaaas, you’re right — that’s the correct behavior. I’ll make the necessary adjustment and resubmit the update. |
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
Signed-off-by: Flavio Garcia <flavio.garcia@serpro.gov.br>
|
@Artemkaaas / @Toktar, kindly review the adjustments made in this PR. |
|
Hello @flaviocgarcia78 , Best regards, |
Hello @Toktar, Best regards, |
6 participants
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.