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(mobile): create main package #5

Closed
wants to merge 34 commits into from

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 8, 2019

  • Split pkg into host/node
    • host build mobile dedicated host (noop for the moment)
    • node build mobile dedicated node with the particularity to create a mobile host
  • the main package can be bind using gomobile to create a ready-to-use
    framework/aar
  • individual pkg can be imported instead and be used for specific project
  • Add simple makefile that show how to build the thing
  • Add a react-native example

moul and others added 5 commits June 30, 2019 17:55
chore: add some context in the README
* Split pkg into repo/host/node
  * host build mobile dedicated host (noop for the moment)
  * repo build mobile dedicated repo, exactly the same as ipfs repo but add the
    ability to add some custom configuration
  * node build mobile dedicated node with the particularity to create a mobile host
* the main package can be bind using gomobile to create a ready-to-use
  framework/aar
* individual pkg can be imported instead and be used for specific project
* Add simple makefile that show how to build the thing
@gfanton gfanton changed the title feat(mobile): create main package WIP: feat(mobile): create main package Aug 21, 2019
@moul moul self-requested a review September 11, 2019 13:04
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Very good PoC 👍

@aeddi
Copy link
Member

aeddi commented Sep 19, 2019

Fixes #1 and #2
Addresses #7, #13 and #14

@aeddi aeddi changed the title WIP: feat(mobile): create main package feat(mobile): create main package Sep 19, 2019
@parkan parkan requested a review from Stebalien September 23, 2019 19:40
Copy link
Contributor

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Nice! I'd like to move some of that go code back into go-ipfs over time but it's probably simpler to leave it here for now.

My main concern is exposing the API like that.

host/host.go Outdated
// MobileHost is an host
var _ p2p_host.Host = (*MobileHost)(nil)

type MobileHost struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this wrapper?

Copy link
Member Author

@gfanton gfanton Sep 25, 2019

Choose a reason for hiding this comment

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

The main purpose of this wrapper was to customize the host dedicated to the mobile, for example: we plan to add a native driver that notifies the host of connectivity changes (switch between wifi/4G/nothing, bluetooth enabled/disabled, internet connected/disconnect, etc...) so the host can adapt his behavior accordingly. For the moment, it does nothing but to show where you can add those upgrades. I don't mind to remove it for the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. SGTM.

You might consider just spawning that as a separate service. However, wrapping the host does give you an easy way to manage the service.

init.go Outdated
API: ipfs_config.API{
HTTPHeaders: map[string][]string{
"Access-Control-Allow-Credentials": []string{"true"},
"Access-Control-Allow-Origin": []string{"*"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is extremely insecure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, despite the fact this was only for testing, it's a real problem. even if we specify localhost, other apps running on the device will be able to access to the API, Also we should use a random port for the api on each run to avoid collision with other app on mobile and force the config to be updated accordingly (on each run). I don't see any way to expose http api securely for the moment, like @asutula said in #14, it should be better and more secure to expose all the core API directly in Java/Swift/Objc using gobind, but this will involve more works to do.
I will just manually rollback to a static port value as temporary solution for the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's for testing, it's probably fine. However, people tend to copy/paste example/test code... that's my main concern.

Actually, I'm pretty sure all apps on the device will get access regardless, right? CORs only applies to browsers. Would a unix domain socket work (ipfs/kubo#6678)? You could put that into a private shared folder (I think...)

Copy link
Member

@aeddi aeddi Sep 26, 2019

Choose a reason for hiding this comment

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

This is exactly what we were thinking yesterday about unix socket. With Android and iOS, it's easy to create files that are only accessible by our app.

We're trying to set this up ASAP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm excited about the unix domain socket (UDS) idea, but having a hard time finding info about HTTP over UDS for Objective C and/or Java. Not that it's impossible, but I don't see any existing solutions. Anyone seen otherwise?

Copy link
Contributor

@Stebalien Stebalien Sep 30, 2019

Choose a reason for hiding this comment

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

On android, you'd construct a java.net.URL with a custom java.net.URLStreamHandler (as far as I can tell). In objective C... you may need to use a third-party library.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find a small shortcut for the moment, instead of trying implementing unix socket in java/objc(/swift), I just exposed an IpfsShell with a unix socket transport to the ios/android side.
Im not exposing http api by default anymore but instead I add some helpers to setup API either by unix socket or tcp. It's not the best way to do but I think it will do the trick for the moment.
My main concern is for react native (in the example) it will be hard to make the WebUI works with the shell (via react native-bridge) since it uses the js-ipfs-http-client (it will be tricky to override this), but since it's only for the example I presume it's ok (?). However I can easily call IpfsShell api from react native with the react native bridge (without enabling tpc api).
I'm almost done, but I still need to implement iOS bridge and cleanup some stuff.
Tell me if it's ok for you so I can finish this tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can get js-ipfs-http-client to use an arbitrary socket.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool idea with the "shell" object to be able to use http over UDS in Go! First questions that come to mind...

Are you able to support async/concurrent requests? That seems important for being able to write apps optimized to deal with larger amounts of data. Also, things like starting and stopping the node can take a non-trivial amount of time.

Do you see the possibility of a mechanism to push data from the node to the client? (I'm not totally familiar with the IPFS API, so maybe this isn't needed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(And those questions there apply to both the shell and ObjC/Swift/Java HTTP over UDS)

init.go Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Nov 25, 2019

There were the following issues with this Pull Request

  • Commit: f9cfbd5
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@aeddi aeddi deleted the dev/base branch November 27, 2019 18:18
@aeddi aeddi restored the dev/base branch November 27, 2019 18:18
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.

5 participants