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

Merging spike to main [WIP] #4

Merged
merged 7 commits into from
Feb 21, 2021
Merged

Conversation

veikkoeeva
Copy link
Contributor

@veikkoeeva veikkoeeva commented Feb 21, 2021

DO NOT MERGE YET

This is currently a rebase onto main to create a common history when there was previously none. Needs to be refactored so the code fits into the current structure and vice versa (and likely squashed with a reasonable commit message).

veikkoeeva and others added 7 commits February 21, 2021 17:17
- Refactors tests by removing duplicate code.
- Adds EBSI test vectors
- Adds comments on test methods.
- Adds placeholders to some simple test plans.

These changes are made towards adding adding simple DID creation and VC
signing flows and creating VC presentations.
@juanfranblanco
Copy link
Member

juanfranblanco commented Feb 21, 2021

@veikkoeeva Following the conversation I am doing the merge and this some of the thoughts of the following actions, i will create an issue with the following items todo and discuss.

The points of debate and issues with different implementations has been around the different libraries required and dependencies, so most of the work is to decouple this the implementations. This spike contains the most uptodate model so it will be easier to move to the main project.

Steps:

  • Merge.

  • Ill move the project to the spike folder, this will keep the whole history and easy to continue testing and easily move to the main project some of the functionality in spike.

  • From there 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 (as i had on my other work), we will use DataMembers 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.

  • 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.

  • 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.

Edit: And lots more.. :)

@juanfranblanco juanfranblanco merged commit eb3f9ea into main Feb 21, 2021
@veikkoeeva
Copy link
Contributor Author

veikkoeeva commented Feb 21, 2021

Sounds like a plan. I agree on decoupling the cryptographic provider and removing attributes from the data model. It is maybe one example on how to wrap Bouncy to Azure specific APIs, or use Azure specific APIs in general (those reading this, some links in code to docs how to extend them). would prefer to have working tests and some crypto API to work with fairly soon to move ahead. I don't mind if it gets completely swapped but if there's isn't limbo for too long. I will record a few thoughts I had about crypto in Monday or Tuesday likely if you don't already start one.

I do not know if there is a need to record why converters indead of, say, "a bag" on having a wrapper class to access attributes (my personal reasons, amonst some others: working with types looks easier and we can reference directly those to spec as links in code show, extendable without refactoring core -- while acknowledging they should be further refactored). Maybe record if we want to maintain a constraint that the code is useable in Blazor, Apple devices and so on. The spike uses attributes to jsonify .NET naming but it can also be configured, I don't about extension data. I record this tersely like this if someone reads this and thinks we can work without any attributes, could be possible.

We need builders and some other APIs too.

@juanfranblanco
Copy link
Member

@veikkoeeva nice I am trying to move your tests now for the convertors so that part is functional and we can think of a common interface to deserialise.

@juanfranblanco
Copy link
Member

@veikkoeeva well all the tests are failing due to this dotnet/runtime#29975 mainly DataMember does not work in system.text.json i truly don't believe it.. and is not coming until 6 or further down the line.. so not netstandard support either.

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.

2 participants