I have spend one hour and created a branch to simply demonstrate what I mean, and why going down the multiple class hierarchies is complete overkill at this point.
Although the PR has many changes, these are mostly name changes, to become more compliant with V11 in terms of naming. So almost every instance of initiateIssuance has been changed to credentialOffer. The functionality however remains the same.
So with only adding 2 sub interfaces for the change in payload and with adding the version discovery on the URI and with 2 points in current code where this becomes a bit more apparent the whole goal of the ticket is solved.
I am just posting this to hopefully show you that these are the types of tradeoffs we need to make. My approach leads to less opportunities for bugs and better maintainability at this point. Does it mean we will never introduce seperate class structures for future version of the spec? No of course not. We simply cannot know that upfront, but just creating class for the sake of OO would mean we might endup with many classes trying to do the same thing, for only minute changes in functionality.
Please have a look at it and see whether you can reuse certain parts of it, or simplify some of your code even more. Please do move the version discovery changes from my branch in your branch, as the server/issuer probably also wants to be able to do version discovery at one point. Of course I didn't create additional tests for V11, as this was just to show you how I would approach it. Adding tests, could result in maybe a few more places where something needs to be changed, but by simply looking at the actual change between initiateIssuance and credentialOffer this should be very limited.
nklomp
commented
1 year ago
I have spend one hour and created a branch to simply demonstrate what I mean, and why going down the multiple class hierarchies is complete overkill at this point.
Although the PR has many changes, these are mostly name changes, to become more compliant with V11 in terms of naming. So almost every instance of initiateIssuance has been changed to credentialOffer. The functionality however remains the same.
The real important bit of the PR is the CredentialOffer class where now a version is being determined, using a slight alteration on your version code (moved to the common module, removed the useless class using only statics) and then in https://github.com/Sphereon-Opensource/OpenID4VCI/blob/a2bfa5dc4be183112d7ec4597a03d0b02adee7fd/packages/client/lib/CredentialOffer.ts#L27 the logic to get the correct version (be aware that I also simply could have used the superclass in the interface, so that code wouldn't even be necessary).
The other change where the difference is exposed is in https://github.com/Sphereon-Opensource/OpenID4VCI/blob/a2bfa5dc4be183112d7ec4597a03d0b02adee7fd/packages/client/lib/AccessTokenClient.ts#L102
So with only adding 2 sub interfaces for the change in payload and with adding the version discovery on the URI and with 2 points in current code where this becomes a bit more apparent the whole goal of the ticket is solved.
I am just posting this to hopefully show you that these are the types of tradeoffs we need to make. My approach leads to less opportunities for bugs and better maintainability at this point. Does it mean we will never introduce seperate class structures for future version of the spec? No of course not. We simply cannot know that upfront, but just creating class for the sake of OO would mean we might endup with many classes trying to do the same thing, for only minute changes in functionality.
Please have a look at it and see whether you can reuse certain parts of it, or simplify some of your code even more. Please do move the version discovery changes from my branch in your branch, as the server/issuer probably also wants to be able to do version discovery at one point. Of course I didn't create additional tests for V11, as this was just to show you how I would approach it. Adding tests, could result in maybe a few more places where something needs to be changed, but by simply looking at the actual change between initiateIssuance and credentialOffer this should be very limited.