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: handle ipfs messages #47

Merged
merged 8 commits into from
Oct 6, 2022
Merged

feat: handle ipfs messages #47

merged 8 commits into from
Oct 6, 2022

Conversation

il3ven
Copy link
Collaborator

@il3ven il3ven commented Sep 26, 2022

This PR adds the ability to handle IPFS messages which are defined in neume-network/schema#40.

The extraction-worker accepts native IPFS URLs like ipfs://<cid> and it will be the responsibility of the strategy to format the request as a native URL. For eg. soundxyz will format pinata URLs. This will also ensure that all the IPFS URLs in music-os are native.

Currently we are not running our own IPFS node therefore a gateway is required in each message.

The extraction-worker also validates CID before sending a request. This will prevent us from sending unnecessary requests and purge bad IPFS URLs. In future, we can also do #45.

This PR also removes @neume-network/message-schema package as that has been deprecated.

Note: In order for tests to pass in GitHub actions we need to merge neume-network/schema#40. Locally all the tests pass.

@il3ven il3ven linked an issue Sep 27, 2022 that may be closed by this pull request
Copy link
Collaborator

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

  • fix unit tests
  • rebase against main branch

} else if (type === "ipfs") {
let { url, gateway, timeout: timeoutFromMsg } = message.options;

const nativeIPFSPattern = /^(ipfs:\/\/)([^/?#]+)(.*)/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this regex could be part of the schema package or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This can be part of the schema package but we will need it here too. Because we are using it to split the IPFS message into protocol, hash and CID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

potentiall we could also use a fork if is-ipfs that spits out the CID

src/api.mjs Outdated Show resolved Hide resolved
This commit also removes @neume-network/message-schema because it has
been deprecated.

Fixes #33
As we have deprecated message-schema package. The `workerMessage` object
is now exported from the schema package.
The CI will still not pass because we need to merge
neume-network/schema#40 first.
@il3ven
Copy link
Collaborator Author

il3ven commented Oct 4, 2022

@TimDaub The unit tests will pass after neume-network/schema#40 has been merged. I have tested the PR locally using npm link and the tests do pass.

We can also see in the latest logs that only the tests related to IPFS fail because they can't validate the message.

Worth noting that the package-lock.json has a lot of changes because I use a newer version of npm i.e. 8.9.0 which automatically modified the package-lock from version 1 to 2.

package.json Outdated Show resolved Hide resolved
@TimDaub
Copy link
Collaborator

TimDaub commented Oct 4, 2022

we need to update package-lock.json to have the unit tests pass

@TimDaub TimDaub self-assigned this Oct 6, 2022
@il3ven
Copy link
Collaborator Author

il3ven commented Oct 6, 2022

@TimDaub Tests are still failing. I am checking the reason right now.

@TimDaub TimDaub assigned il3ven and unassigned TimDaub Oct 6, 2022
This change was prompted by an update in neume-network/schema.
@il3ven
Copy link
Collaborator Author

il3ven commented Oct 6, 2022

@TimDaub This PR is now ready to be merged. Just needed to change url to uri because of change in schema.

@TimDaub
Copy link
Collaborator

TimDaub commented Oct 6, 2022

awesome thanks

@TimDaub TimDaub merged commit dce11f0 into main Oct 6, 2022
@il3ven il3ven deleted the 11/ipfs branch October 6, 2022 18:46
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.

allow downloading data from ipfs://
2 participants