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

fix: interoperability with go repo #173

Merged
merged 5 commits into from
Aug 9, 2018

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jul 21, 2018

When a go-ipfs daemon runs the init command, it creates a datastore_spec file and the config file has the datastore property on it. This way, when a go daemon stars, it expects to have those two in its repo.

Aiming to have interoperability between go and ipfs repo (interop#26 and interop#28), the js-ipfs repo has to have both in its repo.

  • Add datastore default config to js-ipfs js-ipfs#1460 (instead of directly here)
  • Add running test in ipfs/interop for the repo interoperability interop#29

Fixes #163

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

If i customize the storageBackends when creating the repo, https://github.com/ipfs/js-ipfs-repo#new-repopath-options, it should probably fail if the datastore.Spec file does not match.

This PR is supporting the default and gives us the ability to customize, but it also lets the actual repo behavior differ from the spec file. If go runs on a repo that js has a custom config for and the spec file doesn't reflect that, bad things will probably happen.

@vasco-santos
Copy link
Member Author

Yeah @jacobheun ! That is one of the problems that I would like to discuss. The ability to customize may be problematic because go-ipfs has different types of datastores, which are still not available on js-ipfs.

@jacobheun
Copy link
Contributor

That's the downside of customizing, you start losing compatibility. Out of the box we want js and go to interop without issue, but as people change that default configuration we can't guarantee that's the case anymore. I think it's something we can put in the readme when we update it for this to say, "If you change this, you risk losing ipfs-repo interoperability with other languages (go, etc)".

If we don't recognize the spec or we can't find the matching datastores, sharding methods, etc, it should just throw an incompatibility error and exit. No touching the data if it doesn't know how to handle it properly.

@vasco-santos
Copy link
Member Author

After a productive call with @jacobheun we created an endeavour issue to tackle all the tasks regarding the repo interoperability. This issue aims to tackle the following set of tasks:

3) ipfs/js-ipfs-repo

  • 1. Throw an error on invalid / inconsistent configs
    • INIT: create the repo based on the config received (verify if the storageBackends exists and if so, guarantee that it matches the spec). If is is not possible to create the repo an error should be thrown to inform that the options received are not valid.
    • Start: verify if the repo that exists is compliant with the configuration. If not, throw an error
  • 2. transition for using spec instead of the current programatic approach (support both to avoid breaking existing projects, but deprecate the current format)

@vasco-santos vasco-santos force-pushed the fix/interoperability-with-go-repo branch from f5a81d4 to 7a75adc Compare August 9, 2018 13:07
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

I tested these agains the interop https://github.com/ipfs/interop/pull/29/files unskip updates and they look good. This doesn't include 2.2, 3.1 and 3.2 from the awesome endeavor. We will need to include those in the next piece of the update, but this gets us our initial interop functionality.

@jacobheun jacobheun merged commit cef6a3d into master Aug 9, 2018
@jacobheun jacobheun deleted the fix/interoperability-with-go-repo branch August 9, 2018 14:34
@ghost ghost removed the status/in-progress In progress label Aug 9, 2018
@vasco-santos vasco-santos changed the title WIP: fix: interoperability with go repo fix: interoperability with go repo Aug 9, 2018
alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Aug 29, 2018
A working version of **IPNS working locally** is here!  🚀 😎 💪 

Steps:

- [x] Create a new repo (**js-ipns**) like it was made with go in the last week ([go-ipns](https://github.com/ipfs/go-ipns)) and port the related code from this PR to there
- [x] Resolve IPNS names in publish, in order to verify if they exist (as it is being done for regular files) before being published
- [x] Handle remaining parameters in publish and resolve (except ttl). 
- [x] Test interface core spec [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327)
- [x] Test interoperability with go. [interop#26](ipfs/interop#26)
- [x] Integrate logging
- [x] Write unit tests
- [x] Add support for the lifetime with nanoseconds precision
- [x] Add Cache
- [x] Add initializeKeyspace
- [x] Republish

Some notes, regarding the previous steps: 

- There is an optional parameter not implemented in this PR, which is `ttl`, since it is still experimental, we can add it in a separate PR.

Finally, thanks @Stebalien for your help and time answering all my questions regarding the IPNS implementation in GO.

Moreover, since there are no specs, and not that much documentation, I have been writing a document with all the IPNS workflow. It is a WIP by now, but it is currently available [here](ipfs/specs#184).

Related PRs:

- [x] [js-ipns#4](ipfs/js-ipns#4)
- [x] [js-ipfs-repo#173](ipfs/js-ipfs-repo#173)
- [x] [js-ipfs#1496](#1496)
- [x] [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327)
- [x] enable `interface-ipfs-core` tests for IPNS in `js-ipfs`
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.

2 participants