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

feat: embed provider webrtc maddr into share link #206

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

Conversation

2color
Copy link
Contributor

@2color 2color commented Jan 13, 2025

Background

  • Providing to the DHT is slow and heavily constrained by transports, i.e. most DHT servers are not dialable from the browser
  • Providing from the browser is flakey

What's in this PR

  • Embed maddr into share URL and connect optimistically
  • Fix styling, e.g. PeerID overflow from node-info on mobile
  • Fix node info transport counts: use Circuit.exactMatch && WebRTC.matches in order to correctly display the relay dialable address count, e.g. as a browser you are initially only dialable using a circuit relay, which may be over any of the relay's nodes transports, e.g. QUIC, WebSockets etc.
  • refactor code to remove the pending prop which isn't needed
  • Move side-effects relating to data fetching to the files-provider to encapsulate all needed retrieval logic in a single place
  • Remove the download provider and context. Instead encapsulate this login the "router" (use-current-page.ts) and dispatch an action to the files provider.

TODO

  • Make providing to the DHT optional with a toggle d0fc2c8
  • Test to make sure there aren't any bugs with the encoding of maddr in the url

@2color 2color requested a review from SgtPooki January 13, 2025 13:31
@SgtPooki
Copy link
Member

Just want to say while i'm looking into this that I love the updates to the node info:
image

@SgtPooki
Copy link
Member

SgtPooki commented Jan 23, 2025

maddrs is empty in my share link (I have 0 listen addrs..) so I checked the logs and webrtc is throwing some interesting errors:

libp2p:kad-dht:network:error could not send PING to 12D3KooWA7CeLxjCSxRXbf8sbrMieFpzGBGfp6ABavUx98YsrB9T - WebRTC/DataChannelError: WebRTC transport error: [stream: data] data channel error: Data channel was never opened: state: connecting    at http://localhost:5173/node_modules/.vite/deps/helia.js?v=24dc1a65:38743:18

I believe this is related to libp2p/js-libp2p#2805.. I wonder if I can get things working by dialing a local kubo node directly..

@SgtPooki
Copy link
Member

FYI that Daniel and I were able to successfully share a file using the changes in this PR. reviewing code now

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

love all the clarification jsdoc items around use effects, and the consolidation of a lot of the code. I would love to break up files-provider a lot more, but i think this is a huge net win. I'm good with merging as is but I left some comments and code suggestions

Comment on lines +32 to +33
const decodedMaddrs = maddrs != null ? decodeURIComponent(maddrs).split(',') : null
const multiaddrs = decodedMaddrs != null ? decodedMaddrs.map(maddr => multiaddr(maddr)) : null
Copy link
Member

Choose a reason for hiding this comment

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

nit: I feel like a separate useMemo on a maddrs dep with the fully resolved multiaddrs would be a little better.. but there is likely little difference.. sometimes react can get wonky if you have different effects too separated.

const decodedMaddrs = maddrs != null ? decodeURIComponent(maddrs).split(',') : null
const multiaddrs = decodedMaddrs != null ? decodedMaddrs.map(maddr => multiaddr(maddr)) : null

dispatch({ type: 'fetch_start', cid: maybeCid, filename: decodedFilename, providerMaddrs: multiaddrs })
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to cancel the fetch on unmount. maybe create an abort controller and pass signal to fetch_start, and abort on unmount?

Suggested change
dispatch({ type: 'fetch_start', cid: maybeCid, filename: decodedFilename, providerMaddrs: multiaddrs })
const abrtCtl = new AbortController()
// ...
dispatch({ type: 'fetch_start', cid: maybeCid, filename: decodedFilename, providerMaddrs: multiaddrs, signal: abrtCtl.signal })
return () => {
// cancel fetching
abrtCtl.abort('unmounted effect..')
}

Comment on lines +5 to +6
// TODO: Get only WebRTC addrs dialable from other browsers, e.g. WebRTC Direct, WebRTC Secure WebSockets, and WebRTC WebTransport (currently not included in Helia transports)
return addrs?.filter((addr: Multiaddr) => WebRTC.exactMatch(addr)) ?? []
Copy link
Member

Choose a reason for hiding this comment

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

are you talking about:

Suggested change
// TODO: Get only WebRTC addrs dialable from other browsers, e.g. WebRTC Direct, WebRTC Secure WebSockets, and WebRTC WebTransport (currently not included in Helia transports)
return addrs?.filter((addr: Multiaddr) => WebRTC.exactMatch(addr)) ?? []
return addrs?.filter((addr: Multiaddr) => WebRTC.exactMatch(addr) || (Circuit.exactMatch(addr) && (WebRTCDirect.matches(addr) || WebTransport.matches(addr) || WebSockets.matches(addr) || WebSocketsSecure.matches(addr)))) ?? []

Comment on lines +116 to +117
// Whether or not to provide CIDs to the DHT
provideToDHT: boolean
Copy link
Member

Choose a reason for hiding this comment

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

love this. unfortunately, providing seems to hang forever when this is checked and I add a file.

Also, we will want a TODO or issue for "reproviding" to DHT if this is checked after files have already been added.

Comment on lines +507 to +510
* - For each unpublished file:
* --- Provides the file to the IPFS network
* --- Updates state on success/failure
* - Handles cancellation via AbortController
Copy link
Member

Choose a reason for hiding this comment

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

i haven't confirmed whether this is working for files that are added when "provide to DHT" is not checked, and then the "provide to DHT" is checked.. but we should be kicking off publishing to DHT and updating UI that something is in progress.

@SgtPooki
Copy link
Member

FYI there is a bug here. When i share a single file, and click the folder share (after previously having multiple files.. but after a refresh) I get a link that loads up all the multiple files, even though my share is currently only showing a single file:

Sharer view

image

Downloader view

image

@2color
Copy link
Contributor Author

2color commented Jan 24, 2025

FYI there is a bug here. When i share a single file, and click the folder share (after previously having multiple files.. but after a refresh) I get a link that loads up all the multiple files, even though my share is currently only showing a single file:

I'll take a look at that.

I think the combination of a persisted block/data store and the way we use mfs, causes that to happen naturally (maybe on main too?)

@SgtPooki
Copy link
Member

I think the combination of a persisted block/data store and the way we use mfs, causes that to happen naturally (maybe on main too?)

it totally may be on main.. let me test that out

@SgtPooki
Copy link
Member

it totally may be on main.. let me test that out

its pretty difficult to get main to publish to DHT and get the share link tbh..

@SgtPooki
Copy link
Member

Ok yep, issue exists on main:

Provider

image

Downloader

image

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.

2 participants