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

Add ipfs view & ipfs propagate #406

Merged
merged 4 commits into from
Mar 23, 2019
Merged

Conversation

kernelwhisperer
Copy link
Contributor

Closes #384

@kernelwhisperer kernelwhisperer requested a review from 0xGabi March 21, 2019 15:10

Local node:

- ✔️ `aragon ipfs` - Alias for `aragon ipfs start`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a bunch of potential changes here after brainstorming a bit about the ipfs and devchain commands. Would love some feedback! cc: @0xGabi @sohkai @izqui

I think people usually "optimize" their process and run ipfs and devchain in a separate terminal, so that aragon run and other commands are faster. This is why I think we should not stop IPFS/Devchain as part of these commands, but instead expose commands to do so.

Example

IPFS not started, do you wanna start it? [yes/no]
Remember to do aragon ipfs stop afterwards.

And that ipfs start should finish and leave the daemon running in the background.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, having management commands for the daemon would be great! It'd also solve things like the race condition in #383.


E.g.: the [`MiniMeToken`](https://github.com/aragon/aragon-apps/blob/master/shared/minime/contracts/MiniMeToken.sol) contract which snapshots balances at certain block numbers.
## IPFS `@aragon/ipfs-utils`
Copy link
Contributor

Choose a reason for hiding this comment

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

For extensions related to the cli, perhaps it would make sense to prefix them with cli (so this would become @aragon/cli-ipfs-utils)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, makes sense.
What about dao, apm, etc? I think we can publish those without the utils suffix, e.g.: @aragon/cli-dao.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like @aragon/cli-<package> format

- installing & starting a daemon with the right configuration
- interacting with local/remote nodes: authentication, configuration, pinning, etc.
- help setup a "production" node (?)
- automatically pin anything published to an APM repository (?)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't really consider these last two part of the flow; for production you're likely to set up your own infra and the last item is very abusable (we actually used to do this but have since moved away to curating the pinning set).

- Should run `aragon ipfs status` and finish afterwards (optional)
- `aragon ipfs stop` - Stop the **IPFS Daemon**
- Should warn if the node was already stopped
- `aragon ipfs enable-startup` - Start the **IPFS Daemon** automatically at start-up
Copy link
Contributor

Choose a reason for hiding this comment

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

May also make sense to add the ability to clear the IPFS db. Is the plan for us to use our own DB (rather than one the user may already have from a previous IPFS installation)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May also make sense to add the ability to clear the IPFS db.

👍

Is the plan for us to use our own DB (rather than one the user may already have from a previous IPFS installation)?

Hmm, we could totally do this, but also allow it to be the default directory. I wonder what should be the default.. 🤔

function stringifyMerkleDAGNode(merkleDAG) {
// ${merkleDAG.isDir ? '📁' : ''}
const cid = merkleDAG.cid
const name = merkleDAG.name || 'root'
Copy link
Contributor

Choose a reason for hiding this comment

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

When viewing a cid that is a single file the name isn't display, only root. For example:

 aragon ipfs view QmNsZ1b2uXNEYKx17anDvRoJkvvmmeXovz7stKYDEbpRyW
 ✔ Connect to IPFS
 ✔ Fetch the links
─ root - 15.9kB - QmNsZ1b2uXNEYKx17anDvRoJkvvmmeXovz7stKYDEbpRyW

In this example the file is artifact.json. Ideally would be great to display the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK the name of the hash cannot be retrieved. A way to preserve it would be to "wrap" files in a directory, that way, the DAG of the wrapper has a link to the file and it's name.

I think the idea behind it is that I can upload a file called test.json with the exact same contents of artifact.json and it will give me the same hash: QmNsZ1b2uXNEYKx17anDvRoJkvvmmeXovz7stKYDEbpRyW so the data is not duplicated or some other reason 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's why 👌

Copy link
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

Thanks for this new feature Daniel

ipfs propagate is great 🎉

@@ -6,7 +6,7 @@ const publish = require('./apm_cmds/publish')
const devchain = require('./devchain')
const deploy = require('./deploy')
const newDAO = require('./dao_cmds/new')
const startIPFS = require('./ipfs')
const startIPFS = require('./ipfs_cmds/start')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 This one almost slipped... can't wait to have typescript or tests to catch these things.

@kernelwhisperer kernelwhisperer merged commit d4acbc6 into master Mar 23, 2019
@kernelwhisperer kernelwhisperer deleted the feat/ipfs-view-and-propagate branch March 23, 2019 12:56
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.

3 participants