Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web5 agent/register did service endpoints #741

Merged

Conversation

bnonni
Copy link
Contributor

@bnonni bnonni commented Jul 8, 2024

  • Add option to HdIdentityValue.initialize() to pass dwnEndpoints to allow the registration of the dwn did service duing DidDht.create()
  • Enables the ability to use the parent, agent did to connect to a remote DWN for connectedDid recovery
  • Can bypass the agentDid-->connectedDids pattern for use on the server side, if desired

Copy link

changeset-bot bot commented Jul 8, 2024

⚠️ No Changeset found

Latest commit: bb62d8d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@csuwildcat
Copy link
Contributor

Looks good to me!

Copy link
Member

@LiranCohen LiranCohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but I'd like to avoid the circular dependency between the packages.

packages/agent/package.json Outdated Show resolved Hide resolved
@bnonni
Copy link
Contributor Author

bnonni commented Jul 10, 2024

To test this locally, I'm doing:
Connect to localhost DWN using Web5.connect(), wipe the local DATA folder and reconnect with the produced recovery phrase. I confirmed that the didService is being added to the agentDid, but I'm not seeing the prior connected dids upon Web5.connect with recovery phrase. I feel like there's something I may be missing or maybe my local dwn isn't setup properly? Not sure how this recovery process works with the connected dids and the dwn exactly. Could use some more insight into that.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 10, 2024

To test this locally, I'm doing: Connect to localhost DWN using Web5.connect(), wipe the local DATA folder and reconnect with the produced recovery phrase. I confirmed that the didService is being added to the agentDid, but I'm not seeing the prior connected dids upon Web5.connect with recovery phrase. I feel like there's something I may be missing or maybe my local dwn isn't setup properly? Not sure how this recovery process works with the connected dids and the dwn exactly. Could use some more insight into that.

I updated the PR to include a Web5.recover method that simply recovers the agentVault and returns it for use within Web5.connect. Upon testing this code, I get the following error still

Sync failed: Error: Failed to dereference 'did:dht:hpihge9ouh697bj8dcmubb54yxuzzojkm4sbc7jd1csbdpmpa41o#dwn': notFound

Here is my test code:

Step 1: connect.ts

const password = 'some-password'
const { web5, did, recoveryPhrase } = await Web5.connect({
    password,
    sync: '1s'
})
console.log("did ", did);
console.log("recoveryPhrase ", recoveryPhrase);

const agent = web5.agent as Web5PlatformAgent
const dids = await agent.identity.list()
console.log("dids ", dids);
console.log("agentDidPortable ", JSON.stringify(await web5.agent.agentDid.export()));

Step 2: Delete local DATA folder and run recover.ts

const password = 'some-password'
const agentVault = await Web5.recover({ password, recoveryPhrase });
console.log('recover agentVault', agentVault);
const { web5, did } = await Web5.connect({
    agentVault,
    password,
    recoveryPhrase,
    sync: '1s',
});
console.log("web5 ", web5);
console.log("did ", did);
const agent = web5.agent as Web5PlatformAgent
const dids = await agent.identity.list()
console.log("dids ", dids);

const identity = await agent.identity.get({ didUri: 'did:dht:oeegnbss6ykiu4rhi7xeqhcb5tre9z3wsjk5ffsqn86sijiy6hwy' })
console.log("identity ", identity);

I can confirm that this DID has service endpoints

{
    "service": [
        {
            "id": "did:dht:hpihge9ouh697bj8dcmubb54yxuzzojkm4sbc7jd1csbdpmpa41o#dwn",
             "type": "DecentralizedWebNode",
             "serviceEndpoint": [
                 "https://dwn.tbddev.org/dwn6",
                 "https://dwn.tbddev.org/dwn0"
             ],
             "enc": "#enc",
             "sig": "#sig"
       }
   ]
}

I was using localhost:3000 before, but I wasn't getting the result I wanted, so I tried using the default DWNs. I know we saw this error before adding the did service endpoint registration, and it didn't surface after that, so I'm not sure what's happening here. I'll continue to try to debug as I have time, but I do feel as though I've reach some limit to my knowledge. @LiranCohen @csuwildcat any thoughts or ideas?

