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

Review Network Layer #60

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 13, 2019

Issue Number

Overview

  • I have moved modules around and renamed them a bit (mostly to remove the 'Rust' part) and prepare the foundation to reflect on the upcoming architecture

  • I have removed the API newtypes BlockHeader and Block in favor of a newtype ApiT which wraps existing primitive types from the wallet. This has the same effect, but doesn't yield to conflicting names (which messes up with various tooling like tags and forces us to have qualified imports to disambiguate types from the API and types from the primitives).

  • I have merged MockNetworkLayer with the RustHttpBridgeSpec, and the RustHttpBridgeClient with RustHttpBridge. Generally speaking, I'd like to avoid us to spread too much with modules, it makes code hard to find in the long run and eventually end up with us duplicating functionality and logic in various places. If there's no good reason to have multiple modules, I'd rather stick as much as we can inside one.

  • I have remove the ChainProducer monad in favor of an explicit type. Cf the comment below

  • I have moved some slotting comparison functions into the wallet primitives

Comments

There was a bit of confusion IMO about where the network layer sits (understandable because there's no clear layout of what the architecture should / could be anywhere ...).

Also, I took the opportunity to remove the 'Rust' part of the module names since this is quite irrelevant. We are programming to an interface (a.k.a the http-bridge API, and the implementation of that interface shouldn't really matter).

Doing so, I've remove the unnecessary abstraction of the chain producer Monad. We won't be instantiating multiple network layer at once and there's no meed for an abstraction ober this (really, we want to have different implementations behind a network layer but a plain concrete parameterised type is simpler and just fine)..The typeclass adds an extra layer of indirection and complexity that isn't needed.

KtorZ added 3 commits March 13, 2019 18:34
There was a bit of confusion IMO about where the network layer sits
(understandable because there's no clear layout of what the architecture
should / could be anywhere ...).

Also, I took the opportunity to remove the 'Rust' part of the module
names since this is quite irrelevant. We are programming to an interface
(a.k.a the http-bridge API, and the implementation of that interface
shouldn't really matter).

Doing so, I've remove the unnecessary abstraction of the chain producer
Monad. We won't be instantiating multiple network layer at once, so for
now, this adds an extra layer of complexity that isn't needed. In
practice we can just fallback to a plain data type that we instantiate
using one service (e.g. the cardano-http-bridge) or another (e.g. the
Haskell shelley node).
@KtorZ KtorZ requested review from rvl and paweljakubas March 13, 2019 17:44


spec :: Spec
spec = return ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allow for the code coverage to take this untested module into account.

( Spec )

spec :: Spec
spec = return ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allow for the code coverage to take this untested module into account.

@KtorZ KtorZ self-assigned this Mar 13, 2019
@KtorZ KtorZ changed the title Ktor z/review network layer Review Network Layer Mar 13, 2019
@KtorZ KtorZ requested a review from Anviking March 13, 2019 17:53
@KtorZ
Copy link
Member Author

KtorZ commented Mar 13, 2019

After this, I'd like to get the BlockSyncer module merged with the NetworkLayer, because that's where it will enter into play.

Also, I believe the implementation of the HttpBridge can be simplified a bit now that we have the client in context and don't require the extra indirection of the chain producer monad. Ideally, this module would export almost a single function: mkNetworkLayer which creates an explicit network layer interface (in the sense it has after this PR, not in the sense it currently has on master; the current 'NetworkLayer' has been renamed to 'HttpBridge' because that's what it is in essence).

@rvl rvl merged commit 6e54b08 into KtorZ/review-slotting-as-primitive Mar 13, 2019
@rvl rvl mentioned this pull request Mar 14, 2019
@KtorZ KtorZ deleted the KtorZ/review-network-layer branch March 14, 2019 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants