Skip to content

(fix) VM resolution strategy correction#3622

Merged
esune merged 4 commits intoopenwallet-foundation:mainfrom
anonyome:gm/vm-resolve-fix
Apr 2, 2025
Merged

(fix) VM resolution strategy correction#3622
esune merged 4 commits intoopenwallet-foundation:mainfrom
anonyome:gm/vm-resolve-fix

Conversation

@gmulhearn
Copy link
Copy Markdown
Contributor

@gmulhearn gmulhearn commented Mar 31, 2025

The current VM resolution strategy will get potential VMs by getting the set VMs of that proof_purpose (e.g. "assertionMethod", "authentication") from the pydid formatted DIDDocument. It does this with a getattr (e.g. getattr(doc, "assertionMethod", []).

The problem is that the class attributes of the DIDDocument class are snake case (assertion_method), so attempts to use the proof purpose assertionMethod result in an empty result when you try to get the assertionMethod attribute.

I've just changed this to .get the attribute from the original raw doc dict instead, which has the attributes/keys in the camelCase format.

The new test is a regression test - it fails before the fix

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Copy Markdown
Contributor Author

Discovered this while testing W3C VCDM issuance (over DIDComm) with a cheqd DID.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@sonarqubecloud
Copy link
Copy Markdown

@swcurran swcurran requested review from esune and ff137 March 31, 2025 23:44
@swcurran
Copy link
Copy Markdown
Contributor

Thanks! Can you please take a look at the Ruff/lint failure?

I’ve asked @ff137 and @esune to review this.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Copy link
Copy Markdown
Member

@ff137 ff137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍


I think that the camelCase vs snake_case headaches should be avoided by aliasing the Pydantic fields, but that requires changes in pydid
@dbluhm

e.g.

class DIDDocumentRoot(Resource):
    assertion_method: Optional[List[Union[DIDUrl, VerificationMethod]]] = None

could be:

class DIDDocumentRoot(Resource):
    assertion_method: Optional[List[Union[DIDUrl, VerificationMethod]]] = Field(None, alias="assertionMethod")

Looks like Pydantic has a way to generate aliases automatically: https://docs.pydantic.dev/2.1/usage/model_config/#alias-generator

I'll post as an issue on pydid (edit: Indicio-tech/pydid#186)

Copy link
Copy Markdown
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@esune
Copy link
Copy Markdown
Member

esune commented Apr 1, 2025

@ff137 are we good with merging this now, or do we want to wait and see if the upstream library fixes the issue first with aliasing field names?

@ff137
Copy link
Copy Markdown
Member

ff137 commented Apr 2, 2025

So it seems that camelCase aliases are already implemented for the pydid resources. However, the getattr method only sees the original field names, not the aliased names.

The way to get aliased fields would be to dump the model to a dictionary: .model_dump_json(by_alias=True).
But, we already have the original dictionary in this context, so doing doc_raw.get(proof_purpose, []) should be fine. The assumption is just that this resolved doc will always have the expected camelCase format.

The pydid DIDDocument can deserialize either snake_case or camelCase fields, so that would be the benefit in first converting to a DIDDocument, so that it's standardised. But, if the standard is to use camelCase already, then it's much of a muchness.

@esune esune merged commit 2bd6f27 into openwallet-foundation:main Apr 2, 2025
9 checks passed
gmulhearn-anonyome pushed a commit to anonyome/aries-cloudagent-python that referenced this pull request Apr 18, 2025
…olve-fix

(fix) VM resolution strategy correction
gmulhearn-anonyome added a commit to anonyome/aries-cloudagent-python that referenced this pull request Apr 18, 2025
* Merge pull request openwallet-foundation#3622 from anonyome/gm/vm-resolve-fix

(fix) VM resolution strategy correction

* fix for inline VMs

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>

---------

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Co-authored-by: Emiliano Suñé <emiliano.sune@gmail.com>
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.

5 participants