-
Notifications
You must be signed in to change notification settings - Fork 42
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!: remove @waku/message-hash
and use @waku/core
#1810
Conversation
ad57d49
to
5dfee9d
Compare
818a306
to
e30331c
Compare
with the removal of `message-hash` as a package, the package-lock was polluted with outdated references
e30331c
to
8f652bf
Compare
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a lot of other changes slipped into
Not sure how this is justified. The aim is to keep ok, maybe hash and core have similar dependencies... but as long as dependabot ensure that all dependenccies are up to date (ie, core and message-hash use the asme version of the same dependencies) then it should not be a problem. Until message-hash is actually used in core, this change seems unnecessary. |
message hashing is indeed being used by the Filter protocol within here is it's usage within Filter: js-waku/packages/core/src/lib/filter/index.ts Line 281 in 749d84d
apologies that the PR description did not specify the same! |
right! |
I think it would be a better architect if the multiple filter subscriptions handling is done at a Higher level and Core remains the purest protocol implementation possible+ hard requirements such as peer management. Fillter redundancy and message deduplication fit at a general SDK level because it's intrinsically opinionated.
I understand there is a difference with we have done in the past but this is how we solved the problems we previously had. We could even provide several SDK using different approaches: one callback based, one promise based, one event based, etc. Nwaku and js-waku should some how converge in terms of architects. core js-waku package is the same level then libwaku nwaku: protocol implementation and mandatory logic such as peer management. Logic that is here to provide great Dev ex, reliable etc is part of the "compose protocol" milestone scope. In the case of nwaku. It's logic in node nwaku or written directly in Rust/Python/Golang/NodeJS, using the ffi interface. In the case of js-waku, it should be a package different from core. So no, message-hash is currently not used by Waku protocols, you are using it to build a general SDK that composes Waku protocols for better reliability. @waku-org/js-waku-developers Cc @waku-org/nwaku-developers |
tracking the above point here: #1886 |
Problem
@waku/message-hash
was introduced as a package as an implementation of Deterministic Message Hashing (14/WAKU2-MESSAGE). This package relies onsha256
that comes from@nobles/hashes
, and@waku/utils
.Both of these packages are already a dependency in
@waku/core
so it makes sense to move this packages there.Plus, the utility of
@waku/message-hash
, currently, is quite minimal to be setup as a separate package.Solution
Move message hashing within
@waku/core
Notes
cc @fryorcraken