search

TBD54566975 / tbdex

56 stars 26 forks source link

regenerated test vector to add a valid DID for to field in `Message.metadata.from|to` #247

Closed jiyoonie9 closed 9 months ago

KendallWeihe commented 9 months ago

Could you add more description as to what made the pre-existing DIDs invalid? (or maybe point to a ticket which describes the issue)

I'm cool with the CODEOWNERs change, though it's worthy of a discussion (changes here being so upstream of everything else makes changes here have leverage, resulting in large downstream implications; my point being, if there ever were a need to create a constitution of governance then it should be had here -- "we're not there yet" is a valid response such that we can iterate quicker right now). Which, there may have been a discussion for which I wasn't privy to, but if not, could we make the CODEOWNERs a dedicated PR and/or ticket on the backlog?

jiyoonie9 commented 9 months ago

Could you add more description as to what made the pre-existing DIDs invalid? (or maybe point to a ticket which describes the issue)

yes, can do! so i found this while getting my tbdex-js pr up to speed with main, and found that tbdex had some schema changes that enforced Message.metadata.from/to to be a real DID. the test vector was not updated with this change, and some of the test vectors had the value for those fields set to an invalid DID like did:ex:alice or did:ex:pfi. tests in tbdex-js and tbdex-kt that used the test vectors were failing because of this

I'm cool with the CODEOWNERs change, though it's worthy of a discussion (changes here being so upstream of everything else makes changes here have leverage, resulting in large downstream implications; my point being, if there ever were a need to create a constitution of governance then it should be had here -- "we're not there yet" is a valid response such that we can iterate quicker right now). Which, there may have been a discussion for which I wasn't privy to, but if not, could we make the CODEOWNERs a dedicated PR and/or ticket on the backlog?

happy to move the codeowners change out to a separate PR! edit: pr here - https://github.com/TBD54566975/tbdex/pull/248

KendallWeihe commented 9 months ago

the test vectors had the value for those fields set to an invalid DID like did:ex:alice or did:ex:pfi

Got it, thanks! A good amount of these seem like did:dht-to-did:jwk changes. I would imagine we want did:dht in some of our test vectors, albeit maybe not currently? Or maybe it's beside the point and either or are fine and the changes here were just a byproduct of you generating new test vectors. Just raising awareness cc @phoebe-lew

jiyoonie9 commented 9 months ago

the test vectors had the value for those fields set to an invalid DID like did:ex:alice or did:ex:pfi

Got it, thanks! A good amount of these seem like did:dht-to-did:jwk changes. I would imagine we want did:dht in some of our test vectors, albeit maybe not currently? Or maybe it's beside the point and either or are fine and the changes here were just a byproduct of you generating new test vectors. Just raising awareness cc @phoebe-lew

yes, so i actually did initially opt to use did:dht, but found an issue while using did:dht:... for DIDs in test vectors and then ran tests that failed in tbdex-kt. https://github.com/TBD54566975/web5-kt/issues/252

i should probably write an issue to revert the DIDs back to did:dht once that issue is resolved. wrote issue here: https://github.com/TBD54566975/tbdex/issues/249

mistermoe commented 9 months ago

I thiiink we may want to keep the PFI DIDs as did:dht beeecause PFIs won't ever be able to use did:jwk beeecauuse they don't have support for service endpoints.

Thoughts?

jiyoonie9 commented 9 months ago

So would that mean we have to first address the did:dht issue i found while running test vector tests in tbdex-kt? I only used did:jwk to unblock myself so all the tests would pass with the latest test vectors.

On Fri, Feb 23, 2024 at 10:01 PM Moe Jangda @.***> wrote:

I thiiink we may want to keep the PFI DIDs as did:dht beeecause PFIs won't ever be able to use did:jwk beeecauuse they don't have support for service endpoints.

Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/TBD54566975/tbdex/pull/247#issuecomment-1962188385, or unsubscribe https://github.com/notifications/unsubscribe-auth/A4IFEFYPB5AHMOY3CLPAMDLYVE3WFAVCNFSM6AAAAABDV2FWY6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGE4DQMZYGU . You are receiving this because you authored the thread.Message ID: @.***>

mistermoe commented 9 months ago

@jiyoontbd just fixed the issue you found earlier!

Related issue: https://github.com/TBD54566975/web5-kt/issues/252 PR with fix: https://github.com/TBD54566975/web5-kt/pull/251 PR with updates to test vector generation: https://github.com/TBD54566975/tbdex-js/pull/185