@LiranCohen
Copy link
Member

Hey @bnonni This is looking like it's going in the right direction!

I think the only part that's missing is on the recover section to register the agent Did and then kick off sync and wait for it to complete.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 11, 2024

@LiranCohen I see. I didn't realize the agent did is no longer registered when recovering the agent and the local data store, but that makes a lot of sense. Thank you. Will try that now.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

Ok, after some deep investigation, I made some updates; however, I am still unable to recover any connected dids but I discovered something that may be part of the problem:

When sync is not set in .connect(), it defaults to 2m. From what I can tell, that delays the first sync for 2m as well as subsequent syncs, meaning it does not do an initial sync to remote before setting the interval. startSync is called without an await in connect() which makes sense because you want the code execution to start the sync and continue to return the Web5 object. However, in recover, I need to have sync complete and then use the synced data to reestablish a connected did and return a Web5 object to the caller. If startSync is awaited, it never completes since its set on an interval. The order of startSync forces users to push first then pull. Two things come to mind as potential issues here.

  1. During recover, I can't easily do a discrete sync from remote to local, and continue executing code to return the Web5 connection to the caller
  2. startSync as its written forces push before pull and if not awaited, does not afford the ability to get a complete snapshot between the two states (local and remote) before creating and returning the Web5 object
  3. Idk what idk here, but it dawned on me that its possible the sync direction / order is forcing an overwrite of the remote data in the DWN. The newly recovered local data is blank since its just been newly recovered, if I push first, I imagine that pushing empty the empty registeredIdentites from the blank, local datatstore to the remote DWN.

Latest Changes

Based on this thinking, the following reqs make sense for recovery:

  1. Sync local/remote with a push, pull, push/pull or pull/push in a discrete, one-time manner
    • sync method: does a one-time push/pull or pull/push (depending on the arg passed) resolving the promise back to the caller. sync takes an arg for { direction: string }, which defaults to push to be in-line with the pattern established by startSync. Can be called as such sync({ direction: 'pull' }) to force a pull first.
    • push & pull methods exposed: can be used to call .pull() or .push() alone, call .pull() and .push() back-to-back, or .push() and .pull() back-to-back and have each of these calls complete in a discrete manner. My thinking with this pattern is to provide permutable optionality to call only pull, only push, push/pull or pull/push having it complete and return to the caller.
  2. Determine, from a list of existing identities, which one to use
    • identities.reduce(...): In recover, I implemented the ability to take the list of identities from await agent.identity.list() where there's 1 or more and .reduce them using the versionId, which is a timestamp, returning the most recent one.

My flow goes:

  1. Run the connect method with sync set to 0s which returns a new agent did, connected did and recovery phrase.
  2. Run recover with the same recoveryPhrase + password with sync set to 0s
  3. The dwnEndpoints are the same on both sides
  4. In the recover method, I run a sync({ direction: 'pull' })
  5. However, The list of identites is still empty

I've also tried just running a .pull() but to no avail. I am somewhat at a loss again. I welcome thoughts and ideas @LiranCohen @csuwildcat

@csuwildcat
Copy link
Contributor

@frankhinek it's hard for me to diagnose what's going on that's blocking this from working, do you see anything that stands out?

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

Ok, after some deep investigation, I made some updates; however, I am still unable to recover any connected dids but I discovered something that may be part of the problem:

When sync is not set in .connect(), it defaults to 2m. From what I can tell, that delays the first sync for 2m as well as subsequent syncs, meaning it does not do an initial sync to remote before setting the interval. startSync is called without an await in connect() which makes sense because you want the code execution to start the sync and continue to return the Web5 object. However, in recover, I need to have sync complete and then use the synced data to reestablish a connected did and return a Web5 object to the caller. If startSync is awaited, it never completes since its set on an interval. The order of startSync forces users to push first then pull. Two things come to mind as potential issues here.

  1. During recover, I can't easily do a discrete sync from remote to local, and continue executing code to return the Web5 connection to the caller
  2. startSync as its written forces push before pull and if not awaited, does not afford the ability to get a complete snapshot between the two states (local and remote) before creating and returning the Web5 object
  3. Idk what idk here, but it dawned on me that its possible the sync direction / order is forcing an overwrite of the remote data in the DWN. The newly recovered local data is blank since its just been newly recovered, if I push first, I imagine that pushing empty the empty registeredIdentites from the blank, local datatstore to the remote DWN.

