Add UCAN v0.8 Test Fixtures #37

appcypher · 2022-03-24T15:43:41Z

Fixes #7

PR adds test fixtures for complying with UCAN v0.8.
While working on this I used a utility other people might find useful when working with UCAN fixtures in the future.
https://github.com/fission-suite/ucan-fixture-gen

Reviews welcome.

bgins · 2022-03-24T16:44:34Z

Hey @appcypher. Thanks for adding these test fixtures! They are looking great and are much appreciated. 🙏

One detail that should be changed is the ucv in these fixtures. The ucv should use semantic versioning. Instead of 0.8, could we change these to 0.8.1? (Or 0.8.0, if they aren't meant to cover the latest spec version.)

appcypher · 2022-03-25T17:46:01Z

@bgins @expede Forgot to mention that the versions have been fixed

bgins · 2022-03-25T21:44:08Z

tests/0.8.1/valid.json

+    }
+  },
+  {
+    "comment": "Delegated UCAN can have multiple valid proofs",


What would you think about rephrasing this comment to "Delegated UCAN is valid with multiple valid proofs"? This would strengthen the idea that a UCAN can have multiple proofs and is valid because of the capabilities granted by each of those proofs.

bgins · 2022-03-25T21:44:52Z

tests/0.8.1/valid.json

+    }
+  },
+  {
+    "comment": "UCAN attenuation is valid syntax",


"UCAN attenuation ishas valid syntax"

Typo. Has, not is.

bgins · 2022-03-25T21:50:33Z

tests/0.8.1/valid.json

+    }
+  },
+  {
+    "comment": "Payload `nbf` is valid",


Is this test redundant with "UCAN is ready to be used"? If we are testing the same thing, we should only keep one.

bgins · 2022-03-25T21:51:12Z

tests/0.8.1/valid.json

+    }
+  },
+  {
+    "comment": "Payload `exp` is valid",


Might be testing the same thing as "UCAN has not expired".

bgins · 2022-03-25T21:56:27Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "UCAN header section is missing or invalid",


We might want to say "UCAN header section is missing" and add a separate test case for an invalid header.

Got further down into the invalid cases, and I noticed there are cases of headers with fields missing. That's what I would consider and "invalid" header. No need to add more test cases (same goes for payload comment below, but we could use a garbage signature test case.)

The reason for having it as missing or invalid here is because it can be non-trivial to detect that it is actually missing. For example, the header may in fact not be missing but malformed maybe by having some additional non-compliant fields or missing some compliant fields. Or the header could be in an unexpected position. If it just says missing then we are forcing the implementer to detect all of those cases.
I can reword it as malformed if that is okay.

Good point. Reading through the example, it was trivial to see that the header was missing, but testing for it would be challenging. Renaming to malformed sounds good to me. 👍

bgins · 2022-03-25T21:58:20Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "UCAN payload section is missing or invalid",


Separate cases for missing and invalid might be good here as well.

bgins · 2022-03-25T21:59:24Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "UCAN signature is missing or invalid",


Separate cases for missing and invalid might be good here too.

bgins · 2022-03-25T22:04:53Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Witnesses expire before the delegated UCAN",


The expiration times for the delegate and witness are 5435042552. Did you mean edit the witness forward in time for this test case?

bgins · 2022-03-25T22:15:14Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Witness issuer audience did does not align with delegated issuer did",


It may be easier to distinguish the decentralized identifiers in this comment if we change "did" to "DID".

bgins · 2022-03-25T22:20:00Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Witness does not exist",


This comment could be made more specific by mentioning proof schemes. One possibility would be "Witness referenced in prf scheme does not exist".

bgins · 2022-03-25T22:33:52Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Payload `exp` field should be a did:key string",


"Payload exp field should be a ~~did:key string~~ number"

The test is right, but the comment is incorrect.

bgins · 2022-03-25T22:36:57Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Payload `prf` field should be an array of string",


"Payload prf field should be an array of ~~string~~strings"

Typo. Strings, not string.

bgins · 2022-03-25T22:37:46Z

tests/0.8.1/invalid.json

+    }
+  },
+  {
+    "comment": "Payload `prf` field should be an array of string",


"Payload prf field should be an array of ~~string~~strings"

Typo. Strings, not string.

bgins

Fantastic! Thrilled to have these tests, great work on them! 💯 🎉

I left a few comments, mostly nitpicky stuff around the comments. Thanks again for writing these! 🙏

appcypher · 2022-03-28T12:28:37Z

Fixed @bgins

expede

LGTM 👍

(@appcypher merge #49 first, and the cla-bot will also sign off)

expede · 2022-04-03T18:21:24Z

@appcypher ucan-fixture-generator is fabulous 🙌 Any concerns against us moving it from fission-suite to ucan-wg?

appcypher · 2022-04-04T08:38:29Z

@expede None and done

appcypher requested review from bgins and expede March 24, 2022 17:15

bgins reviewed Mar 25, 2022

View reviewed changes

bgins approved these changes Mar 25, 2022

View reviewed changes

appcypher force-pushed the appcypher/v0.8-test-fixtures branch from de16f9d to 73324f8 Compare March 28, 2022 12:23

bgins mentioned this pull request Mar 28, 2022

Consume test fixtures from spec ucan-wg/ts-ucan#61

Open

appcypher added a commit to ucan-wg/ucan-fixture-gen that referenced this pull request Mar 28, 2022

Fix impl: ucan-wg/spec#37

91c997d

appcypher requested a review from bgins April 1, 2022 07:29

expede approved these changes Apr 3, 2022

View reviewed changes

appcypher added 3 commits April 4, 2022 09:42

Add v0.8 fixtures

da37b67

Fix versioning

3a36998

Fix issues raised by @bgins

e13c7dc

appcypher force-pushed the appcypher/v0.8-test-fixtures branch from 73324f8 to e13c7dc Compare April 4, 2022 08:42

cla-bot bot added the cla-signed label Apr 4, 2022

appcypher merged commit d1a6490 into main Apr 4, 2022

expede deleted the appcypher/v0.8-test-fixtures branch April 7, 2022 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UCAN v0.8 Test Fixtures #37

Add UCAN v0.8 Test Fixtures #37

appcypher commented Mar 24, 2022 •

edited

Loading

bgins commented Mar 24, 2022 •

edited

Loading

appcypher commented Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

appcypher Mar 28, 2022 •

edited

Loading

bgins Mar 28, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022

bgins Mar 25, 2022 •

edited

Loading

bgins left a comment

appcypher commented Mar 28, 2022

expede left a comment

expede commented Apr 3, 2022

appcypher commented Apr 4, 2022

Add UCAN v0.8 Test Fixtures #37

Add UCAN v0.8 Test Fixtures #37

Conversation

appcypher commented Mar 24, 2022 • edited Loading

bgins commented Mar 24, 2022 • edited Loading

appcypher commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appcypher Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgins Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

bgins left a comment

Choose a reason for hiding this comment

appcypher commented Mar 28, 2022

expede left a comment

Choose a reason for hiding this comment

expede commented Apr 3, 2022

appcypher commented Apr 4, 2022

appcypher commented Mar 24, 2022 •

edited

Loading

bgins commented Mar 24, 2022 •

edited

Loading

appcypher Mar 28, 2022 •

edited

Loading

bgins Mar 25, 2022 •

edited

Loading