-
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
feat: metadata protocol #1732
feat: metadata protocol #1732
Conversation
8b5f328
to
8f04682
Compare
c7fee6e
to
47b95f1
Compare
47b95f1
to
e691aa7
Compare
size-limit report 📦
|
f6976e5
to
4efcad5
Compare
fa922b4
to
6524032
Compare
0f9c628
to
61f7e5a
Compare
}); | ||
const response = proto_metadata.WakuMetadataResponse.decode(bytes); | ||
if (!response) { | ||
throw new Error("Error decoding metadata response"); |
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.
let's move away from exceptions and use enum error across all req-res interfaces
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.
yes. updated the issue for metadata & tracked part of #1694
await tearDownNodes([nwaku1], waku); | ||
}); | ||
|
||
it("interop", async function () { |
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.
More tests are needed:
- same cluster, same shard: nodes connect
- same cluster, different shardL nodes connects
- different cluster, same shard, node disconnect
- different cluster, different shard, node disconnects
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.
right, thanks!
ca17b30
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.
Some comments.
I think it would have been easier to get the API right (enum error) from the get go.
this.shardInfo | ||
); | ||
|
||
await pipe([encodedResponse], lp.encode, streamData.stream); |
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.
The metadata request contains the shardinfo from the remote node. We should save it locally.
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.
await waku.libp2p.dialProtocol(nwaku1Ma, MetadataCodec); | ||
|
||
await expect( | ||
waku.libp2p.services.metadata?.query(nwaku1PeerId) |
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.
What is that?
It seems better to check whether you have an active connection with the node, as the expectation is no active connection.
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.
the metadata query fails (with a throw) if the clusters are different between the service node it's attempting to dial therefore the expecting a catch
you're right in pointing out that there will no active connection, you're right in pointing out that the connection will be closed so good to add an expect for that as well: d53eddb
--
wanted to be explicit with making the query, but perhaps it's simpler to remove the query()
attempt and directly expect the connection to be dropped:d53eddb
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.
LGTM
This PR introduces the Metadata Protocol as a service
Notes: