Skip to content

added ipfs resolver wrapper to default config#1475

Merged
dOrgJelli merged 48 commits into
origin-devfrom
ipfs-resolver-wrapper
Feb 11, 2023
Merged

added ipfs resolver wrapper to default config#1475
dOrgJelli merged 48 commits into
origin-devfrom
ipfs-resolver-wrapper

Conversation

@krisbitney
Copy link
Copy Markdown
Contributor

@krisbitney krisbitney commented Jan 10, 2023

This PR embeds the IFPS URI Resolver and IPFS HTTP Client wrappers in the default config, and removes the IPFS and IPFS Resolver plugins. It also cleans up the URIs in the default client config.

The IPFS and IPFS Resolver plugins have been moved to a folder labeled "deprecated" in the IPFS repo: https://github.com/polywrap/ipfs

The wrapper schemas can be viewed here:
IPFS URI Resolver wrapper: https://github.com/polywrap/ipfs/blob/main/uri-resolver/async-uri-resolver/src/schema.graphql
IPFS HTTP Client wrapper: https://github.com/polywrap/ipfs/blob/main/http-client/ipfs-http-client/src/schema.graphql

This PR also updates the UriResolver interface to make the methods accept an optional Env, which can be defined by the implementation. If a wrapper implementation of the interface does not need an env, it can define an empty env type type Env {}. The interface update is published to wrap://ens/wrappers.polywrap.eth:uri-resolver-ext@1.1.0.

Some differences from the IPFS plugin:

  • The env is associated with the ipfs resolver wrapper, whereas before it was associated with the ipfs plugin
  • The HTTP plugin is a dependency of the ipfs resolver wrapper

Adding the ipfs resolver wrapper to a custom client config is slightly different from adding a plugin. An object has been added to the default config to make it easy for users to instantiate an instance of an embedded package.

// use to create a package instance
export const defaultEmbeddedPackages = {
 ipfsHttpClient: (): IWrapPackage => getEmbeddedPackage("ipfs-http-client"),
 ipfsResolver: (): IWrapPackage => getEmbeddedPackage("ipfs-resolver"),
};

In a client config, simply call the method to create a new instance of the package:

