Modular credential format support for oid4vci

[weiiv]

In this PR, we've restructured the OID4VCI plugin by extracting the JWT credential issuance logic and moving it to a new JWT_VC_JSON plugin. This adjustment enables the OID4VCI plugin to concentrate exclusively on the protocol, eliminating the need to process individual credential formats. This modular approach simplifies the addition of new credential formats in the future without requiring changes to the OID4VCI plugin.

[dbluhm]

This is great! I'm very excited about this one. My only hesitation (upon first evaluation) is that the credential format plugins have a lot of boilerplate all just to provide that single cred processor for the format (plus supporting components). I'm interested in coming up with the right pattern to reduce that boilerplate.

A couple of thoughts

• ACA-Py plugins can also be defined by having just a minimal setup(InjectionContext) method in the main __init__ of the package; I think it would make sense to follow that pattern rather than using the definition.py plugin loading style since that's generally intended for plugins defining DIDComm protocols.
• @jamshale how do you feel about a category of "lite" plugins? I think the credential format plugins are most interesting in the context of their "parent" plugin, the oid4vci plugin. Having a full set of devcontainer and integration tests feels like overkill. The integration tests of oid4vci could pull in the plugins and exercise them their. With the caveat that if @weiiv found having those around on the format plugins helpful, then we should keep them.

Are there other ways we could slim up the cred format plugins?

If this line of thought dead-ends and we don't have immediately actionable outcomes, we can address these questions another time -- I'd rather have these changes merged sooner and figure out reducing boilerplate later.

[weiiv]

Thank you @dbluhm for your reviews. I think we should adopt the "lite" plugin pattern to reduce that boilerplate, e.g. devcontainer and integration tests.

[jamshale]

If the plugin isn't standalone then removing the devcontainer and integration tests is fine. They are meant to easily spin up an aca-py agent and test it without needing to install it into an aca-py project. Push code, re-install type dev pattern.

The only issue is currently the github actions expects a plugin to have integration tests in the integration folder, and also one of the management tools updates common libraries in the integration tests folder. We will need a way to exclude these lite plugins from these processes. Could be a simple text file or something in the root. I could probably do something like this pretty quickly.

Right now I think the github action integration test check will fail for any attempted lite plugin.

[weiiv]

Attached
jwt_vc_json_lite.zip
is the "lite" version I've tested locally. It doesn't include any setup methods, so it won't be loaded as a root-level plugin by acapy. Instead, it will be loaded by the oid4vci plugin on demand. Please review and let me know if this makes sense. Thanks.
P.S. not too sure if it will pass github flows though.

[jamshale]

Attached jwt_vc_json_lite.zip is the "lite" version I've tested locally. It doesn't include any setup methods, so it won't be loaded as a root-level plugin by acapy. Instead, it will be loaded by the oid4vci plugin on demand. Please review and let me know if this makes sense. Thanks. P.S. not too sure if it will pass github flows though.

Sounds good. I will try and review it. It should still pass the linting/formatting and unit test checks. It won't pass the integration test github action. I will need to do some work to support this. I will try and get to that asap. Shouldn't be much work.

In the meantime we can get this as ready as we can and approved and it can go in after the github action work is completed.

[weiiv]

Thanks. For Unit tests, it requires installing optional acapy and oid4vc packages, but not sure how to install those extras in github flows.

[weiiv]

Just found in the workflow file there is line:
poetry install --no-interaction --no-root --extras "aca-py"

Can we change it to below for additional dependencies?
poetry install --no-interaction --no-root --all-extras

Thanks.

[jamshale]

Can we change it to below for additional dependencies? poetry install --no-interaction --no-root --all-extras

Yes, we could change that, no problem.

[weiiv]

Good morning @dbluhm @jamshale, I've committed the "lite" version of the plugins, please have a look. Thanks.

[dbluhm]

Looks pretty good! Glad to see the boilerplate trimmed down significantly. I had a few comments.

[weiiv]

Fixes have been committed.

I've encountered a distuitls not found error when running pytest with python 3.12. Adding setuptools as dev-dependencies fixed the issue. Should we include setuptools for pytest?

[jamshale]

Fixes have been committed.

I've encountered a distuitls not found error when running pytest with python 3.12. Adding setuptools as dev-dependencies fixed the issue. Should we include setuptools for pytest?

Strange. I've never had this issue. We can address it if we have problems with the unit testing workflow.

[weiiv]

distuitls not found error

Found this: https://docs.python.org/3/whatsnew/3.12.html, looks like the package is removed in newer python.

[jamshale]

Odd. I know I ran the unit and integration tests on the existing code in python 3.12 in the draft PR #567. Anyway, If we have to install something than that is fine.

[weiiv]

Odd. I know I ran the unit and integration tests on the existing code in python 3.12 in the draft PR #567. Anyway, If we have to install something than that is fine.

Try recreate env and reinstall all with poetry, see if the error will popup. Never seen the error until I did so.

[weiiv]

@dbluhm @jamshale, any further comments to the PR? @jamshale, did you get a chance to check the distutils error with fresh poetry env? Thanks.

[jamshale]

@dbluhm @jamshale, any further comments to the PR? @jamshale, did you get a chance to check the distutils error with fresh poetry env? Thanks.

I will try and look today. I think it's looking pretty good. I'm not the best to review it, but will try.

It will need to be rebased after the #774 lite plugin support gets merged. Then it should pass the QA pipeline.

[jamshale]

I checked out and rebased with the lite plugins PR and everything worked as expected. Once we get that merged I will approve this.

FYI. I didn't have any problems with distutils in the integration pipeline. It would have came up there. So not sure what the deal is.

[weiiv]

FYI. I didn't have any problems with distutils in the integration pipeline. It would have came up there. So not sure what the deal is.

I think integration pipeline uses python v3.9 which still has distutils builtin, but it's been taken away from v3.12+ if I'm not mistaken.

[jamshale]

Oh I see... We can worry about that later when acapy 1.0.0 is released on python 3.12

[jamshale]

You'll need to rebase your branch with hyperledger/main. It's a few commits behind. We should be able to do a final review and get this merged after.

[weiiv]

You'll need to rebase your branch with hyperledger/main. It's a few commits behind. We should be able to do a final review and get this merged after.

Can I do merge just like the previous one ca8c3d2? Thanks.

[jamshale]

Merge is fine. We might possibly have to update the poetry.lock files with a commit after.

[jamshale]

Ya. I'm not sure why but the poetry lock file is inconsistent with pyproject.toml in oid4vci. This can happen when re-basing or merging main.

You can fix this by just running poetry lock --no-update in the root and integration directory and committing the lock file.

[weiiv]

Ya. I'm not sure why but the poetry lock file is inconsistent with pyproject.toml in oid4vci. This can happen when re-basing or merging main.

You can fix this by just running poetry lock --no-update in the root and integration directory and committing the lock file.

Did that in both folders, but only root generated a delta. Committed that.

[jamshale]

Nice! Everything is passing. We can give this a final review and merge.

@dbluhm When you get the time could you give this a quick go over?

[jamshale]

Looks very well done 👍

[dbluhm]

LGTM! Thanks!