rfc3339, json, Web5Error, and VerifiableCredential #283

KendallWeihe · 2024-08-14T15:37:12Z

Add json.rs module
Add rfc3339.rs module
Add Web5Error in the errors.rs module
Redo VerifiableCredential::create()
Bind & implement Kotlin code for VC create
Rename RustCoreError to Web5Errror
Write full unit test for VC create for both Rust and Kotlin

KendallWeihe · 2024-08-14T20:29:18Z

Justfile

@@ -6,7 +6,7 @@ setup:
  source bin/activate-hermit
  git submodule update --init --recursive
  if [[ "$(cargo 2>&1)" == *"rustup could not choose a version of cargo to run"* ]]; then
-    rustup default 1.78.0
+    rustup default 1.80.0


we get LazyLock included now! https://blog.rust-lang.org/2024/07/25/Rust-1.80.0.html#lazycell-and-lazylock

crates/web5_cli/src/vcs.rs

crates/web5/src/rfc3339.rs

KendallWeihe · 2024-08-15T19:54:56Z

Tests failing, but will fix here shortly

nitro-neal · 2024-08-15T20:36:20Z

bound/kt/src/test/kotlin/web5/sdk/TestHelper.kt

+    val tests: MutableList<String>
+
+    init {
+        val path = Paths.get("../../tests/unit_test_cases/$name.json")


This almost needs to be copied in the binary or at least in the bound directory (kind of like schemas) because if people pull it in maven this would not work..

However, people who pull in maven would not execute theses tests so it could be fine? Just some food for thought

fair point, but should be fine for now. as I understand it, our maven artifact does not include tests

nitro-neal · 2024-08-15T20:39:33Z

bound/kt/src/test/kotlin/web5/sdk/vc/VerifiableCredentialTest.kt

+    @Test
+    fun test_issuer_string_must_not_be_empty() {
+        this.testSuite.include()
+        val emptyIssuer = Issuer.StringIssuer("")
+        try {
+            VerifiableCredential.create(emptyIssuer, CREDENTIAL_SUBJECT, VerifiableCredentialCreateOptions())
+            fail("Expected an exception to be thrown")
+        } catch (e: Web5Exception) {
+            when (e) {
+                is Web5Exception.Exception -> {
+                    assertEquals(e.msg, "parameter error issuer id must not be empty")
+                }
+                else -> {
+                    fail("Caught an unexpected exception type")
+                }
+            }
+        }
+    }


Suggested change

@Test

fun test_issuer_string_must_not_be_empty() {

this.testSuite.include()

val emptyIssuer = Issuer.StringIssuer("")

try {

VerifiableCredential.create(emptyIssuer, CREDENTIAL_SUBJECT, VerifiableCredentialCreateOptions())

fail("Expected an exception to be thrown")

} catch (e: Web5Exception) {

when (e) {

is Web5Exception.Exception -> {

assertEquals(e.msg, "parameter error issuer id must not be empty")

}

else -> {

fail("Caught an unexpected exception type")

}

}

}

}

@Test

fun testIssuerStringMustNotBeEmpty() {

testSuite.include()

val emptyIssuer = Issuer.StringIssuer("")

val exception = assertFailsWith<Web5Exception.Exception> {

VerifiableCredential.create(emptyIssuer, CREDENTIAL_SUBJECT, VerifiableCredentialCreateOptions())

}

assertEquals("parameter error issuer id must not be empty", exception.msg)

}

very nice! done on all of 'em!

nitro-neal · 2024-08-15T20:39:58Z

bound/kt/src/test/kotlin/web5/sdk/vc/VerifiableCredentialTest.kt

+        this.testSuite.include()
+        val issuer = Issuer.ObjectIssuer("", "Example Name")
+
+        try {


if assertFailsWith works as the other one can update here too

yup done, it's assertThrows btw

nitro-neal · 2024-08-15T20:44:36Z

bound/kt/src/test/kotlin/web5/sdk/vc/VerifiableCredentialTest.kt

+        val vc = VerifiableCredential.create(ISSUER, CREDENTIAL_SUBJECT, VerifiableCredentialCreateOptions())
+
+        val now = Date()
+        val oneSecondAgo = Date(now.time - 1000)


maybe set to 1 year or something incase this github action nondeterministically lags at the wrong time or something lol

meh, we'd just re-run the test

maybe I could make it 10 seconds tho, that seems reasonable to me... done!

nitro-neal · 2024-08-15T20:45:51Z

crates/web5_cli/src/vcs.rs

@@ -49,7 +49,7 @@ impl Commands {
                    Some(i) => i.to_string(),
                    None => match &portable_did {
                        Some(p) => p.did_uri.to_string(),
-                        None => String::default(),
+                        None => panic!("either --issuer or --portable-did must be supplied"),


did you mean to keep the panic in?

yeah I think it's a slightly better error DX than what the call to ::create() will return which is just that the issuer string can't be non-empty

remember, this is in the CLI so panic is fine, preferred actually

nitro-neal · 2024-08-15T20:49:34Z

crates/web5/src/credentials/verifiable_credential_1_1.rs

+            types
+        };
+
+        let id = options


now we get into the philosophical discussion on "what it is to be a vc"
Should we let people put anything they want and create "invalid" vcs

Or should add stuff like this to a validate at the end and "dont let them create" whatever they want

I definitely lean towards being more constrictive than not, but I think we've struck the right amount

the web5 spec recommends a format:

A URI representing a unique identifier for the credential. RECOMMENDED to be of form urn:uuid:3978344f-8596-4c3a-a978-8fcaba3903c5

so we generate in accordance with that format

I'd be down to do more validation, but idk what ruleset we could add here other than "non-empty". MUST NOT have more than one value is impossible to validate AFAIK. MUST be a URI could be validated, but that seems heavy handed.

it's great food for thought! I welcome any additional ideas!

nitro-neal · 2024-08-15T20:51:44Z

crates/web5/src/credentials/verifiable_credential_1_1.rs

-            issuance_date,
-            expiration_date,
+            issuance_date: options.issuance_date.unwrap_or_else(SystemTime::now),
+            expiration_date: options.expiration_date,


should we let them create an "invalid" vc that is expired out of the gate.

I think what we have is fine actually, people using this should be aware of what they are doing,

and once they sign and verify then it will instantly be invalid so they can catch it there but yea things to think about

yeah I thought about that but I lean towards enabling them to create an date which is already expired, may be useful is weird edge cases, and the spec doesn't specifically make an assertion except, to your point, vc-jwt verification

nitro-neal · 2024-08-15T20:52:42Z

crates/web5/src/credentials/verifiable_credential_1_1.rs

    }

-    pub fn sign(&self, bearer_did: &BearerDid) -> Result<String> {
+    pub fn sign(&self, bearer_did: &BearerDid) -> ResultOld<String> {


lol I want to streamline all error cases into the single Web5Error instead of the uber-fragmented and all-over-the-place error system we currently have

it's worth noting this is just a rename on the import

added this #289

nitro-neal

I'm super onboard with the CreateOptions it keeps the apid clean and tight 🥇

KendallWeihe · 2024-08-16T16:15:05Z

I'm super onboard with the CreateOptions it keeps the apid clean and tight 🥇

Same, I'm going to iterate through everything and do the same. Feel free to beat me to it @nitro-neal @diehuxx!

… mod

* main: rfc3339, json, Web5Error, and VerifiableCredential (#283) Web5 test vectors (#284)

KendallWeihe requested review from nitro-neal, kirahsapong, shamilovtim, diehuxx, decentralgabe, frankhinek, leordev and ALRubinger as code owners August 14, 2024 15:37

KendallWeihe marked this pull request as draft August 14, 2024 15:37

KendallWeihe commented Aug 14, 2024

View reviewed changes

crates/web5_cli/src/vcs.rs Show resolved Hide resolved

nitro-neal reviewed Aug 14, 2024

View reviewed changes

crates/web5/src/rfc3339.rs Outdated Show resolved Hide resolved

KendallWeihe marked this pull request as ready for review August 15, 2024 19:20

nitro-neal reviewed Aug 15, 2024

View reviewed changes

KendallWeihe changed the title ~~Draft: rfc3339, json, Web5Error, and VerifiableCredential~~ rfc3339, json, Web5Error, and VerifiableCredential Aug 15, 2024

nitro-neal reviewed Aug 15, 2024

View reviewed changes

nitro-neal approved these changes Aug 15, 2024

View reviewed changes

This was referenced Aug 16, 2024

Consolidate test vectors, add nextest filter #288

Merged

Add singular Web5Error for error cases #289

Open

KendallWeihe added 3 commits August 16, 2024 12:21

Add rfc3339 module, add json module, and use in vc module, add errors…

fd76604

… mod

VerfiableCredential::create with full unit test coverage

f58e166

Add JSON unit test suite

49f7585

KendallWeihe added 12 commits August 16, 2024 12:21

Bind VC create to Kotlin

d60e71f

Rename serialize_optional*

dc45f3e

Add more kt tests

8083b62

Add UnitTestSuite to kt tests

e68a893

Undo the ResultOld

3371eec

Fix indentation

39c5352

Undo APID change

c4c98e9

Fix CLI and linting

c368f44

Rename RustCoreError to Web5Error

e16935e

Add #[allow(dead_code)]

dc0376c

Cleanup kt tests with assertThrows

acdf9b1

Increase the buffer to 10 seconds

f83e121

KendallWeihe force-pushed the kendall/vcs branch from 0a425f3 to f83e121 Compare August 16, 2024 16:21

Consolidate test vectors, add nextest filter (#288)

98b6209

KendallWeihe merged commit ea1c48e into main Aug 16, 2024
15 checks passed

KendallWeihe deleted the kendall/vcs branch August 16, 2024 17:01

diehuxx added a commit that referenced this pull request Aug 17, 2024

Merge branch 'main' into debug-logging

0654617

* main: rfc3339, json, Web5Error, and VerifiableCredential (#283) Web5 test vectors (#284)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfc3339, json, Web5Error, and VerifiableCredential #283

rfc3339, json, Web5Error, and VerifiableCredential #283

KendallWeihe commented Aug 14, 2024 •

edited

Loading

KendallWeihe Aug 14, 2024

KendallWeihe commented Aug 15, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

nitro-neal Aug 15, 2024

KendallWeihe Aug 16, 2024

KendallWeihe Aug 16, 2024

nitro-neal left a comment

KendallWeihe commented Aug 16, 2024

rfc3339, json, Web5Error, and VerifiableCredential #283

rfc3339, json, Web5Error, and VerifiableCredential #283

Conversation

KendallWeihe commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

KendallWeihe commented Aug 15, 2024

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

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

nitro-neal left a comment

Choose a reason for hiding this comment

KendallWeihe commented Aug 16, 2024

KendallWeihe commented Aug 14, 2024 •

edited

Loading