add sdk storage interface #394

adam-maj · 2022-05-12T16:58:51Z

No description provided.

jnsdls

left an initial batch of reviews

src/contracts/nft-drop.ts

src/core/classes/delayed-reveal.ts

jnsdls · 2022-05-13T02:59:25Z

src/core/classes/ipfs-storage.ts

+  constructor(
+    gatewayUrl: string = DEFAULT_IPFS_GATEWAY,
+    uploader: IStorageUpload = new IpfsUploader(),
+  ) {
    this.gatewayUrl = `${gatewayUrl.replace(/\/$/, "")}/`;
+    this.uploader = uploader;


I would prefer if we could not have 2 params here, can the gatewayUrl be part of the IpfsUploader() interface and then we just pass that in here by itself? this api just doesn't feel clean

These are separate concerns though - the uploader is just for uploading files, whereas the gateway is for file download - these are two separate things may want to specify (ie I want filebase upload + pinata gateway download) - it doesn't seem right for gateway to be on the uploader considering its used in the main IpfsStorage interface.

Also fwiw, this is an internal interface - the public storage interface is now different (src/core/classes/storage.ts)

src/core/classes/ipfs-storage.ts

joaquim-verges

so nice to have upload progress now 🔥 xhr FTW

src/contracts/nft-drop.ts

joaquim-verges · 2022-05-13T04:38:11Z

src/core/classes/storage.ts

+    return this.storage.get(hash);
+  }
+
+  public async upload(data: FileOrBuffer[] | JsonObject[]): Promise<string> {


hmmm can this be (FileOrBuffer | JsonObject)[] | (FileOrBuffer | JsonObject)

so i can pass a single thing if i want to?

also the return type should give me both the base uri and the individual uris i think right?

joaquim-verges

just a couple of nits - let's merge an iterate

joaquim-verges · 2022-05-13T20:28:48Z

src/constants/urls.ts

+/**
+ * @internal
+ */
+export const TW_FILEBASE_SERVER_URL = "http://localhost:3002";


what is this used for?

joaquim-verges · 2022-05-13T20:33:42Z

src/core/classes/storage.ts

+   */
+  public async upload(
+    data: FileOrBuffer[] | JsonObject[] | FileOrBuffer | JsonObject,
+  ): Promise<string> {


would still be nice to also return the individual uris here, or at least something like the total number of files uploaded?

src/core/classes/storage.ts

…nto am/ipfs

lgtm-com · 2022-05-14T19:13:47Z

This pull request introduces 1 alert when merging 27fa534 into 79d199d - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

joaquim-verges · 2022-05-16T04:17:56Z

src/core/classes/ipfs-storage.ts

 /**
 * IPFS Storage implementation, accepts custom IPFS gateways
 * @public
 */
 export class IpfsStorage implements IStorage {
  private gatewayUrl: string;
  private failedUrls: string[] = [];
+  public uploader: IStorageUpload;


this doesn't need to be public right?

joaquim-verges · 2022-05-16T04:19:02Z

src/core/interfaces/IStorage.ts

-   * @returns - The one time use token that can be passed to the Pinata API.
-   */
-  getUploadToken(contractAddress: string): Promise<string>;
+  uploader: IStorageUpload;


also doesn't need to be in the interface, it's an implementation detail

joaquim-verges · 2022-05-16T04:20:38Z

src/core/storage/ipfs-uploader.ts

+
+    data.append("pinataMetadata", JSON.stringify(metadata));
+
+    if (typeof window === "undefined") {


man i would reaaaaally love this for the thirdweb CLI - what would it take to make it happen?

lgtm-com · 2022-05-16T09:51:59Z

This pull request introduces 1 alert when merging a122f98 into 79d199d - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

joaquim-verges

looks like there was a bad merge in the ThirdwebSDK constructor!

joaquim-verges · 2022-05-26T05:40:05Z

src/core/sdk.ts

-    this.storage = storage;
-    this.deployer = new ContractDeployer(rpc, options, storage);
-    this.wallet = new UserWallet(rpc, options);
+    super(network, options);


what happened to my getProviderForNetwork here?

joaquim-verges · 2022-05-26T05:41:21Z

package.json

@@ -91,6 +92,7 @@
    "form-data": "^4.0.0",
    "keccak256": "^1.0.6",
    "merkletreejs": "^0.2.24",
+    "request": "^2.88.2",


we're not using this are we?

joaquim-verges

bunch of minor things + still an issue in ThirdwebSDK post merge

joaquim-verges · 2022-05-26T15:47:08Z

package.json

@@ -52,6 +52,7 @@
    "@types/lodash.isequal": "^4.5.5",
    "@types/mocha": "^9.1.1",
    "@types/node": "^17.0.0",
+    "@types/request": "^2.48.8",


@adam-maj what about this one? don't see it being used either?

joaquim-verges · 2022-05-26T15:48:54Z

src/core/classes/storage.ts

+  }
+
+  /**
+   * Upload any data to an IPFS directory.


should we add here that we handle pinning for you?

joaquim-verges · 2022-05-26T15:50:29Z

src/core/classes/storage.ts

+   *   fs.readFileSync("file2.png"),
+   *   fs.readFileSync("file3.png"),
+   * ]
+   * const uri = await sdk.storage.upload(files);


would be interesting to also show an example of uploading a single json object and maybe passing the upload progress option (with a disclaimer) to show the different capabilities.

Would also add a comment on what the expected response looks like : ipfs://hash/0, ipfs://hash/1...

can you also add an example uploading just regular JSON, and one example with a progress upload callback?

joaquim-verges · 2022-05-26T15:51:55Z

src/core/storage/ipfs-uploader.ts

+  // eslint-disable-next-line @typescript-eslint/no-var-requires
+  globalThis.FormData = require("form-data");
+}
+export class IpfsUploader implements IStorageUpload {


lets mark this class as @internal for now. It's not added to the index.ts of the folder is it?

joaquim-verges · 2022-05-26T15:55:16Z

src/core/classes/storage.ts

+import { UploadProgressEvent } from "../../types/events";
+import { IStorage } from "../interfaces/IStorage";
+
+interface StorageUpload {


this one can be exported though - add it to the index.ts of the folder, and add a top level doc with @public

also, should this interface be moved to IStorageUpload ? also feels like it could be a type instead of an interface, but nbd

joaquim-verges · 2022-05-26T15:58:35Z

src/core/interfaces/IStorage.ts

+/**
+ * @internal
+ */
+export interface UploadBatchResult {


what's the difference between this interface and StorageUpload defined above? looks the same?

joaquim-verges · 2022-05-26T15:59:18Z

src/core/interfaces/IStorageUpload.ts

+  fileNames: string[];
+}
+
+export interface IStorageUpload {


this can be @internal

joaquim-verges · 2022-05-26T16:00:23Z

src/core/sdk.ts

@@ -133,16 +134,22 @@ export class ThirdwebSDK extends RPCConnectionHandler {
   */
  public wallet: UserWallet;

+  /**
+   * Upload and download files from IPFS


add 'or from your own storage service'

joaquim-verges · 2022-05-26T16:01:26Z

src/core/sdk.ts

+    this.storageHandler = storage;
+    this.storage = new Storage(storage);
+    this.deployer = new ContractDeployer(network, options, storage);
+    this.wallet = new UserWallet(network, options);


@adam-maj careful here - you're passingnetwork instead of the transformed provider rpc (should prob be renamed)

this will def break things

Hmm good catch - typing doesn't throw here understandably (maybe because of overlapping types?)

joaquim-verges · 2022-05-26T16:02:39Z

src/types/events.ts

@@ -0,0 +1,11 @@
+export interface UploadProgressEvent {


make sure this gets exported properly (add to types/index.ts)

joaquim-verges · 2022-05-26T20:33:00Z

src/core/classes/storage.ts

+   */
+  uris: string[];
+}
+export class Storage {


@adam-maj you're still not exporting this class for docs, needs to be declared in core/classes/index.ts - also add a top level class documentation 🙏

joaquim-verges · 2022-05-26T20:33:29Z

src/core/classes/storage.ts

+   *   fs.readFileSync("file2.png"),
+   *   fs.readFileSync("file3.png"),
+   * ]
+   * const uri = await sdk.storage.upload(files);


can you also add an example uploading just regular JSON, and one example with a progress upload callback?

joaquim-verges · 2022-05-26T20:34:27Z

src/core/interfaces/IStorage.ts

+    options?: {
+      onProgress: (event: UploadProgressEvent) => void;
+    },
+  ): Promise<StorageUpload>;


i actually kinda liked that the type has Result in it, like StorageUploadResult, but nbd

joaquim-verges · 2022-05-26T20:35:03Z

src/core/interfaces/IStorageUpload.ts

+ * director and the URIs of the uploaded files.
+ * @public
+ */
+export type StorageUpload = {


make sure this is exported in docs (add to core/interfaces/index.ts)

…nto am/ipfs

adam-maj added 9 commits April 28, 2022 14:18

Add filebase temp

f3755d0

Merge branch 'main' into am/filebase

f2bb76a

Merge branch 'main' into am/filebase

6779d97

Storage

70a1063

Add uploader support

f89e410

Merge branch 'main' into am/filebase

02282aa

Remove filebase uploader

6cdc19c

Remove filebase storage

99d6c6b

Preserve file names on upload

d8f2091

adam-maj changed the title ~~am/ipfs~~ ipfs bug fixes May 12, 2022

adam-maj added 3 commits May 12, 2022 17:44

Add browser only upload progress support

9aaa556

Add browser support for file upload progress

59cfbb3

Add SDK storage interface

210edf1

jnsdls suggested changes May 13, 2022

View reviewed changes

joaquim-verges reviewed May 13, 2022

View reviewed changes

adam-maj added 3 commits May 13, 2022 13:01

Add support for string with buffer to upload interface

c4fdac7

Fix listener interface

da72462

Support single file

bc65bd3

joaquim-verges reviewed May 13, 2022

View reviewed changes

joaquim-verges and others added 5 commits May 13, 2022 13:45

Merge branch 'main' into am/ipfs

558af6e

Merge branch 'main' into am/ipfs

b9b433d

Add storage upload interface

80f8300

Merge branch 'am/ipfs' of https://github.com/nftlabs/nftlabs-sdk-ts i…

f8edcae

…nto am/ipfs

Add progress listener to storage upload interface

27fa534

joaquim-verges reviewed May 16, 2022

View reviewed changes

Refactor build form data

a122f98

adam-maj added 2 commits May 16, 2022 03:09

Update uploadBatch interface

30b1c60

Update test

fca1c7a

adam-maj added 5 commits May 25, 2022 13:53

Merge branch 'main' into am/ipfs

a23af2b

Merge ipfs storage

2d05ea6

Merge ipfs storage

d63020d

Add on progress warning

9ce75a9

Merge branch 'main' into am/ipfs

849de63

jnsdls self-requested a review May 26, 2022 02:27

joaquim-verges suggested changes May 26, 2022

View reviewed changes

adam-maj added 2 commits May 26, 2022 02:28

Remove unused import

465dbc5

Fix merge

f76b8c2

joaquim-verges suggested changes May 26, 2022

View reviewed changes

adam-maj added 3 commits May 26, 2022 11:49

Update PR

f65e69d

Add testing for SDK storage interface

98c58ad

Remove metadataUris format

0efc751

adam-maj changed the title ~~ipfs bug fixes~~ add sdk storage interface May 26, 2022

Update mock storage and generate docs

425281a

joaquim-verges reviewed May 26, 2022

View reviewed changes

adam-maj and others added 11 commits May 26, 2022 19:01

Update exports and examples

70bdac9

Merge branch 'main' into am/ipfs

fcdc3b1

Merge branch 'main' into am/ipfs

8d05405

fix conflict, cleanup docs

c3c52ea

fix weird import

d77cb94

Storage

06c9fb0

Merge branch 'am/ipfs' of https://github.com/nftlabs/nftlabs-sdk-ts i…

33df331

…nto am/ipfs

Update to storage handler

4324dad

Merge branch 'main' into am/ipfs

264aaf7

Update to remote storage

3fa622a

Merge branch 'main' into am/ipfs

d4a3f18

joaquim-verges approved these changes May 27, 2022

View reviewed changes

joaquim-verges merged commit 5032ae9 into main May 27, 2022

joaquim-verges deleted the am/ipfs branch May 27, 2022 23:12


		data.append("pinataMetadata", JSON.stringify(metadata));

		if (typeof window === "undefined") {

add sdk storage interface #394

add sdk storage interface #394

Uh oh!

Conversation

adam-maj commented May 12, 2022

Uh oh!

jnsdls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joaquim-verges left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaquim-verges left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgtm-com bot commented May 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented May 16, 2022

Uh oh!

joaquim-verges left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaquim-verges left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment