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

Support custom context sets #388

Closed
vdods opened this issue Feb 10, 2022 · 8 comments
Closed

Support custom context sets #388

vdods opened this issue Feb 10, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@vdods
Copy link
Contributor

vdods commented Feb 10, 2022

I'm working within a consortium of organizations which have defined a couple of industry-specific W3C VC contexts. ssi::jsonld::StaticLoader provides a list of general-use contexts, and I've got a branch of ssi in which I've added my own. I'm wondering if the ssi crate might provide a way to specify the set of contexts, in case of the need to add additional application-specific contexts. Looking at the current implementation, there's no clear simple way to plumb in a &dyn json_ld::loader::Loader, as the current use of StaticLoader funnels through about a dozen functions that aren't simply related (they don't share an obviously common set of parameters to generalize).

My initial thought was it might make sense to add to ssi::vc::LinkedDataProofOptions, but that's for generating proofs, not verifying them. So a custom Loader would be a parameter "parallel" to LinkedDataProofOptions for vc/vp-generating functions, but the custom Loader would need to be plumbed into signing functions separately. I'm hesitant to just do this myself because it feels like spaghetti code, and maybe there's a plan for adding that in a more sensible way later.

@clehner
Copy link
Contributor

clehner commented Feb 11, 2022

In many places a DID resolver trait object is passed around, also adjacent to the proof options. One possible approach may be to change that to use a more generalized resolver/loader trait object in the high-level functions, that DIDResolver could blanket-implement. Like how the JS implementation use a document loader: https://github.com/decentralized-identity/jsonld-document-loader

Passing around a set of contexts, rather than using a loader object, would avoid having to deal with security questions about loading remote context files - but would probably mean adding another parameter like you said -- unless the loader object just uses a set of passed-in URLs mapped to files/objects as its "backing" along with the existing DID resolver(s), and loading additional remote things does not need to be enabled.

@clehner
Copy link
Contributor

clehner commented Feb 11, 2022

Related issue in didkit: spruceid/didkit#151

@vdods
Copy link
Contributor Author

vdods commented Mar 1, 2022

I'm making an attempt at this, and it's proving a bit tricky, because adding a "context loader" parameter requires touching many function definitions and call sites. Unfortunately, based on how the ::jsonld::context::Loader trait is defined (it sneaks a Sized bound in there somewhere), Loader can't be used as a trait object, so I wasn't able to have the loader parameter simply be a &mut (dyn Loader + Sync + Send). For now, I'm using a simple ContextLoader struct which contains a map of the context URLs and their resolved documents. And also based on the unfortunate way ::jsonld::context::Loader is defined, it requires the loader parameter be a mut ref, which sure complicates having a built-in, static ContextLoader with the existing defaults from StaticLoader.

Current work, which is a bit unpolished, and I wouldn't consider to be the ideal implementation of this feature (could use some work to refine how ContextLoader is defined and passed into the various functions), is at: https://github.com/LedgerDomain/ssi/tree/context-loader -- I'm not attempting to address spruceid/didkit#151 but potentially this branch could support that effort.

@vdods
Copy link
Contributor Author

vdods commented Mar 1, 2022

I think one way to deal with the mutability of the self parameter in json_ld::Loader trait's load method is to have ContextLoader simply hold an std::sync::Arc<tokio::sync::RwLock<_>> to the desired HashMap of contexts. This way, there's a no-cost way to use the built-in contexts, and it's easy enough to specify one's own map of contexts.

@clehner clehner added the enhancement New feature or request label Apr 11, 2022
@vdods
Copy link
Contributor Author

vdods commented Apr 19, 2022

I'd like to get back to work on this one. Any feedback about the approach? I.e. passing around a ContextLoader object around the many various function calls (this is how the https://github.com/LedgerDomain/ssi/tree/context-loader branch currently implements it) vs putting together something that combines DIDResolver and ContextLoader?

@clehner
Copy link
Contributor

clehner commented Apr 21, 2022

@vdods I think it's a good approach.

I was hoping the use of lazy_static per context file could be kept for the parsing. With the changes all the context files get JSON-parsed at runtime. But maybe this is a pre-mature optimization. This would mostly be relevant for DIDKit CLI or other programs that exit after one operation more or less.

@vdods
Copy link
Contributor Author

vdods commented May 10, 2022

Ok, updated (new) PR is #432

@chunningham
Copy link
Contributor

This issue is addressed, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants