Support ION DID Reconstruction & Long Form DID resolution #389

decentralgabe · 2023-05-22T22:40:08Z

adds the ability to reconstruct any ION DID's state from a set of patches

codecov-commenter · 2023-05-22T22:45:07Z

Codecov Report

Merging #389 (601c509) into main (3c2456d) will increase coverage by 0.15%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   57.18%   57.34%   +0.15%     
==========================================
  Files          65       67       +2     
  Lines        7016     7250     +234     
==========================================
+ Hits         4012     4157     +145     
- Misses       2290     2362      +72     
- Partials      714      731      +17

Impacted Files	Coverage Δ
did/ion/operations.go	`50.30% <ø> (-2.31%)`	⬇️
did/pkh/resolver.go	`0.00% <0.00%> (ø)`
did/web/resolver.go	`0.00% <0.00%> (ø)`
did/ion/model.go	`43.00% <36.00%> (-19.96%)`	⬇️
did/resolution/resolver.go	`16.95% <50.00%> (ø)`
did/ion/resolver.go	`57.32% <57.32%> (ø)`
did/ion/did.go	`67.01% <73.94%> (+24.70%)`	⬆️
did/ion/enum.go	`100.00% <100.00%> (ø)`
did/ion/request.go	`62.15% <100.00%> (ø)`
did/jwk/resolver.go	`66.67% <100.00%> (ø)`
... and 6 more

did/ion/did.go

andresuribe87 · 2023-05-23T05:36:31Z

did/ion/did.go

@@ -85,3 +108,216 @@ func LongToShortFormDID(longFormDID string) (string, error) {
 	}
 	return shortFormDID, nil
 }
