feat: improve ipfs media download when file its in folder #7752
Conversation
brancoder
left a comment
brancoder
left a comment
There was a problem hiding this comment.
There seams to be an issue with encoding this example ipfs://bafybeigbmawzzda64hrr2zenodmivfandhkjsrf7nkcic52y5wb6nxywja
|
|
||
| export async function getIpfsUri(link: { path?: string; hash: string }): Promise<string | undefined> { | ||
| try { | ||
| const slicedLink = link.hash.slice('https://ipfs.io/'.length) |
There was a problem hiding this comment.
We could use the the constant IPFS_ENDPOINT here instead of hardcoded string
| const nft = state[accountIndex].find((_nft) => _nft.id === nftId) | ||
| if (nft) { | ||
| const downloadUrl = nft.downloadUrl | ||
| void getIpfsUri({ hash: downloadUrl }).then((ipfsUri) => { |
There was a problem hiding this comment.
Should we check if the downloadUrl is a ipfs link before calling getIpfsUri for every nft? wdyt?
| nft.composedUrl = ipfsUri | ||
| } | ||
| }) | ||
| if (downloadUrl) { |
There was a problem hiding this comment.
I was thinking something along the lines like this check we have in explorer: https://github.com/iotaledger/explorer/blob/dev/client/src/helpers/hooks/useNftMetadataUri.ts#L22
Looks to me that we now try to getIpfsUri even if downloadUrl is not ipfs link.
This one is a folder inside a folder, I would ignore it, we should just check 1 level |
| const ipfsPrefix = 'ipfs' | ||
|
|
||
| if (url?.includes(ipfsPrefix)) { | ||
| return url.slice(ipfsPrefix.length) |
There was a problem hiding this comment.
This method now returns url with first 4 characters sliced ex. s://ipfs.io/ipfs....... instead of ipfs hash.
begonaalvarezd
left a comment
begonaalvarezd
left a comment
There was a problem hiding this comment.
Im requesting changes just to block this PR from being merged, I would like to have a security expert have a look at it before we add it to production builds 🙏🏼
| @@ -0,0 +1,7 @@ | |||
| export function getIPFSHash(url?: string): string | undefined { | |||
| const ipfsPrefix = 'https://ipfs.io' | |||
There was a problem hiding this comment.
There are other gateways apart from ipfs.io, this isn't very robust imo
| const ipfsPrefix = 'https://ipfs.io' | ||
|
|
||
| if (url?.includes(ipfsPrefix)) { | ||
| return url.slice(ipfsPrefix.length) |
There was a problem hiding this comment.
Would it be better to use the URL module? Then we can also address @marc2332 's comment. If we have this URL:
https://cloudflare-ipfs.com/ipfs/bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Vincent_van_Gogh.html
and we want to get the bafy... part I think we could do this:
const ipfsUrl = new URL("https://cloudflare-ipfs.com/ipfs/bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Vincent_van_Gogh.html")
const path = ipfsUrl.pathname
return path.replace("/ipfs/", "").split("/")[0]
|
🫡 |
Summary
...
Changelog
Testing
Platforms
Instructions
...
Checklist