Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

feat: instantiate with upgrader #53

Merged
merged 4 commits into from
Sep 6, 2019
Merged

feat: instantiate with upgrader #53

merged 4 commits into from
Sep 6, 2019

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Sep 5, 2019

As part of the migration to async await, and in order to add better flexibility to libp2p connection upgrades, Transports will now take, and be required to support using an Upgrader instance.

Upgraders have 2 methods: upgradeOutbound and upgradeInbound.

  • upgradeOutbound must be called and returned by transport.dial.
  • upgradeInbound must be called and the results must be passed to the createListener handlerFunction and the connection event handler, anytime a new connection is created.
const connection = await upgrader.upgradeOutbound({ source, sink, conn, remoteAddr })
const connection = await upgrader.upgradeInbound({ source, sink, conn, remoteAddr })

The Upgrader methods will return interface-connection instances, the new API for which is being solidified at libp2p/interface-connection#29.

@jacobheun
Copy link
Contributor Author

@alanshaw if this looks good my thought for testing is to just add spies on the upgrader to verify the methods are called, and return a { sink, source } object as that's all that should be needed for testing. Transport tests shouldn't care about interface-connection, and returning the original { sink, source } should enable us to do stream verification.

cc: @vasco-santos

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM - just one suggestion

README.md Outdated

This method uses a transport to dial a Peer listening on `multiaddr`.

`multiaddr` must be of the type [`multiaddr`](https://www.npmjs.com/multiaddr).

`[options]` the options that may be passed to the dial. Must support the `signal` option (see below)

`conn` must implement the [interface-connection](https://github.com/libp2p/interface-connection) interface.
Dial **MUST** call and return `upgrader.upgradeOutbound({ source, sink, conn, remoteAddr })`. The upgrader will return an [interface-connection](https://github.com/libp2p/interface-connection) instance.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth creating a definition of { source, sink, conn, remoteAddr } as MultiaddrConnection somewhere so we can refer to it better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking interface-connection initially, but I think it's better in the readme here since it's specific to transports.

@jacobheun
Copy link
Contributor Author

@alanshaw @vasco-santos I have added tests to dial and listen. Test setup now takes options, and Transports will need to pass those to their constructors in test setup (the readme has been updated).

The tests now validate the respective spies are called, and that the connections returned from the transport (both dial and listen), are the same values returned by the upgrader spies.

I did some quick hacks locally on the existing libp2p-tcp async branch to ensure these tests are running properly.

@jacobheun jacobheun merged commit a5ad120 into master Sep 6, 2019
@jacobheun jacobheun deleted the feat/upgrader branch September 6, 2019 07:44
@jacobheun
Copy link
Contributor Author

This has been released as 0.6.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants