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] NFT.storage continue... #159

Merged
merged 27 commits into from Aug 19, 2022
Merged

[feat] NFT.storage continue... #159

merged 27 commits into from Aug 19, 2022

Conversation

melMass
Copy link
Member

@melMass melMass commented Aug 12, 2022

Fixing issues due to the IPFS switch and older unreported ones:

  • Cannot edit profile
    • Works but there is absolutely no feedback...
  • MD preview / styling -> fixed displayURi issue, styling should be done is a dedicated PR
  • Subfolders for generative tokens (linked to fix: ⚡️ use nft.storage directory API ipfs-upload-proxy#3)
  • Clean the folders before upload? (macOS creates .DS_Store files and ._. prefixed files, windows has ThumbDB), might be "dangerous" to alter the archive

@render
Copy link

render bot commented Aug 12, 2022

@melMass melMass force-pushed the mel/fix/nft-storage-continue branch from 801abdb to 15c5184 Compare August 12, 2022 12:30
@melMass melMass linked an issue Aug 12, 2022 that may be closed by this pull request
@melMass
Copy link
Member Author

melMass commented Aug 12, 2022

I was about to remove every instance of IPFS as we use various gateways.
@NoRulesJustFeels while doing that I wonder if this won't make reaching gateways limits faster?

For now I just changed the SUBJKT icon (was DWEB -> IPFS.io), keeping the current gateway used by others, we can switch easily once we decide

Also add NFTStorage as suggested by NRJF in #150.
@NoRulesJustFeels
Copy link
Contributor

As part of the discussion on #150, there are various IPFS options to consider. We should probably spend some time testing those when we have access. We might even consider some kind of failover solution for the client if the rate limit is reached for a particular gateway.

@melMass
Copy link
Member Author

melMass commented Aug 12, 2022

As part of the discussion on #150, there are various IPFS options to consider. We should probably spend some time testing those when we have access. We might even consider some kind of failover solution for the client if the rate limit is reached for a particular gateway.

I was not up to date with that issue! Awesome

@melMass melMass force-pushed the mel/fix/nft-storage-continue branch from 6393ea5 to efdd92b Compare August 12, 2022 13:39
This was setting a .gravatar property to the parsed metadatafile.
Does not make sense and is not used anywhere.
The optional second param for HashToURL and CIDToURL
allows you to override the IPFS gateway to use.
- use const where it makes sense (easier to read)
- fixed the getBalance function even if it should not be there...
- added a fetchJSON util function.
@melMass
Copy link
Member Author

melMass commented Aug 13, 2022

Added a few things:

  • Check the balance on the mint page: This displays if you have less than 0.15Tz (6Tz for the screenshot)
    image

  • Check for existing SUBJKT: This is an ongoing issue people had as there is no check before making the contract call so it only failed in Temple/Kukai not before. Now it's showing the owner of the name:
    image

  • Replaced alert() with the context feedback:
    image

  • All IPFS CIDs and Hashes now use dedicated util functions shared across to convert them to http gateway links. The second argument to specify the gateway is optional and by default uses a new dedicated ENV Variable (REACT_APP_IPFS_DEFAULT_GATEWAY). A few different gateways (DWeb, Cloudflare, Ipfs.io, Infura) where used before.

@melMass melMass linked an issue Aug 13, 2022 that may be closed by this pull request
@melMass melMass force-pushed the mel/fix/nft-storage-continue branch from 94ad636 to fc103e2 Compare August 15, 2022 21:26
@melMass
Copy link
Member Author

melMass commented Aug 16, 2022

Ah ok... The CI is not building to the stage URL since quite a few commits, related to this somehow even if we use npm everywhere
dilanx/craco#435

Copy link
Contributor

@xat xat left a comment

Choose a reason for hiding this comment

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

left a few comments, nothing dramatic :)

@@ -1,14 +1,18 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

out of interest: why the changes here?

src/components/collab/show/CollabDisplay.js Outdated Show resolved Hide resolved
src/components/media-types/html/index.js Outdated Show resolved Hide resolved
src/components/media-types/index.js Show resolved Hide resolved
src/components/media-types/video/index.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
const buffer = Buffer.from(await file.arrayBuffer())
console.log(buffer)
this.setState({ identicon: 'ipfs://' + (await ipfs.add(buffer)).path })
const picture_cid = await uploadFileToIPFSProxy({
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure, but can't file just be passed directly to uploadFileToIPFSProxy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably!

This was a bit of the trouble in this PR, that's why I roughly typed it @typedef { {path: string?, blob: Blob} } FileHolder, extracting it from the old code and it's usage mainly to make sense of the code. I would happily clean it further in a separate branch/PR later

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, yeah, no need that we need to block merging this pr because of that.

@@ -125,6 +127,36 @@ export const ObjktDisplay = () => {
}, [])

const Tab = TABS[tabIndex].component

const objkt_classes = []
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this needs to be put in useState.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't making the useEffect depend on nft alleviates that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done all besides this as I'm not sure ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, well, if it works, it works :)

as soon as you add something async to the effect, it will probably break though.

@melMass
Copy link
Member Author

melMass commented Aug 18, 2022

left a few comments, nothing dramatic :)

Thank youu, making the edits now!

@xat
Copy link
Contributor

xat commented Aug 19, 2022

i manually tested the "Cannot edit profile" fix, it worked 👍

if there is nothing left otherwise, i can merge.

@melMass
Copy link
Member Author

melMass commented Aug 19, 2022

i manually tested the "Cannot edit profile" fix, it worked 👍

if there is nothing left otherwise, i can merge.

Yep it solves a few important things like subfolders for generative and profile editing. Further edits can be made in another one ;)

Copy link
Contributor

@xat xat left a comment

Choose a reason for hiding this comment

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

great work!

@xat xat merged commit 1c387d9 into main Aug 19, 2022
@xat xat deleted the mel/fix/nft-storage-continue branch August 19, 2022 14:30
@melMass melMass linked an issue Aug 26, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants