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

Add ns record gateway handling #576

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

nitro-neal
Copy link
Contributor

@nitro-neal nitro-neal commented May 16, 2024

When resolving a did dht this change allows it to look at the ns records and find the authoritative gateways.

It will try to use these gateways, and accumulate errors if it fails. It will keep trying until all gateway uris are exhausted and then will return the accumulated errors.

  1. Do a resolution with default gateway to get dns records for did
  2. If there are NS records, get all of them into an array of endpoints
  3. Loop over all endpoints, if you successfully get dns records for did from endpoint, return and continue to resolve did, otherwise try the next endpoint

Reference
https://did-dht.com/#resolution-gateways

Copy link

changeset-bot bot commented May 16, 2024

⚠️ No Changeset found

Latest commit: 6313e60

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

Copy link
Contributor

github-actions bot commented May 16, 2024

TBDocs Report

🛑 Errors: 0
⚠️ Warnings: 1

@web5/api

  • Project entry file: packages/api/src/index.ts
📄 File: ./packages/api/src/web-features.ts
⚠️ extractor:typedoc:missing-docs: installNetworkingFeatures (CallSignature) does not have any documentation.

@web5/crypto

  • Project entry file: packages/crypto/src/index.ts

@web5/crypto-aws-kms

  • Project entry file: packages/crypto-aws-kms/src/index.ts

@web5/dids

  • Project entry file: packages/dids/src/index.ts

@web5/credentials

  • Project entry file: packages/credentials/src/index.ts

TBDocs Report Updated at 2024-05-21T19:10:02Z 6313e60

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

Attention: Patch coverage is 52.23881% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 90.78%. Comparing base (eef9396) to head (6a153e4).
Report is 48 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
- Coverage   91.10%   90.78%   -0.32%     
==========================================
  Files         116      118       +2     
  Lines       29664    29809     +145     
  Branches     2199     2219      +20     
==========================================
+ Hits        27024    27062      +38     
- Misses       2605     2711     +106     
- Partials       35       36       +1     
Components Coverage Δ
agent 80.86% <ø> (ø)
api 94.15% <ø> (-3.64%) ⬇️
common 98.68% <ø> (ø)
credentials 95.26% <ø> (ø)
crypto 93.81% <ø> (ø)
dids 97.22% <52.23%> (-0.48%) ⬇️
identity-agent 96.70% <ø> (ø)
crypto-aws-kms 100.00% <ø> (ø)
proxy-agent 96.70% <ø> (ø)
user-agent 96.70% <ø> (ø)

packages/dids/tests/methods/did-dht.spec.ts Dismissed Show dismissed Hide dismissed
packages/dids/tests/methods/did-dht.spec.ts Dismissed Show dismissed Hide dismissed
@nitro-neal nitro-neal marked this pull request as ready for review May 17, 2024 18:38
let resolutionGatewayUris = await DidDhtDocument.getAuthoritativeGatewayUris({ didUri, dnsPacket });

// Only do a second retrieval if the authoritative resolution gateway URIs are different from the given gateway URI.
if(!resolutionGatewayUris.includes(gatewayUri)) {
Copy link
Member

Choose a reason for hiding this comment

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

does this work with the . / FQDN syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it takes out the trailing . and the documentation is updated .

The resolutionGatewayUris will be the value in the ns minus the last .

decentralgabe
decentralgabe previously approved these changes May 17, 2024
Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

approving if the condition I mentioned as test coverage

Copy link
Member

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

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

Looks legit, only missing test for the code added.

Also added a small refactoring PR to merge that addresses a few small issues: #608

* minor refactoring

* Fixed an ambiguous if condition
@nitro-neal
Copy link
Contributor Author

I will wait to merge until vector 3 is in, and then I will resolve conflict and finish testing missing lines

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