Latest Changes

Based on this thinking, the following reqs make sense for recovery:

  1. Sync local/remote with a push, pull, push/pull or pull/push in a discrete, one-time manner

    • sync method: does a one-time push/pull or pull/push (depending on the arg passed) resolving the promise back to the caller. sync takes an arg for { direction: string }, which defaults to push to be in-line with the pattern established by startSync. Can be called as such sync({ direction: 'pull' }) to force a pull first.
    • push & pull methods exposed: can be used to call .pull() or .push() alone, call .pull() and .push() back-to-back, or .push() and .pull() back-to-back and have each of these calls complete in a discrete manner. My thinking with this pattern is to provide permutable optionality to call only pull, only push, push/pull or pull/push having it complete and return to the caller.
  2. Determine, from a list of existing identities, which one to use

    • identities.reduce(...): In recover, I implemented the ability to take the list of identities from await agent.identity.list() where there's 1 or more and .reduce them using the versionId, which is a timestamp, returning the most recent one.

My flow goes:

  1. Run the connect method with sync set to 0s which returns a new agent did, connected did and recovery phrase.
  2. Run recover with the same recoveryPhrase + password with sync set to 0s
  3. The dwnEndpoints are the same on both sides
  4. In the recover method, I run a sync({ direction: 'pull' })
  5. However, The list of identites is still empty

I've also tried just running a .pull() but to no avail. I am somewhat at a loss again. I welcome thoughts and ideas @LiranCohen @csuwildcat

I want to make a note here that I reorganized this comment to be a bit more clear and reflective of my latest changes and process.

@LiranCohen
Copy link
Member

When sync is not set in .connect(), it defaults to 2m. From what I can tell, that delays the first sync for 2m as well as subsequent syncs, meaning it does not do an initial sync to remote before setting the interval

Yeah it's designed on an interval and only kicks off after the first timeout currently until a live sync is put into place.
I think exposing a sync() method that allows you a one-shot sync without any intervals is the way to go here, and then startSync can likely leverage that method within it's interval.

I also was wondering if maybe the sync direction / order is overwriting the newly recovered, blank local leveldb DATA to remote

I'm not sure that this is your problem, because of the way the DWN is designed sync in either direction is intended to create the same state between both (or all) parties. So only if an explicit delete is created will a record be deleted. So a blank local couldn't overwrite a remote.

It's hard to say what's happening without debugging a bit further, ie: when sync is doing a MessagesQuery is it recieving anything?

Also I did a quick glance at the code and saw you added an async to startSync, you can't really await startSync because it will only resolve when an error happens (by design). I think awaiting on sync is the way to go.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

When sync is not set in .connect(), it defaults to 2m. From what I can tell, that delays the first sync for 2m as well as subsequent syncs, meaning it does not do an initial sync to remote before setting the interval

Yeah it's designed on an interval and only kicks off after the first timeout currently until a live sync is put into place. I think exposing a sync() method that allows you a one-shot sync without any intervals is the way to go here, and then startSync can likely leverage that method within it's interval.

I also was wondering if maybe the sync direction / order is overwriting the newly recovered, blank local leveldb DATA to remote

I'm not sure that this is your problem, because of the way the DWN is designed sync in either direction is intended to create the same state between both (or all) parties. So only if an explicit delete is created will a record be deleted. So a blank local couldn't overwrite a remote.

It's hard to say what's happening without debugging a bit further, ie: when sync is doing a MessagesQuery is it recieving anything?

I see. That makes a lot of sense. I was also wondering if everything was being sent properly during sync but wasn't sure where exactly to look to debug that. I'll take a look at the sync code closer and see if I can do some logging.

Also I did a quick glance at the code and saw you added an async to startSync, you can't really await startSync because it will only resolve when an error happens (by design). I think awaiting on sync is the way to go.

