Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate and or integrate spikes (or previous work) into new architectural structure main #5

Open
8 of 9 tasks
juanfranblanco opened this issue Feb 21, 2021 · 4 comments

Comments

@juanfranblanco
Copy link
Member

juanfranblanco commented Feb 21, 2021

There has been previous work to start the dotnet dids libraries, this is how this project has started. So the first part of the project is to validate / move and convert these spikes / prototypes into the new main project.

Steps:

  • Create branches with spike projects, removing most of the obsolete code
  • Create a new skeleton architecture to start implementation
  • Move simple non obsolete components from Nethereum spike (url parser, tests).
  • Move the most up to date functional model from (@veikkoeeva ) spike into a temporary spike folder to keep history and ease migration, redesing or refactoring (@veikkoeeva Dotdecentralized) and testing.

Note: Whilst both spikes could have been ported directly (although the Nethereum was older). The Dotdecentralised introduced json convertors and other initial crypto, the only major issue is the general dependencies that both implementations had on specific libraries, this is a common problem in .net so a more abstract design is required, nevertheless the model will remain unchanged and specific libraries implementation can be moved into their projects.

  • First we can move the model from the spike as it is the latest. Following the conversation and research to remove the dependencies to System.Text.Json and Newtonsoft we will use DataMember attribute instead. Note I have found this to be an issue also on the ExtendedAttributes so that will need a common deserialiser for that type of object. A base class can be created to detect this.

System.Text.Json does not support yet DataMember to be decoupled this is coming maybe in .Net 6. For reference : This is going to be a major issue for decoupling. dotnet/runtime#29975 and dotnet/runtime#30009 (comment)

Edit: see #5 (comment) for further discussion, this might have to become its own issue.

  • Public key formats, the spike implements a specific converter to avoid having a attributes per each key type, this will need to be also implemented in Json.Net as the model has to follow the same pattern, Another alternative although simpler but not as clean, will be to refactor the model to include all the attributes as per the Java and other spike implementation, but that will mean including extra information in some public keys and probably not have the capability to plug extra key formats with different attributes. Something to think about or to note for clarification / documentation as both the Java and Javasript includes them internally https://github.com/decentralized-identity/did-common-java/blob/

  • New projects will be created to support System.Text.Json and Newtonsoft a common interface will be created in the common model project and implementations on specific projects.

  • The unit tests data utilized in System.Text.Json can be then reused for both, so that will be moved straight away.

  • All the crypto in the spike has dependencies to Azure libraries, this need to be removed or have an specific implementation. The current new structure has the new DidNet.Crypto that can hold all the common interfaces. The dependencies I have seen are JsonWebKeyConverter, AsymmetricSignatureProvider and ICryptoProvider all part of Azure Microsoft.IdentityModel.Tokens that need to be removed or decoupled. The pattern followed is similar to the JWT https://github.com/decentralized-identity/did-common-dotnet/blob/did-url-parser/spikes/did-url-parser/DID/Did.Jwt.Tests/JwtTests.cs#L29-L50 which will be necessary anyway for other interoperability both functional and a library perspective, but this might be a truly bigger topic to discuss.

More...

juanfranblanco added a commit that referenced this issue Feb 21, 2021
Move @veikkoeeva to the spike folder to start the migration to the new main project. T
juanfranblanco added a commit that referenced this issue Feb 21, 2021
…ext.Json #5

Move the model from the @veikkoeeva as it is the latest.
Extract out classes from files (simple refactoring) adjusting namespaces, etc
Remove dependencies to System.Text.Json, using DataMember with json property name instead.
Remove types depending on System.Text.Json and replace with object instead.
Create projects skeleton for Newtonsoft and System.Text.Json
DidNet.Json.SystemText and DidNetJson.Newtonsoft
@juanfranblanco
Copy link
Member Author

juanfranblanco commented Feb 22, 2021

The truth is that this extremely frustrating as what we are experience is a huge delay for what was something so simple is due to all the introduced incompatibilities in .Net and lack of framework implementation.

a) JSON having to create all the extra code for abstractions to support different libraries due to introduced incompatibilities and having to use Convertors when normally was not necessary. Still there is not a simple solution until dotnet/runtime#30009 (comment)

I don't know if it will be worthwhile to make it compatible with System.Text.Json then as they mentioned the reason for not making it compatible with DataMembers and my guess Json.Net that has been here for many years is because they don't support many features yet. Overall the architecture now supports to add this as an extension this and the code on your spike can be reused later on.

I propose we continue with a single simpler implementation what later on can be plugged in the extra functionality, truly MS should not have put System.Text.Json in the framework until it was ready and compatible with Newtonsoft. The fact that we are wasting so much time due to a Json library in 2021 is truly bad.

Also if we can remove the need of extra convertors, as possible the users of the library may just easily grab the model for deserialisation if wanted to use other libraries or extended it.

b) In the similar way System.Cryptography libraries do not support wasm yet as we have seen here, this was totally removed before. dotnet/runtime#40074 (comment) and this is what it has lead to that Bouncy abstraction, together with the proprietary Azure libraries.

@veikkoeeva
Copy link
Contributor

veikkoeeva commented Feb 22, 2021

I think STJ is the case to make compatible with, as noted in the linked issue. :)

It looks to me converters are needed because the DID specification introduces first a named type and then the actual type. This is a great thing since now the deserializer knows what to expect. Otherwise it would need to probe "all the possible options" introducting potential type confusion (it can be many since DID specification is extensible at some points, as also noted in the merged spike tests). The pattern is like TypeNameHandling = TypeNameHandling.Objects that embeds type information to JSON documents as is needed if there's inheritace. But what Json.NET does automatically with that option is not in DID compatible way. The other way also in JSON.NET I believe is to explicitly write a converter. Also one thing is that as JSON does not guarantee order elements but DID specification some places order is required, converters are the way to (de)serialize elements in certain, predictable order.

@juanfranblanco
Copy link
Member Author

@veikkoeeva Json.net provides a capability for order, this can be used already in DataMember Order too, this is truly key if we require to sign documents using JWT etc.. but yeah a converter will need to be used.

STJ is not finished and yes it will be great in the future hence we need to support both, but that truly makes the work super hard in the time being. But that is where we are..

@juanfranblanco
Copy link
Member Author

I believe we are in a good shape apart from the crypto to area to mark most items as done, obviously awaiting review from this branch https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces

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

No branches or pull requests

2 participants