Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Incompatible argument types #101

Closed
achingbrain opened this issue Jul 5, 2021 · 8 comments · Fixed by #111
Closed

Incompatible argument types #101

achingbrain opened this issue Jul 5, 2021 · 8 comments · Fixed by #111
Labels
kind/bug A bug in existing code (including security flaws) released

Comments

@achingbrain
Copy link
Member

The pubsub _publish method takes an arg of InMessage|RPCMessage.

InMessage has an optional from field of type string, RPCMessage has an optional from field of Uint8Array|undefined.

These are not compatible. Which one is correct?
@achingbrain
Copy link
Member Author

It looks to me like it's only every going to be a Uint8Array since messages passed to _publish first go through normalizeOutRpcMessage which turns string from fields into Uint8Arrays if they are present but I'm not sure if this is the intention or not.

@achingbrain
Copy link
Member Author

Actually sometimes it can be a string. I think this only ever worked in the past because the CID constructor took so many input types.

@vasco-santos
Copy link
Member

It is computed in https://github.com/libp2p/js-libp2p-interfaces/blob/master/src/pubsub/utils.js#L100 and I think the from optional field is for the signature validation. Perhaps @wemeetagain can give more insights, I don't recall exactly the reasoning

@lidel lidel added the kind/bug A bug in existing code (including security flaws) label Oct 8, 2021
achingbrain added a commit that referenced this issue Nov 6, 2021
@achingbrain
feat: split out code
8f80033 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
achingbrain added a commit that referenced this issue Nov 6, 2021
@achingbrain
feat: split out code
e9d383a 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
@achingbrain achingbrain mentioned this issue Nov 6, 2021
Merged
achingbrain added a commit that referenced this issue Nov 6, 2021
@achingbrain
feat: split out code
85f8dfc 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
achingbrain added a commit that referenced this issue Nov 6, 2021
@achingbrain
feat: split out code
2909163 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
achingbrain added a commit that referenced this issue Nov 6, 2021
@achingbrain
feat: split out code
0fedfa0 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
achingbrain added a commit that referenced this issue Nov 7, 2021
@achingbrain
feat: split out code
42e2ed5 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110)
* Converts code to typescript (fixes #101)
* Adds types for PeerData and Registrar - these will need to be fleshed out properly in a subsequent PR

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces
achingbrain added a commit that referenced this issue Nov 22, 2021
@achingbrain
feat: split out code, convert to typescript (#111)
e174bba 
* Splits implementations out from interfaces so we can depend on interfaces module without pulling in crypto, etc (fixes #110).  New implementation modules are:
  * libp2p-pubsub
  * libp2p-connection
  * libp2p-topology
* Converts code to typescript (fixes #101)
* Adds types for PeerData, PeerStore, Registrar and some other libp2p internals - these will need to be fleshed out properly in a subsequent PR
* No CJS, only ESM, only forwards
* Upgrades deps that have also gone ESM-only
* Renames package folders for consistency

I've tried to break some of the circular dependencies in the interfaces (e.g. libp2p depends on pubsub, which depends on libp2p, which depends on pubsub, etc).  This hasn't been possible in the tests so I've added `@ts-expect-error` where we access these properties with the aim to revisit it once this has been rolled up to libp2p itself.

BREAKING CHANGE: not all fields from concrete classes have been added to the interfaces, some adjustment may be necessary as this gets rolled out
@wemeetagain
Copy link
Member

Sorry late reply here, but the reason for the difference is related to libp2p/js-libp2p#680

It's more convenient for the from field to be a string peer id.
But the protobuf expects the Uint8Array representation of the from field.

The normalize*RpcMessage functions were there to translate between these two worlds, user-facing InMessage object vs wire RPCMessage object.

That _publish interface was too flexible, not great. It looks like that _publish interface wasn't (isn't) respected in js-libp2p-gossipsub: _publish(msg: InMessage): Promise<void>.

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

🎉 This issue has been resolved in version @libp2p/interfaces-v1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

🎉 This issue has been resolved in version @libp2p/topology-v1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

🎉 This issue has been resolved in version @libp2p/connection-v1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

🎉 This issue has been resolved in version @libp2p/pubsub-v1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tinytb tinytb moved this to Done in js-libp2p Oct 11, 2022
@tinytb tinytb added this to js-libp2p Oct 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
kind/bug A bug in existing code (including security flaws) released
Projects
js-libp2p
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: split out code, convert to typescript libp2p/js-libp2p-interfaces
4 participants
@lidel @achingbrain @wemeetagain @vasco-santos