spruceid / ssi

Core library for decentralized identity.
https://spruceid.dev
Apache License 2.0
187 stars 59 forks source link

Upgrade `json-ld`. #500

Closed timothee-haudebourg closed 1 year ago

timothee-haudebourg commented 1 year ago

This PR upgrades the json-ld dependency.

Implementation

Added dependencies

Possible improvements

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

timothee-haudebourg commented 1 year ago

@sbihel the ssi_json_ld::json_to_dataset is always called with the same options across ssi. Should I remove those options to simplify the code or is it really used outside ssi with different options?

Also I have a few clippy warnings in ssi-ucan, but I don't think they were introduced by my changes so I won't fix them here (unless I'm wrong).

warning: the borrowed expression implements the required traits
   --> ssi-ucan/src/lib.rs:160:21
    |
160 |                     &DagJsonCodec.encode(&to_ipld(&self.header).map_err(IpldError::new)?)?,
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `DagJsonCodec.encode(&to_ipld(&self.header).map_err(IpldError::new)?)?`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `#[warn(clippy::needless_borrow)]` on by default

warning: the borrowed expression implements the required traits
   --> ssi-ucan/src/lib.rs:164:21
    |
164 |                     &DagJsonCodec.encode(&self.payload)?,
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `DagJsonCodec.encode(&self.payload)?`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: the borrowed expression implements the required traits
   --> ssi-ucan/src/lib.rs:296:21
    |
296 |                     &DagJsonCodec.encode(&to_ipld(&header).map_err(IpldError::new)?)?,
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `DagJsonCodec.encode(&to_ipld(&header).map_err(IpldError::new)?)?`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: the borrowed expression implements the required traits
   --> ssi-ucan/src/lib.rs:299:39
    |
299 |                 base64::encode_config(&DagJsonCodec.encode(&self)?, base64::URL_SAFE_NO_PAD),
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `DagJsonCodec.encode(&self)?`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: `ssi-ucan` (lib) generated 4 warnings

Edit: well it prevents me from passing the CI, so I'll fix it anyway.

timothee-haudebourg commented 1 year ago

Everything seems to work except for the presentation_from_credential_issue_verify test in ssi-vc that runs out of memory during some JSON-LD document expansion.

Async function seem to consume a lot of memory, and I've checked, less than a megabyte of stack remains when the expansion algorithm is called. I'm not sure how that can be solved without increasing the stack size. I've tried in another context, with more stack and the expansion succeeded.

timothee-haudebourg commented 1 year ago

I solved the stack size issue by increasing the minimum stack size to 3MB. This is done by defining the RUST_MIN_STACK environment variable in .cargo/config.toml.

sbihel commented 1 year ago

@sbihel the ssi_json_ld::json_to_dataset is always called with the same options across ssi. Should I remove those options to simplify the code or is it really used outside ssi with different options?

I would be in favour. I don't think it's used by anyone and we can always re-introduce them later.

sbihel commented 1 year ago

The LinkedDataDocument::get_contexts returns a (optional) string representing a JSON-LD context. The implementer is expected to convert its context to a string, and the caller then re-parse the context before calling the json-ld lib. Instead the function could directly return a json_ld::syntax::context::Value or even better jsonld::RemoteDocumentReference<, _, json_ld::syntax::context::Value> directly. Then no need to parse anything, but I'm not sure of the impact on ssi, it may involve greater changes.

This is probably a issue for later but I agree with you, it's a bit too funky. After a quick glance, I wonder why it's not directly returning Contexts. Because that's really the only use we have for it