+
+// PatchesToDIDDocument applies a list of sidetree state patches in order resulting in a DID Document.
+func PatchesToDIDDocument(shortFormDID, longFormDID string, patches []any) (*did.Document, error) {


I strongly discourage the use of any unless there is a strong reason to do so. Can you clarify why it's preferable to have patches be []any?

An alternative is to define an interface that all patches implement.

type Patch interface{ isPatch() } func (a AddServicesAction) isPatch() { }

will switch to interface

andresuribe87 · 2023-05-23T05:38:15Z

did/ion/did.go

+		switch knownPatch.(type) {
+		case AddServicesAction:
+			addServicePatch := knownPatch.(AddServicesAction)
+			doc.Services = append(doc.Services, addServicePatch.Services...)


If you do this, then you don't need to re-cast within each case. You can use typedPatch

Suggested change

switch knownPatch.(type) {

case AddServicesAction:

addServicePatch := knownPatch.(AddServicesAction)

doc.Services = append(doc.Services, addServicePatch.Services...)

switch typedPatch := knownPatch.(type) {

case AddServicesAction:

doc.Services = append(doc.Services, typedPatch.Services...)

andresuribe87 · 2023-05-23T05:40:34Z

did/ion/did.go

+
+// tryCastPatch attempts to cast a patch to a known patch type
+func tryCastPatch(patch any) (any, error) {
+	switch patch.(type) {


Same comment as above.

Mixing map[string]any and typed structs leads to too much freedom and code that is harder to maintain. Can this API be structured so we can have devs avoid shooting themselves in the foot?

andresuribe87 · 2023-05-23T05:47:48Z

did/ion/did.go

+		removed := false
+		for i, key := range doc.VerificationMethod {
+			if key.ID == id {
+				doc.VerificationMethod = append(doc.VerificationMethod[:i], doc.VerificationMethod[i+1:]...)


There is a lot of copying around within this function. Is this OK?

just a way to remove an element in an array should be fine

andresuribe87 · 2023-05-23T05:48:29Z

did/ion/did.go

+						doc.AssertionMethod = append(doc.AssertionMethod[:j], doc.AssertionMethod[j+1:]...)
+					}
+				}
+				for j, auth := range doc.Authentication {


Should this be KeyAgreement?

andresuribe87 · 2023-05-23T05:48:39Z

did/ion/did.go

+						doc.KeyAgreement = append(doc.KeyAgreement[:j], doc.KeyAgreement[j+1:]...)
+					}
+				}
+				for j, auth := range doc.Authentication {


CapabilityInvocation?

andresuribe87 · 2023-05-23T05:48:52Z

did/ion/did.go

+						doc.CapabilityInvocation = append(doc.CapabilityInvocation[:j], doc.CapabilityInvocation[j+1:]...)
+					}
+				}
+				for j, auth := range doc.Authentication {


CapabilityDelegation?

andresuribe87 · 2023-05-23T05:50:21Z

did/ion/did.go

+
+				// TODO(gabe): in the future handle the case where the value is not a simple ID
+				// remove from all other key lists
+				for j, auth := range doc.Authentication {


Would it be possible to un-nest this for loop and the following ones?

andresuribe87 · 2023-05-23T05:51:38Z

did/ion/did_test.go

+		doc, err := PatchesToDIDDocument("did:ion:test", "", []any{struct{ Bad string }{Bad: "bad"}})
+		assert.Empty(tt, doc)
+		assert.Error(tt, err)
+		assert.Contains(tt, err.Error(), "unknown patch type")


Here and elsewhere

Suggested change

assert.Contains(tt, err.Error(), "unknown patch type")

assert.ErrorContains(tt, err, "unknown patch type")

did/ion/did.go

andresuribe87 · 2023-05-23T20:11:03Z

did/ion/did.go

+					doc.AssertionMethod = append(doc.AssertionMethod[:j], doc.AssertionMethod[j+1:]...)
+				}
+			}
+			for j, auth := range doc.Authentication {


This is repeated. Is it intentional?

If not, should there be a test for this?

andresuribe87 · 2023-05-23T21:04:18Z

did/ion/did.go

+					doc.Authentication = append(doc.Authentication[:j], doc.Authentication[j+1:]...)
+				}
+			}
+			for j, auth := range doc.CapabilityInvocation {


Suggested change

for j, auth := range doc.CapabilityInvocation {

for j, ci := range doc.CapabilityInvocation {

andresuribe87 · 2023-05-23T21:04:26Z

did/ion/did.go

+					doc.CapabilityInvocation = append(doc.CapabilityInvocation[:j], doc.CapabilityInvocation[j+1:]...)
+				}
+			}
+			for j, auth := range doc.CapabilityDelegation {


Suggested change

for j, auth := range doc.CapabilityDelegation {

for j, cd := range doc.CapabilityDelegation {

andresuribe87 · 2023-05-23T21:08:18Z

did/resolution/resolver.go

-// ResolutionOption https://www.w3.org/TR/did-spec-registries/#did-resolution-options
-type ResolutionOption any
+// Option https://www.w3.org/TR/did-spec-registries/#did-resolution-options
+type Option any


Having any makes me sad :(

we can fix this when we implement any options
#331

andresuribe87 · 2023-05-23T21:09:33Z

did/ion/model.go

+		if !ok {
+			return fmt.Errorf("patch has no action")
+		}
+		currPatchBytes, err := json.Marshal(currPatch)


I think there is a way to use json.RawMessage to avoid the whole unmarshal -> marshal -> unmarshal round tripping, but I've never done it. If you're feeling brave :D

hm you're right but I am not feeling brave

andresuribe87 · 2023-05-23T21:10:25Z

did/ion/did.go

+
+			// TODO(gabe): in the future handle the case where the value is not a simple ID
+			// remove from all other key lists
+			for j, auth := range doc.Authentication {


Can you also unnest this and the following for loops?

how? this needs to be nested as far as I can tell

i have no idea what I was talking about... ignore please.

Co-authored-by: Andres Uribe <[email protected]>

support ion did reconstruction

f465c86

decentralgabe requested review from nitro-neal and andresuribe87 as code owners May 22, 2023 22:40

gabe and others added 5 commits May 22, 2023 16:09

name conflicts

f85a9e6

badrename

d519b04

Merge branch 'main' into oops

6d63e0e

Merge branch 'main' into oops

65ad480

Merge branch 'main' into oops

8a14de7

andresuribe87 reviewed May 23, 2023

View reviewed changes

gabe added 2 commits May 23, 2023 10:41

Switch to custom unmarshaller

13e315e

pr comments

72d488c

andresuribe87 reviewed May 23, 2023

View reviewed changes

decentralgabe and others added 3 commits May 23, 2023 19:04

Update did/ion/did.go

a782767

Co-authored-by: Andres Uribe <[email protected]>

Update did/ion/did.go

f6657de

Co-authored-by: Andres Uribe <[email protected]>

pr comments

601c509

andresuribe87 approved these changes May 24, 2023

View reviewed changes

decentralgabe merged commit bb5e0d7 into main May 24, 2023

decentralgabe deleted the oops branch May 24, 2023 01:38

decentralgabe mentioned this pull request May 24, 2023

Support a local ION resolver for long form DIDs #349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ION DID Reconstruction & Long Form DID resolution #389

Support ION DID Reconstruction & Long Form DID resolution #389

decentralgabe commented May 22, 2023

codecov-commenter commented May 22, 2023 •

edited

Loading

andresuribe87 May 23, 2023

decentralgabe May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

decentralgabe May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

decentralgabe May 23, 2023

andresuribe87 May 23, 2023

decentralgabe May 23, 2023

andresuribe87 May 23, 2023

decentralgabe May 24, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

andresuribe87 May 23, 2023

decentralgabe May 24, 2023

andresuribe87 May 23, 2023

decentralgabe May 24, 2023

andresuribe87 May 23, 2023

decentralgabe May 24, 2023

andresuribe87 May 24, 2023

	assert.Contains(tt, err.Error(), "unknown patch type")
	assert.ErrorContains(tt, err, "unknown patch type")

	for j, auth := range doc.CapabilityInvocation {
	for j, ci := range doc.CapabilityInvocation {

	for j, auth := range doc.CapabilityDelegation {
	for j, cd := range doc.CapabilityDelegation {

Support ION DID Reconstruction & Long Form DID resolution #389

Support ION DID Reconstruction & Long Form DID resolution #389

Conversation

decentralgabe commented May 22, 2023

codecov-commenter commented May 22, 2023 • edited Loading

Codecov Report

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

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

codecov-commenter commented May 22, 2023 •

edited

Loading