export const getDefaultPackages = (): Record<string, IWrapPackage> => {
 return {
   [defaultInterfaces.ipfsHttpClient]: defaultEmbeddedPackages.ipfsHttpClient(),
   [defaultPackages.ipfsResolver]: defaultEmbeddedPackages.ipfsResolver(),
...

Closes #1177

@krisbitney krisbitney marked this pull request as draft January 10, 2023 18:28
@krisbitney krisbitney marked this pull request as ready for review January 11, 2023 10:55
Copy link
Copy Markdown
Contributor

@cbrzn cbrzn left a comment

Choose a reason for hiding this comment

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

i am so happy to see this PR :-) thanks for pushing this Kris 🙏

the only thing that I am still not sure of is that we are storing the ipfs wrappers in the client config builder; even tho I think this is a smart approach because we are embedding, would also make more sense that we push these built wrappers somewhere and just fetch them using the http plugin? (i think that something like this https://github.com/polywrap/rust-client/blob/develop/packages/client/tests/test_uri_resolvers.rs#L42-L54 can be achieved); I am just thinking out loud here and don't have a clear path (because in that example we are pulling from the test harness, but these wrappers like IPFS can not live inside of this repository), but just wanted to share in case someone agree 😛 . Also, I've realized that GitHub is a good source to fetch packages, and it's really used in the software development ecosystem (swift, go)

Ignoring my previous paragraph, I think this looks amazing 😄

@krisbitney
Copy link
Copy Markdown
Contributor Author

i am so happy to see this PR :-) thanks for pushing this Kris 🙏

the only thing that I am still not sure of is that we are storing the ipfs wrappers in the client config builder; even tho I think this is a smart approach because we are embedding, would also make more sense that we push these built wrappers somewhere and just fetch them using the http plugin? (i think that something like this https://github.com/polywrap/rust-client/blob/develop/packages/client/tests/test_uri_resolvers.rs#L42-L54 can be achieved); I am just thinking out loud here and don't have a clear path (because in that example we are pulling from the test harness, but these wrappers like IPFS can not live inside of this repository), but just wanted to share in case someone agree 😛 . Also, I've realized that GitHub is a good source to fetch packages, and it's really used in the software development ecosystem (swift, go)

Ignoring my previous paragraph, I think this looks amazing 😄

That's a fantastic idea! I especially like the idea of pulling it directly from github. I'll update the PR very soon.

@dOrgJelli
Copy link
Copy Markdown
Contributor

dOrgJelli commented Jan 12, 2023

Hey @cbrzn @krisbitney, I'm on the fence about using github via http to fetch the IPFS wrapper each time.

IMO a more web3-friendly way is to have IPFS resolution built-in, and fetch all other URI resolvers (ENS for example) from there.

@cbrzn
Copy link
Copy Markdown
Contributor

cbrzn commented Jan 13, 2023

Hey @cbrzn @krisbitney, I'm on the fence about using github via http to fetch the IPFS wrapper each time.

IMO a more web3-friendly way is to have IPFS resolution built-in, and fetch all other URI resolvers (ENS for example) from there.

@dOrgJelli I don't have a strong opinion about the built-in wrappers for the default build, I think it makes sense to improve the URI resolution performance. But I think we should relay on github to use any other wrappers

@krisbitney krisbitney requested a review from cbrzn January 14, 2023 11:36
@krisbitney
Copy link
Copy Markdown
Contributor Author

@dOrgJelli @cbrzn sounds good. If we embed the ipfs wrappers, we can eventually create an http resolver wasm wrapper in the future and fetch it over IPFS.

I think we should continue looking at solutions for ipfs reliability. For example, we could give users the option to automatically retry resolving a wrapper if it fails. Sometimes ipfs resolution fails even when i know the wrapper is pinned at ipfs.wrappers.io.

…rapper

# Conflicts:
#	packages/interfaces/ipfs/package.json
#	packages/js/plugins/ipfs/package.json
#	packages/js/plugins/uri-resolvers/ipfs-resolver/package.json
@Niraj-Kamdar
Copy link
Copy Markdown
Contributor

@dOrgJelli @cbrzn @krisbitney I really like the idea of having an HTTP URI resolver fetching the IPFS wrapper and URI resolver. I agree some people in web3 may not like fetching using HTTP but we are currently doing the same with wrappers.io which is a centralized solution. The only downside would be the need to fetch the IPFS wrapper which can also be improved by caching.

I remember we discussed different kinds of default config for client config builder. Ex: Polywrap can have polywrapDefaultClientConfig, gelato can have gelatoDefaultClientConfig, tezos can have tezosDefaultClientConfig, decentralizedDefaultClientConfig (don't even use wrappers.io), etc.

We can't have one solution that fits for all so by offering all these different default client config flavors, users can choose what is the most appropriate for their app.

@krisbitney
Copy link
Copy Markdown
Contributor Author

@dOrgJelli @cbrzn @krisbitney I really like the idea of having an HTTP URI resolver fetching the IPFS wrapper and URI resolver. I agree some people in web3 may not like fetching using HTTP but we are currently doing the same with wrappers.io which is a centralized solution. The only downside would be the need to fetch the IPFS wrapper which can also be improved by caching.

I remember we discussed different kinds of default config for client config builder. Ex: Polywrap can have polywrapDefaultClientConfig, gelato can have gelatoDefaultClientConfig, tezos can have tezosDefaultClientConfig, decentralizedDefaultClientConfig (don't even use wrappers.io), etc.

We can't have one solution that fits for all so by offering all these different default client config flavors, users can choose what is the most appropriate for their app.

I'm happy with both solutions.

I think one caveat is that maybe the IPFS resolver should fetch an HTTP resolver wrapper instead. I updated the IPFS wrapper to retry fetches when they fail, and it is working much better.

Maybe we should make wrappers available at multiple URIs so that users can choose HTTP, IPFS, ENS, embedded, etc. ?

…rapper

# Conflicts:
#	packages/js/plugins/uri-resolvers/ipfs-resolver/src/__tests__/e2e.spec.ts
#	yarn.lock
@krisbitney krisbitney requested a review from cbrzn February 3, 2023 15:52
cbrzn
cbrzn previously approved these changes Feb 3, 2023
@krisbitney
Copy link
Copy Markdown
Contributor Author

krisbitney commented Feb 4, 2023

I changed the UX for working with the embedded packages and updated the PR description

cbrzn
cbrzn previously approved these changes Feb 9, 2023
Niraj-Kamdar
Niraj-Kamdar previously approved these changes Feb 10, 2023
Copy link
Copy Markdown
Contributor

@Niraj-Kamdar Niraj-Kamdar left a comment

Choose a reason for hiding this comment

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

LGTM! I like the new UX for embedded

Copy link
Copy Markdown
Contributor

@dOrgJelli dOrgJelli left a comment

Choose a reason for hiding this comment

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

Going to work on removing the problem mentioned below now.

import { ExtendableUriResolver } from "@polywrap/uri-resolver-extensions-js";
import path from "path";
import { WasmPackage } from "@polywrap/wasm-js";
import fs from "fs";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will cause problems in the browser. Instead we should be embedding the raw bytes of the files within a JS module so that reading from a file is not needed.

@dOrgJelli dOrgJelli dismissed stale reviews from Niraj-Kamdar and cbrzn via 76625d6 February 11, 2023 00:12
dOrgJelli
dOrgJelli previously approved these changes Feb 11, 2023
@dOrgJelli dOrgJelli merged commit 38650a9 into origin-dev Feb 11, 2023
@dOrgJelli dOrgJelli deleted the ipfs-resolver-wrapper branch April 10, 2023 17:06
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.

Replace IPFS plugin with IPFS wrapper

4 participants