Support adding/removing services and keys to an ion DID #562

andresuribe87 · 2023-06-23T22:12:18Z

Overview

Support adding/removing services and keys to an ion DID. Must be custodied.

Description

This is the last part on #340. Most of the complexity comes from needing to ensure concurrent updates work.

Note that this PR uses the same abstractions from the SDK.

How Has This Been Tested?

Wrote a unit test.

codecov-commenter · 2023-06-23T22:19:24Z

Codecov Report

Merging #562 (1b19f92) into main (5710ace) will decrease coverage by 1.21%.
The diff coverage is 7.31%.

@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
- Coverage   26.64%   25.44%   -1.21%     
==========================================
  Files          55       55              
  Lines        5855     6198     +343     
==========================================
+ Hits         1560     1577      +17     
- Misses       4023     4346     +323     
- Partials      272      275       +3

Files Changed	Coverage Δ
integration/common.go	`2.20% <0.00%> (-0.03%)`	⬇️
pkg/server/router/did.go	`2.79% <0.00%> (-0.66%)`	⬇️
pkg/service/did/service.go	`0.00% <0.00%> (ø)`
pkg/service/did/ion.go	`30.39% <8.96%> (-31.93%)`	⬇️
pkg/server/server.go	`70.91% <100.00%> (+0.11%)`	⬆️

decentralgabe · 2023-06-25T20:56:48Z

pkg/server/router/did.go

