Fix DIFPresFormatHandler returning invalid V20PresExRecord on presentation verification

[rmnre]

We noticed that a V20PresExRecord which is returned by the present-proof v2.0 /verify-presentation endpoint does not match its schema when DIF format is used to request a presentation. Because of this, parsing it into a Kotlin class fails:
com.squareup.moshi.JsonDataException: Expected one of [true, false] but was True at path $.verified

Fixed this by applying json.dumps() when setting pres_ex_record.verified - just like it's done in the indy handler.

However, couldn't verified be a boolean instead of a string? I found comments in the schema and in the indy handler saying it has to be a string to be used as a tag (that was my takeaway at least ;) ), but I couldn't find where it is actually declared or used that way.

Signed-off-by: Roman Reinert [email protected]

[andrewwhitehead]

I agree that this should be fixed, but I wonder if changing the format in a patch release would cause issues for existing controllers. @ianco maybe?

[TimoGlastra]

I've seen multiple codebases that do verified == "true" because of the boolean being a string value (the better approach here is of course to parse the value, also accepting boolean values). So it should probably not be updated to a boolean value in a patch release.

[rmnre]

Just to avoid confusion: This fix does not change the type of verified to boolean, it only ensures that verified is a lowercase string representation of a boolean (instead of, currently, uppercase) and therefore valid as per the schema definition of V20PresExRecord.

-Sorry, I should probably have opened another issue to bring up the question about changing it to bool in the long run.

[andrewwhitehead]

Okay, I think this is probably reasonable to change then. If existing controllers are checking for == "True" then they will at least get spurious failures and not spurious verifications.

[codecov-commenter]

Codecov Report

Merging #1645 (f4690a8) into main (e77d087) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1645   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         528      528           
  Lines       32736    32737    +1     
=======================================
+ Hits        31264    31265    +1     
  Misses       1472     1472           

[swcurran]

@andrewwhitehead -- given your last comment, is this OK to merge? Could you provide a line or two about what to put into the release notes about this?

[swcurran]

Per the comments and that this is for W3C VC use cases, approving this.

[ianco]

I agree that this should be fixed, but I wonder if changing the format in a patch release would cause issues for existing controllers. @ianco maybe?

I think this change is ok, controllers that do case-sensitive compares of the verified value will need to be fixed, not a big deal.