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

Refactor topology, NymTopology trait, and related code #200

Closed
futurechimp opened this issue Apr 23, 2020 · 1 comment · Fixed by #274
Closed

Refactor topology, NymTopology trait, and related code #200

futurechimp opened this issue Apr 23, 2020 · 1 comment · Fixed by #274
Milestone

Comments

@futurechimp
Copy link
Contributor

In 8eae7c4 I did a horrible, expedient thing: I needed some code from NymTopology in the WebAssembly client.

NymTopology (which is marbled through the entire codebase) depends on a low level network library called net2 which incorporates C code. So it can't be compiled to WebAssembly.

Ideally we could split out the HTTP-related part (which in the case of the WebAssembly client is done by JavaScript anyway). Alternately, perhaps we could move the HTTP part into the WebAssembly if we can get rid of that C dependency.

One thing that struck me: looking through the use of the NymTopology trait it is used everywhere primarily to convert between what I'd call in server-side systems a "front-end" data model (where we receive untrusted camelCase input from the network) to a "back-end" data model, which in our case is ultimately the Sphinx crate - a snake_cased chunk of code that has a more compact data model than what real network clients use. The question I'd have about NymTopology overall is, would it be better to just do some permanent type conversions into a set of structures that would make Sphinx happy, probably right when the data comes in the door, and dispense with the trait?

These are just some ruminations that I don't want to lose. Ultimately the main problem is I've done a dirty copy-paste into the WebAssembly client that gets it working quickly but results in ugly duplication. Reminder: fix it when there's time!

@futurechimp futurechimp added this to the 0.8.0 milestone Apr 23, 2020
@futurechimp futurechimp added this to Backlog in Core systems via automation Apr 23, 2020
@jstuczyn
Copy link
Contributor

jstuczyn commented Apr 28, 2020

Because of the mess caused by Topology, we have an undeterministic test at:

assert_eq!(service_mixnode, service_topology.mixnodes[0]);

Basically ServiceTopology::from(rest_topology) causes multiple calls to now() which in very rare circumstances can result in contradictory values.
edit: that particular test was fixed

Core systems automation moved this from Backlog to Done Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Core systems
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants