Skip to content
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

Add setters on DidDocument, remove DidDocumentBuilder #1164

Merged

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Mar 23, 2024

Removes custom DidDocumentBuilder in favor of setters on DidDocument itself.

Rationale:

  • While it's nice to have chaining API, the builder pattern didn't bring much in this case. All fields of DidDocument except for id are optional, so the builder itself didn't do much in terms of assuring DidDocument being valid. Rather then playing typical role of builder, assuring correctness of constructed datastructure, DidDocumentBuilder was rather used in manner of signalling "mut DidDoc". You could had very well rename DidDocumentBuilder to MutableDidDocument and the code would had been still making sense. This made implementation somewhat awkward where DidDocument had to be converted to DidDocumentBuilder in order to be edited (eg. DidPeerResolver), then subsequently converted back to DidDoc after edit.
  • Additionally, with builder being self-consuming, it was difficult to use builder within iteration scopes.

It's worth noting that the mutating method could in future performs runtime checks, such as whether a provided DidUrl reference (eg pub fn add_authentication_ref(&mut self, reference: DidUrl)) is referring to existing VerificationMethod.

Additionally:

  • Analogous approach applied on DidPeer4ConstructionDidDoc

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas changed the title Add setters on DidDocument, remove DidDocBuilder Add setters on DidDocument, remove DidDocumentBuilder Mar 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 189 lines in your changes are missing coverage. Please review.

Project coverage is 0.05%. Comparing base (413b183) to head (d3a02ef).

Files Patch % Lines
...peer_did/numalgos/numalgo4/construction_did_doc.rs 0.00% 114 Missing ⚠️
did_core/did_doc/src/schema/did_doc.rs 0.00% 41 Missing ⚠️
...did_peer/src/peer_did/numalgos/numalgo2/helpers.rs 0.00% 19 Missing ⚠️
...rc/protocols/did_exchange/state_machine/helpers.rs 0.00% 4 Missing ⚠️
...d_methods/did_resolver_sov/src/resolution/utils.rs 0.00% 4 Missing ⚠️
did_core/did_methods/did_peer/src/resolver/mod.rs 0.00% 3 Missing ⚠️
...ods/did_peer/src/peer_did/numalgos/numalgo2/mod.rs 0.00% 2 Missing ⚠️
...ods/did_peer/src/peer_did/numalgos/numalgo4/mod.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1164      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        484     484              
  Lines      24165   24184      +19     
  Branches    4489    4484       -5     
========================================
  Hits          13      13              
- Misses     24152   24171      +19     
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…dDocument

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@gmulhearn gmulhearn merged commit 09144f1 into hyperledger:main Mar 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants