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

Improve ipfs-pack and upload behaviour with required mapped fields #20

Closed
dohomi opened this issue May 11, 2022 · 14 comments
Closed

Improve ipfs-pack and upload behaviour with required mapped fields #20

dohomi opened this issue May 11, 2022 · 14 comments

Comments

@dohomi
Copy link
Contributor

dohomi commented May 11, 2022

As edition is currently required it would be great to mention this in the README. I wasn't aware of it - and then all filenames are corrupted after running the pack command.

Another solution would be to use the map index instead of the edition field. Is there any downside?

@juliancwirko
Copy link
Owner

So would you like to use the ipfs pack without the previous steps/generation?
Because I think I don't understand how you get the files for the ipfs pack without the edition field.

@dohomi
Copy link
Contributor Author

dohomi commented May 11, 2022

@juliancwirko I created 10K pngs/jsons without the edition field - that seems not an issue.

@juliancwirko
Copy link
Owner

But have you configured a custom metadata structure without 'edition' then?

The assumption was that even if you modify the metadata output, all fields should be used, but it would be good to focus more on how to make some of them optional and not break the whole flow. Of course, making all optional doesn't make sense because at least the ipfs cid for the image/asset should be there. I'll look at this PR and overall logic. Maybe most of the fields could be optional.

@dohomi
Copy link
Contributor Author

dohomi commented May 12, 2022

I haven't looked too much into the generation of the png/json files but it seems on the creation part the edition is not required. So yes I was running the script with an overwritten schema (to fit OpenSea) and did not include any edition. The generator worked but while packing instead of filename 1.png [object object].png would be generated.

@juliancwirko
Copy link
Owner

Understood, I haven't had much time, but I will for sure look into how to make the fields optional on each step. I'll try today.

@dohomi
Copy link
Contributor Author

dohomi commented May 16, 2022

@juliancwirko I think the main issue on the ipfts-pack is that the script we include to upload packed files works but not on a large collection base. 10K items with ~14GB will silently fail on some point.

I'll try to extract the metadata adjustment based on a given IPFS location. NFT UP seems to handle large uploads very well but this requires the upload in 2 separate steps.

@juliancwirko
Copy link
Owner

@dohomi, do you mean the uploading or packing process? Because you wrote about ipfs-pack.
We could check what 'nft up' does with it. It is built with Electron, so the source code regarding packing and uploading should be ready to copy. I haven't had much time lately, but I will try to test it on a more extensive set.

@juliancwirko
Copy link
Owner

juliancwirko commented May 16, 2022

They don't use ipfs-car. If the problem was with packing using this lib, the nft-art-maker should probably pack the CID files using nft.storage, I am not sure yet, but I think the whole process is here: https://github.com/nftstorage/nftup/blob/main/public/electron.js

The problem is that it will need some more work because the nft-art-maker pack does some additional job updating the metadata files. Maybe it won't be challenging to switch to nft. storage, I will try to sit and think about it today evening.

car pack operation: https://github.com/nftstorage/nftup/blob/main/public/electron.js#L105

@juliancwirko
Copy link
Owner

Also there are similar issues in ipfs-car repo: web3-storage/ipfs-car#114

@juliancwirko juliancwirko changed the title Currently "editon" is required in combination with ipfts-pack Currently "editon" is required in combination with ipfs pack May 16, 2022
@dohomi
Copy link
Contributor Author

dohomi commented May 17, 2022

The current pack command creates *.car files of images and metadata and after trying to upload these with the upload command the script never succeeded on the images (probably to the upload size). The main issue was, that there is no process indication on how much data is already sent from the script.

Secondly I was using NFT UP of the images and that succeeded. Now - as the CID changed I would need to re-run a script to change the CID of all metadata files.

I am open to any suggestion - the nftup is very vague as they state you can drop metadata and image in one go - but that would end up with wrong CID's again on the metadata.

[EDIT] I changed the title of this task as it went sideways :-)

@dohomi dohomi changed the title Currently "editon" is required in combination with ipfs pack Improve ipfs-pack and upload behaviour and required mapped fields May 17, 2022
@dohomi dohomi changed the title Improve ipfs-pack and upload behaviour and required mapped fields Improve ipfs-pack and upload behaviour with required mapped fields May 17, 2022
@juliancwirko
Copy link
Owner

Ok, so I think there are two problems:

  1. We need an additional command to update the CIDs on demand by providing the proper CID (primary metadata and each file metadata JSON files). We need to loop through the files and change fields. This would be an option outside the normal flow when you don't want to use the nft-art-maker pack functionality.
  2. We need to research nft up and check where it is different. I think it should also work through CLI. Maybe ipfs-car produces different .car files or something. Nft up also packs the files into .car, but they don't use the ipfs-car lib. For now, this is the only difference I found, but I need to dig deeper.

@juliancwirko
Copy link
Owner

@dohomi
Copy link
Contributor Author

dohomi commented May 18, 2022

@juliancwirko I can look into #22

@dohomi
Copy link
Contributor Author

dohomi commented May 22, 2022

@juliancwirko great improvement for the upload functionality! I will close this issue now because we handle all subtasks already in tickets or PRs

@dohomi dohomi closed this as completed May 22, 2022
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

No branches or pull requests

2 participants