+
+type UpdateDIDByMethodRequest struct {
+	// Expected to be populated when `method == "ion"`. Describes the changes that are requested.
+	StateChange StateChange `json:"stateChange" validate:"required"`


this works until we support update for other methods, then we'll probably want to reconsider this API

I agree. Since we don't have other examples, I think this makes sense for now.

decentralgabe · 2023-06-25T20:57:24Z

pkg/server/router/did.go

+		framework.LoggingRespondErrMsg(c, errMsg, http.StatusBadRequest)
+		return
+	}
+	if *method != didsdk.IONMethod.String() {


I think this logic is better suited for the service layer, though it is OK to duplicate it

The service layer method is called UpdateIONDID, to which you pass in an UpdateIONDIDRequest object. By design, it's not possible to call that method without an ION did.

The external facing API is open for evolution. The internal is specific so it's more clear and it's easier to maintain.

decentralgabe · 2023-06-25T20:58:23Z

pkg/service/did/service.go

@@ -167,6 +167,18 @@ func (s *Service) CreateDIDByMethod(ctx context.Context, request CreateDIDReques
 	return handler.CreateDID(ctx, request)
 }

+func (s *Service) UpdateIONDID(ctx context.Context, request UpdateDIDRequest) (*UpdateDIDResponse, error) {


this could work better (but also maybe premature optimization) to have this be a generic UpdateDID method, and then check if the method is ION. This helps us maintain a consistent abstraction between the router (just focused on http) and service (focused on all DID methods here)

I do think it's premature. I propose we wait for when we have at least 2 methods for which we'll support updates.

Currently, it's only did:ion and I don't foresee others in the pipeline either.

decentralgabe · 2023-06-25T20:58:48Z

pkg/service/did/storage.go

@@ -69,7 +69,7 @@ func NewDIDStorage(db storage.ServiceStorage) (*Storage, error) {
 	return &Storage{db: db}, nil
 }

-func (ds *Storage) StoreDID(ctx context.Context, did StoredDID) error {
+func StoreDID(ctx context.Context, did StoredDID, tx storage.Tx) error {


nit:

Suggested change

func StoreDID(ctx context.Context, did StoredDID, tx storage.Tx) error {

func StoreDID(ctx context.Context, tx storage.Tx, did StoredDID) error {

decentralgabe · 2023-06-25T20:59:28Z

pkg/server/router/did.go

+	didION := ion.ION(id)
+	if !didION.IsValid() {
+		return nil, errors.Errorf("invalid ion did %s", id)
+	}


could check to see whether any of the updates are set to make sure it's not a no-op

I added this validation to ionHandler.UpdateDID. Could validate in both places, but the ionHandler one seems more appropriate in case other endpoints end up calling that method.

sounds good

decentralgabe · 2023-06-25T21:00:38Z

pkg/service/did/ion.go

+		},
+	}
+
+	if state.status == "" {


could this be inverted to reduce nesting?

or consider using a switch and 3 helper methods for readability

I don't think it can be inverted. This works like a state machine. When all updates succeed, status will reflect all 3 states within the same call.

When there are failures in the middle, the last state is kept. This is done to ensure that our version of a DID is kept in sync with what was anchored.

decentralgabe · 2023-06-25T21:01:07Z

pkg/service/did/ion.go

+				return nil, errors.Wrap(err, "creating update request")
+			}
+
+			state.preAnchor = &preAnchor{


this should be blocked until 539 is in, no?

Depends on how much pushback there will be for #539 :)

decentralgabe · 2023-06-25T21:04:07Z

pkg/service/did/ion.go

+
+			updateKey := updatePrivateKey.ToPublicKeyJWK()
+
+			signer, err := ion.NewBTCSignerVerifier(*updatePrivateKey)


why not call https://github.com/TBD54566975/ssi-sdk/blob/main/did/ion/operations.go#L175 instead

I wasn't able to find a way in which I could create an instance of ion.DID with a custom update key. The only way to create an instance of that struct is by calling NewIONDID, which creates new update keys.

I agree that it would be desirable, but didn't want to block this PR on an SDK update.

andresuribe87 · 2023-06-26T14:59:38Z

pkg/server/router/did.go

+
+type UpdateDIDByMethodRequest struct {
+	// Expected to be populated when `method == "ion"`. Describes the changes that are requested.
+	StateChange StateChange `json:"stateChange" validate:"required"`


I agree. Since we don't have other examples, I think this makes sense for now.

andresuribe87 · 2023-06-26T16:18:43Z

pkg/server/router/did.go

+		framework.LoggingRespondErrMsg(c, errMsg, http.StatusBadRequest)
+		return
+	}
+	if *method != didsdk.IONMethod.String() {


The service layer method is called UpdateIONDID, to which you pass in an UpdateIONDIDRequest object. By design, it's not possible to call that method without an ION did.

The external facing API is open for evolution. The internal is specific so it's more clear and it's easier to maintain.

andresuribe87 · 2023-06-26T16:22:08Z

pkg/server/router/did.go

+	didION := ion.ION(id)
+	if !didION.IsValid() {
+		return nil, errors.Errorf("invalid ion did %s", id)
+	}


I added this validation to ionHandler.UpdateDID. Could validate in both places, but the ionHandler one seems more appropriate in case other endpoints end up calling that method.

andresuribe87 · 2023-06-26T16:40:40Z

pkg/service/did/ion.go

+		},
+	}
+
+	if state.status == "" {


I don't think it can be inverted. This works like a state machine. When all updates succeed, status will reflect all 3 states within the same call.

When there are failures in the middle, the last state is kept. This is done to ensure that our version of a DID is kept in sync with what was anchored.

andresuribe87 · 2023-06-26T16:45:00Z

pkg/service/did/ion.go

+
+			updateKey := updatePrivateKey.ToPublicKeyJWK()
+
+			signer, err := ion.NewBTCSignerVerifier(*updatePrivateKey)


I wasn't able to find a way in which I could create an instance of ion.DID with a custom update key. The only way to create an instance of that struct is by calling NewIONDID, which creates new update keys.

I agree that it would be desirable, but didn't want to block this PR on an SDK update.

andresuribe87 · 2023-06-26T16:46:52Z

pkg/service/did/ion.go

+				return nil, errors.Wrap(err, "creating update request")
+			}
+
+			state.preAnchor = &preAnchor{


Depends on how much pushback there will be for #539 :)

andresuribe87 · 2023-06-26T16:50:49Z

pkg/service/did/service.go

@@ -167,6 +167,18 @@ func (s *Service) CreateDIDByMethod(ctx context.Context, request CreateDIDReques
 	return handler.CreateDID(ctx, request)
 }

+func (s *Service) UpdateIONDID(ctx context.Context, request UpdateDIDRequest) (*UpdateDIDResponse, error) {


I do think it's premature. I propose we wait for when we have at least 2 methods for which we'll support updates.

Currently, it's only did:ion and I don't foresee others in the pipeline either.

andresuribe87 · 2023-06-26T16:51:22Z

pkg/service/did/storage.go

@@ -69,7 +69,7 @@ func NewDIDStorage(db storage.ServiceStorage) (*Storage, error) {
 	return &Storage{db: db}, nil
 }

-func (ds *Storage) StoreDID(ctx context.Context, did StoredDID) error {
+func StoreDID(ctx context.Context, did StoredDID, tx storage.Tx) error {


andresuribe87 · 2023-06-26T16:52:22Z

pkg/service/did/ion.go

+}
+
+func isSignatureError(_ error) bool {
+	// TODO: figure out how to determine this error from the body of the response.


@decentralgabe could you help pointing me to the type of error that I would get if I send an operation to an ION node which has already been applied?

I'm not sure to be honest, maybe @thehenrytsai would know

nitro-neal · 2023-06-26T17:51:48Z

pkg/server/router/did.go

+	}
+	updateIONDIDResponse, err := dr.service.UpdateIONDID(c, *updateDIDRequest)
+	if err != nil {
+		errMsg := fmt.Sprintf("could not create DID for method<%s>", *method)


is this the right error message?

nitro-neal · 2023-06-26T19:51:00Z

if we remove pub keys / add other pub keys, and the private keys are not in the system, this will be like a "gimp" did if I'm understanding this correctly

It wont be able to do things like issue a vc and stuff, I think we already have good error messages that would explain that if you try to do something like that.

…yption keys

decentralgabe

@andresuribe87 can we push this through?

andresuribe87 added 2 commits June 23, 2023 17:58

Support adding/removing services and keys to an ion DID

3a4fb90

Merge branch 'main' into issue_340

e70388b

andresuribe87 requested review from decentralgabe and nitro-neal as code owners June 23, 2023 22:12

decentralgabe reviewed Jun 25, 2023

View reviewed changes

Reviews

b36fce8

andresuribe87 commented Jun 26, 2023

View reviewed changes

nitro-neal reviewed Jun 26, 2023

View reviewed changes

Merge branch 'main' into issue_340

0b955f3

andresuribe87 and others added 4 commits June 26, 2023 16:39

Fixes 560 where multiple instances of the service used different encr…

5311f6f

…yption keys

Merge branch 'main' into issue_560

60821bd

Merge branch 'issue_560' into issue_340

8b48809

Factories, refactoring, and integration test.

33f6710

decentralgabe approved these changes Jul 31, 2023

View reviewed changes

andresuribe87 added 5 commits July 31, 2023 15:33

Merge branch 'main' into issue_340

fc53eaf

Move to actually dealing with patches.

61a1a9e

Merge branch 'main' into issue_340

2b5bd4f

Merge branch 'main' into issue_340

39b7824

spec

38c99f4

andresuribe87 force-pushed the issue_340 branch from 2dca27b to 38c99f4 Compare August 3, 2023 19:31

andresuribe87 and others added 3 commits August 3, 2023 15:33

200 and spec

1b19f92

Moving around bits

7a7b672

Merge branch 'main' into issue_340

ac761e5

andresuribe87 merged commit 028dbf9 into TBD54566975:main Aug 3, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support adding/removing services and keys to an ion DID #562

Support adding/removing services and keys to an ion DID #562

andresuribe87 commented Jun 23, 2023

codecov-commenter commented Jun 23, 2023 •

edited

Loading

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 26, 2023

decentralgabe Jun 25, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 25, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

andresuribe87 Jun 26, 2023

decentralgabe Jun 26, 2023

nitro-neal Jun 26, 2023

andresuribe87 Aug 3, 2023

nitro-neal commented Jun 26, 2023 •

edited

Loading

decentralgabe left a comment

	func StoreDID(ctx context.Context, did StoredDID, tx storage.Tx) error {
	func StoreDID(ctx context.Context, tx storage.Tx, did StoredDID) error {


		updateKey := updatePrivateKey.ToPublicKeyJWK()

		signer, err := ion.NewBTCSignerVerifier(*updatePrivateKey)

Support adding/removing services and keys to an ion DID #562

Support adding/removing services and keys to an ion DID #562

Conversation

andresuribe87 commented Jun 23, 2023

Overview

Description

How Has This Been Tested?

codecov-commenter commented Jun 23, 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

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 commented Jun 26, 2023 • edited Loading

decentralgabe left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 23, 2023 •

edited

Loading

nitro-neal commented Jun 26, 2023 •

edited

Loading