Yeah, good point. I think my brain saw a returning Promise and defaulted to "shouldn't that be async?" Removed that. I agree, though, that awaiting startSync is not the right move given the pattern. That said, I think it's still useful to have a discrete sync method that takes a direction arg and to expose push and pull for finer-grained control.

@LiranCohen
Copy link
Member

I see. That makes a lot of sense. I was also wondering if everything was being sent properly during sync but wasn't sure where exactly to look to debug that. I'll take a look at the sync code closer and see if I can do some logging.

A very basic overview of how sync works is as follows:

Pulling from remote:
Issue a MessagesQuery to the remote DWN, getting back an array of messageCids, loading those into a queue.
Iterate the queue and do a MessagesRead on each messageCid and process the message locally, removing it from the queue and continuing.

Pushing to remote:
Issue a MessagesQuery to the LOCAL DWN, getting back an array of messageCids, loading those into a queue.
Iterate the queue and snd the messages to the remote DWN if succesful removing them from the queue and continuing.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

Thanks for the rundown. That's helpful. I'll take a look at this and the sync code.

Questions

  1. When the registeredIdentites are pushed to the DWN, how are they stored? As individual records? As one record?
  2. What exactly is stored? The entire did? Or just the URI? It seems like its only storing the URI.
  3. Does the HdIdentityVault have the key material for these child, connected dids? Or does it not need it because all of them are managed by the agent?
  4. If the latter from 3 is true (i.e. the agent has delegated rights for signing etc.)
    a. What if a user wants to export a child identity to make it an agent of its own?
    b . What if the agent wants to give the user of the connected identity the ability to port it over to be sovereign without the agent? Is that an option?

@frankhinek
Copy link
Contributor

@frankhinek it's hard for me to diagnose what's going on that's blocking this from working, do you see anything that stands out?

I don't see any immediate issues and expect this will take some in depth examination of exactly what is attempting to be done in order to assess the best approach.

It would likely be helpful to write out in words the desired sequence of events that should be happening for the recover operation.

Probably also a good idea to break this up into multiple PRs. Have 1 that just adds the service endpoints... and then work on follow-ups that introduce new features.

@LiranCohen
Copy link
Member

  1. When the registeredIdentites are pushed to the DWN, how are they stored? As individual records? As one record?
    Yes the registerIdentity method for the syncApi will just register a DID to include in the sync process.
  1. What exactly is stored? The entire did? Or just the URI? It seems like its only storing the URI.
    So there are several APIs and each are responsible for storing different data. The DIDApi will store DID information + the actual private key within the keyManager. The IdentityApi stores IdentityMetadata associated with an identity.

When the IdentityApi.create() is called it issues a create to the DidApi which creates a DID, stores its info, and stores the private key in the key manager. Then Identity metadata is stored.

  1. Does the HdIdentityVault have the key material for these child, connected dids? Or does it not need it because all of them are managed by the agent?

As far as I understand it the HDIdentityVault only creates a single DID, and that DID is used to manage the agent and store DIDs/Identities within the agent's stores. So I don't think the vault has any information about the other DIDs afaik, maybe @frankhinek can chime in.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

@frankhinek it's hard for me to diagnose what's going on that's blocking this from working, do you see anything that stands out?

I don't see any immediate issues and expect this will take some in depth examination of exactly what is attempting to be done in order to assess the best approach.

It would likely be helpful to write out in words the desired sequence of events that should be happening for the recover operation.

I agree. Outlining the requirements would be helpful. I've sort of been winging it and coding to requirements in my head that change as I test my thinking, which can get messy.

Probably also a good idea to break this up into multiple PRs. Have 1 that just adds the service endpoints... and then work on follow-ups that introduce new features.

I also agree here. Tbh, I really only needed the registering of the agent did for the DCX project to move forward. Right now, I'm somewhat blocked. I'll slim down this PR to only the original code that registered the agent did with service endpoints. Then I'll create another PR that builds on that with the remainder of the code I wrote.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 12, 2024

  1. When the registeredIdentites are pushed to the DWN, how are they stored? As individual records? As one record?

