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

Chore/add typedefs peerstore book template 2 #831

Closed
wants to merge 57 commits into from

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Dec 10, 2020

This pull request does following:

@vasco-santos one other thing I want to call out here is that this change turns StreamHandler into non generic, that is because it uses CircuitPB encode / decode which in turn expects concrete structs. I am however not exactly sure I got all the details right, specifically:

  1. According to CircuitRelay protobuf all fields are optional.
  2. But than typedef in chore: add typedefs #802 was defining CircuitRequest with all fields (but code) mandatory.
  3. Writes to StreamHandler seems to always contain code & type
  4. Reads values from StreamHandler are assumed to have type srcPeer, dstPeer.

Which is what were reflected in the StreamHandler API reads return CircuitRequests with all the mandatory fields and writes take CircuitMessages with all the optional fields. However I am guessing that CircuitMessage in fact is more of a union of different message types where each corresponds to Type enum and has different subset of fields & probably each must have some subset of the the fields and that is why protobuf just marks them all optional.

If above hypothesis is correct it would make much more sense to represent each message type via actual type definition with it's fields and represent CircuitMessage as a type union. It would be good to sync up on this to fix those details up.

vasco-santos and others added 30 commits November 9, 2020 14:11
* feat: auto relay

* fix: leverage protoBook events to ask relay peers if they support hop

* chore: refactor disconnect

* chore: do not listen on a relayed conn

* chore: tweaks

* chore: improve _listenOnAvailableHopRelays logic

* chore: default value of 1 to maxListeners on auto-relay
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
BREAKING CHANGE: pubsub signing policy properties were changed according to libp2p-interfaces changes to a single property. The emitSelf option default value was also modified to match the routers value
* feat: custom dialer addr sorter

* chore: use libp2p utils sorter via addressBook getMultiaddrsForPeer

* chore: use new libp2p utils

* chore: apply suggestions from code review

Co-authored-by: Jacob Heun <jacobheun@gmail.com>

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
* chore: auto relay example

* chore: update examples to use process arguments

* chore: add test setup for node tests and test for auto-relay

* chore: apply suggestions from code review

* chore: do not use promise for multiaddrs event on example
* chore: update websockets
@vasco-santos
Copy link
Member

Added a commit merging in the new updates of the base types branch to check this

@vasco-santos vasco-santos force-pushed the chore/add-typedefs-peerstore-book-template branch 2 times, most recently from 66b9abd to 222bb7b Compare December 10, 2020 11:07
@vasco-santos vasco-santos force-pushed the chore/add-typedefs-peerstore-book-template-2 branch from 671ba33 to e014fee Compare December 10, 2020 11:21
src/upgrader.js Outdated
@@ -231,6 +231,7 @@ class Upgrader {
const { stream, protocol } = await mss.handle(Array.from(this.protocols.keys()))
log('%s: incoming stream opened on %s', direction, protocol)
if (this.metrics) this.metrics.trackStream({ stream, remotePeer, protocol })
// @ts-ignore - metadata seems to be required
Copy link
Member

Choose a reason for hiding this comment

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

@vasco-santos vasco-santos changed the base branch from chore/add-typedefs-peerstore-book-template to chore/add-typedefs December 10, 2020 11:51
@vasco-santos
Copy link
Member

vasco-santos commented Dec 10, 2020

This looks good to me! However, a new issue appeared as a result of this update...

File './node_modules/@types/node/events.d.ts' is not a module.
import { EventEmitter } from "node/events"

This only happens with the new ts. If you look into the dist on libp2p/dist/src/connection-manager/index.d.ts:153:30 you will see the issue with vscode. That is: microsoft/TypeScript#41800 for 4.2.0

@vasco-santos
Copy link
Member

vasco-santos commented Dec 10, 2020

Which is what were reflected in the StreamHandler API reads return CircuitRequests with all the mandatory fields and writes take CircuitMessages with all the optional fields. However I am guessing that CircuitMessage in fact is more of a union of different message types where each corresponds to Type enum and has different subset of fields & probably each must have some subset of the the fields and that is why protobuf just marks them all optional.

If above hypothesis is correct it would make much more sense to represent each message type via actual type definition with it's fields and represent CircuitMessage as a type union. It would be good to sync up on this to fix those details up.

Yes, this sounds good! I will add it to the follow up list to not block this anymore.
We can follow up with this PR, as well as the list of pending items for the next release

@vasco-santos vasco-santos changed the base branch from chore/add-typedefs to 0.30.x December 10, 2020 13:48
@vasco-santos vasco-santos force-pushed the chore/add-typedefs-peerstore-book-template-2 branch from 1d3492f to 2e7f49e Compare December 10, 2020 14:03
@vasco-santos vasco-santos mentioned this pull request Dec 10, 2020
7 tasks
Base automatically changed from 0.30.x to master December 16, 2020 12:56
@lidel
Copy link
Member

lidel commented Apr 12, 2021

Discussion continued in #830

@lidel lidel closed this Apr 12, 2021
@achingbrain achingbrain deleted the chore/add-typedefs-peerstore-book-template-2 branch May 18, 2023 13:41
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