-
Notifications
You must be signed in to change notification settings - Fork 268
Sim2h Integration #1744
Sim2h Integration #1744
Conversation
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.
for the sake of related stuff I've been doing, reading over this code anyways so I thought I'd chime in...
Co-Authored-By: Connor Turland <connorturland@gmail.com>
@@ -0,0 +1,14 @@ | |||
[package] |
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.
@AshantiMutinta, take note of this new crate. It only includes this ConductorApi
struct which is a proxy for the conductor api that does not depend on anything in the conductor_api
crate - which is good because we can't depend on that in the net
crate without creating cyclic dependencies. ConductorApi
was in core
before we can't depend on that either.
With the repository clean-up that we discussed yesterday, I would suggest with rename this crate to conductor_api
since the current conductor_api
will be conductor_lib
, right?
net/src/sim2h_worker.rs
Outdated
transport.request( | ||
Span::todo("Find out how to use spans the right way"), | ||
RequestToChild::Bind { | ||
spec: Url::parse("wss://localhost:0").expect("can parse url").into(), |
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.
Didn't we decide we need wss://127.0.0.1:0
to ensure we use ipv4 for now?
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.
Hm, good point. Interesting. It seems this was only a problem with the server. This code worked on @zippy's machine with IPv6 interfaces... will change it anyway to be sure.
net/src/sim2h_worker.rs
Outdated
|
||
/// Check for messages from our NetworkEngine | ||
fn tick(&mut self) -> NetResult<bool> { | ||
self.num_ticks += 1; |
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.
as a u32
, if tick gets called once per millisecond (on a fast computer hopefully it'll get called more often than this even) - num_ticks
will overflow in ~50 days. I think it should be a u64
: )
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. I've removed num_ticks
now since it wasn't used anyway.
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.
couple minor comments / suggestions - all in all looks awesome!
Co-Authored-By: David Braden <neonphog@gmail.com>
@Connoropolous, the sim2h_server needs to be accessible from the outside so it has to bind to real network interfaces. The sim2h worker is a client that only calls out, we don't even want somebody to connect on that port. |
PR summary
==== parity with sim1h ^^ =====
WireMessages
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)