Yes the registerIdentity method for the syncApi will just register a DID to include in the sync process.

  1. What exactly is stored? The entire did? Or just the URI? It seems like its only storing the URI.

So there are several APIs and each are responsible for storing different data. The DIDApi will store DID information + the actual private key within the keyManager. The IdentityApi stores IdentityMetadata associated with an identity.

When the IdentityApi.create() is called it issues a create to the DidApi which creates a DID, stores its info, and stores the private key in the key manager. Then Identity metadata is stored.

I may have asked the question poorly. I'm wondering what is stored in the DWN during the sync process? Registering a did adds it to the registeredIdentites sublevel, and then getSyncPeerState is called during push which grabs all the keys from registeredIdentites

const registeredIdentities = await this._db.sublevel('registeredIdentities').keys().all();

I'm trying to parse through the rest of the push method, but struggling to understand how these identities are stored in the DWN. As records? As messages?

  1. Does the HdIdentityVault have the key material for these child, connected dids? Or does it not need it because all of them are managed by the agent?

As far as I understand it the HDIdentityVault only creates a single DID, and that DID is used to manage the agent and store DIDs/Identities within the agent's stores. So I don't think the vault has any information about the other DIDs afaik, maybe @frankhinek can chime in.

Thanks for clarifying. The real questions behind the questions are:

  1. Does the agent recover the private key material for each connected did from the dwn?
  2. Or does it not need to because the agent has been delegated the rights to manage that connected did such that the agent can sign on its behalf?
  3. If 2 is true, then are we only recovering the list of connected did URIs from the DWN?

@bnonni bnonni force-pushed the web5-agent/register-did-service-endpoints branch from dbeb97d to 348ee12 Compare July 12, 2024 17:04
@bnonni bnonni force-pushed the web5-agent/register-did-service-endpoints branch from 348ee12 to dcac4ad Compare July 12, 2024 23:57
@bnonni
Copy link
Contributor Author

bnonni commented Jul 16, 2024

This PR has been updated to only register the agent did with service endpoints. I haven't submitted a subsequent PR yet for recover but will do that this week.

@bnonni
Copy link
Contributor Author

bnonni commented Jul 19, 2024

Closing as this PR is now unnecessary

@bnonni bnonni closed this Jul 19, 2024
@bnonni
Copy link
Contributor Author

bnonni commented Jul 30, 2024

Reopening on direction from @LiranCohen and @csuwildcat. I mis-interpreted the implementation of DwnRegistrar as fulfilling the need to register the agent did with a dwn service.

@bnonni bnonni reopened this Jul 30, 2024
@bnonni bnonni mentioned this pull request Aug 14, 2024
Copy link
Member

@LiranCohen LiranCohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ant to get this merged in! Good work, just a couple of comments and a question for @frankhinek.

packages/agent/src/hd-identity-vault.ts Outdated Show resolved Hide resolved
packages/api/src/web5.ts Show resolved Hide resolved
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 78.43137% with 11 lines in your changes missing coverage. Please review.

Project coverage is 93.40%. Comparing base (9f08161) to head (bb62d8d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
- Coverage   93.42%   93.40%   -0.03%     
==========================================
  Files         118      118              
  Lines       33134    33157      +23     
  Branches     2649     2650       +1     
==========================================
+ Hits        30956    30969      +13     
- Misses       2138     2149      +11     
+ Partials       40       39       -1     
Components Coverage Δ
agent 87.38% <68.57%> (-0.08%) ⬇️
api 99.58% <100.00%> (+0.02%) ⬆️
common 98.68% <ø> (ø)
credentials 94.95% <ø> (ø)
crypto 93.79% <ø> (ø)
dids 97.77% <ø> (ø)
identity-agent 96.42% <ø> (ø)
crypto-aws-kms 100.00% <ø> (ø)
proxy-agent 96.42% <ø> (ø)
user-agent 96.57% <100.00%> (+0.13%) ⬆️

@LiranCohen LiranCohen merged commit 3da24db into TBD54566975:main Sep 2, 2024
35 checks passed
@bnonni bnonni deleted the web5-agent/register-did-service-endpoints branch September 3, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants