Changed Proof (Verifier) API for checking presentation status

[Patrik-Stas]

Short summary

• Changes to Proof (verifier) API
• Backward compatible changes 0.53.0, however won't be anymore in the subsequent release 0.54.0 - simple migration strategy is provided

Changes aries-vcx

• Function fn presentation_status(&self) -> Status removed - it returned a result as a combination of revocation status and aries state.
• Function

fn get_revocation_status(handle: u32) -> LibvcxResult<VcxRevocationStatus>

replaced by new

fn get_presentation_status(&self) -> PresentationVerificationStatus

semantically returning the same result.

Other way to look at it is that get_revocation_status() was removed and presentation_status() / get_presentation_status() was changed to yield similar result as get_revocation_status did.

Changes aries-vcx Internals

• Replace RevocationStatus enum by new enum PresentationVerificationStatus.
  • The enum values are more accurately reflecting the actual nature of things - when presentation is verified, we only know whether the presentation is valid/invalid https://github.com/hyperledger/aries-vcx/blob/main/aries_vcx/src/protocols/proof_presentation/verifier/state_machine.rs#L225 (and yes, revocation can be common cause of invalidity, but not necessarily the only one - it can be simply buggy prover or a malicious actor tweaking the proof)
• The new PresentationVerificationStatus is backwards compatible, so there's a mapping of old values such as Revoked to new value Invalid etc.
• The verifier's FinishedState previously maintained the information as Option<RevocationStatus> whereas now it keeps PresentationVerificationStatus and the new enum contains variant Unavailable, so we don't need the Option. This makes it bit nicer to work with as there's less destructuring
  • Again, for sake of backwards compatibility, null or missing value will deserialize to PresentationVerificationStatus::Unavailable
• Renamed field revocation_status to verification_status, however also enable deserialization from the originalrevocation_status to enable backward compatibility

Changes libvcx_core

• Changes analogously propagated from aries_vcx eg.
  • Removed get_revocation_status
  • Behaviour of get_presentation_verification_status changed, as this function now simply returns value of PresentationVerificationStatus (as opposed to previous convoluted behaviour of that function).

Changeslibvcx, ios+java wrappers

• Modified function vcx_get_proof_msg so it returns presentation message as the name suggests (removed value of get_presentation_verification_status from the result tuple)

Migration

This slightly changes format of serialized (Verifier) Proof state machines, however still supports old format for reading in the next release 0.53.0. The support for reading serialized Proof created before 0.53.0 will be dropped in 0.54.0. If you wish to maintain ability to these older state machines, you will need to re-serialize state machine with 0.53.0 release, eg. (example in JS)

let vcxProofSerialized = await someStorage.load("proof123") // load the proof created by VCX before 0.53.0
let vcxProof = Proof.deserialize(vcxProofSerialized) // the 0.53.0 is still able to deserialize the old format
let reserialized = vcxProof.serialize() // serialized into new format
await smeStorage.save("proof123", reserialized) // stores the new format

Summary of the API after changes:

Forgetting about the delta, this is the final state:
Verifiers' Proof has 2 main methods to determine state

• pub fn get_state(&self) -> VerifierState - returns the Aries state from enum:

Initial
PresentationProposalReceived
PresentationRequestSet
PresentationRequestSent
Finished
Failed

• pub fn get_verification_status(&self) -> PresentationVerificationStatus - returns state of presentation verification, which can be one of following variants:

Valid - presentation was verified and valid
Invalid - presentation was verified, but was not valid (common cause can be using revoked credential in presentation)
Unavailable - no presentation is available to verifier

[codecov-commenter]

Codecov Report

Merging #770 (57d82d6) into main (4b73b09) will increase coverage by 0.04%.
The diff coverage is 55.81%.

@@            Coverage Diff             @@
##             main     #770      +/-   ##
==========================================
+ Coverage   54.64%   54.69%   +0.04%     
==========================================
  Files         380      381       +1     
  Lines       36762    36858      +96     
  Branches     8104     8124      +20     
==========================================
+ Hits        20089    20159      +70     
- Misses      10690    10702      +12     
- Partials     5983     5997      +14     

Flag	Coverage Δ
unittests-aries-vcx	54.58% <55.81%> (+0.04%)	⬆️

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

Impacted Files	Coverage Δ
...x/src/protocols/proof_presentation/verifier/mod.rs	100.00% <ø> (ø)
...s_vcx/src/utils/mockdata/profile/mock_anoncreds.rs	62.71% <ø> (ø)
...ries_vcx/src/utils/mockdata/profile/mock_ledger.rs	27.65% <ø> (ø)
...ries_vcx/src/utils/mockdata/profile/mock_wallet.rs	19.04% <ø> (ø)
aries_vcx/tests/utils/scenarios.rs	80.29% <0.00%> (ø)
aries_vcx/tests/test_creds_proofs.rs	88.00% <7.69%> (+0.09%)	⬆️
aries_vcx/tests/test_creds_proofs_revocations.rs	86.44% <16.66%> (-0.47%)	⬇️
...ocols/proof_presentation/verifier/state_machine.rs	69.07% <47.82%> (+0.26%)	⬆️
...proof_presentation/verifier/verification_status.rs	53.84% <53.84%> (ø)
aries_vcx/tests/utils/devsetup_agent.rs	73.36% <75.00%> (+0.30%)	⬆️
... and 3 more

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mirgee]

I assume this is why you gave the Unavailable variant parenthesis? How about we rather implement Default for PresentationVerificationStatus and use

#[serde(default, ...)]

here?

[Patrik-Stas]

I removed empty tuple parameter on Unavailable because it was causing issue with de/serialization - it ended up being serialized as

{ ... "verification_status": { "Unavailable": []} }

and even failed to be deserialized - not sure but looked like serde bug.

So anyway, I changed, though not exactly as you suggested; instead I refer to function verification_status_unavailable as default builder in this case. Since it's only local case where we need this default, I think it's not appropriate global default for the enum.

[bobozaur]

I don't think it's a serde bug. Why would the unit type be present in the Unavailable variant?

[Patrik-Stas]

Well I would expect that if a structure is serialized, it would be successfully deserialized back. Perhaps it was related to the custom deserialization?
Anyway, it doesn't matter anymore.

The question now is if you guys are fine with current solution default = "verification_status_unavailable"

[bobozaur]

LGTM

[Patrik-Stas]

@bobozaur

---- protocols::proof_presentation::verifier::states::finished::unit_tests::test_verifier_state_finished_deser_legacy_values_verification_status stdout ----
thread 'protocols::proof_presentation::verifier::states::finished::unit_tests::test_verifier_state_finished_deser_legacy_values_verification_status' panicked at 'called `Result::unwrap()` on an `Err` value: Error("missing field `verification_status`", line: 1, column: 68)', aries_vcx/src/protocols/proof_presentation/verifier/states/finished.rs:128:80

so nope, #[serde(default = "verification_status_unavailable")] must also stay, otherwise deserialization fails if neitherverification_status nor revocation_status is specified.
And #[serde(deserialize_with = "null_to_unavailable")] must stay for the case when it is specified, but value is null