Skip to content
This repository has been archived by the owner on Feb 25, 2023. It is now read-only.

Define rough architecture for middleware #31

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

TheCharlatan
Copy link
Contributor

The functionality of the middleware is now split into three packages. The middleware package takes care of communicating with bitcoind and lightningd and emits an event if new data is received.

The handlers package takes care of the communication with the bitbox-wallet-app. It upgrades connections to websocket, if requested, and starts the main middleware event loop. Events from the
middleware are caught and passed into a websocket handler.

The main package is what is compiled into a binary. It parses command line arguments and creates new middleware and handler instances.

This commit also implements a coherent logging structure, which is handled in the same fashion as in the bitbox-wallet-app.

@hkjn hkjn self-requested a review May 29, 2019 13:50
Copy link
Contributor

@hkjn hkjn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

See some comments inline from an initial review.

I noticed this is a large change (+231k LoC); see comment inline discussing some alternatives.

It also wasn't 100% clear to me if this change was entirely a refactor, or if there's also new features added (beyond logging).

Would it be possible to split out more granular commits, to simplify the review process and merge the easier parts first?

middleware/cmd/middleware/main.go Outdated Show resolved Hide resolved
middleware/cmd/middleware/main.go Show resolved Hide resolved
middleware/cmd/middleware/main.go Outdated Show resolved Hide resolved
middleware/src/handlers/handlers.go Outdated Show resolved Hide resolved
middleware/src/handlers/handlers.go Show resolved Hide resolved
middleware/util/logging/instance.go Outdated Show resolved Hide resolved
middleware/util/logging/instance.go Outdated Show resolved Hide resolved
middleware/util/logging/logging-stack-hook.go Outdated Show resolved Hide resolved
middleware/util/logging/logging-stack-hook.go Outdated Show resolved Hide resolved
@TheCharlatan TheCharlatan force-pushed the clean branch 4 times, most recently from 1ef7200 to 80f9a76 Compare June 3, 2019 13:02
@hkjn
Copy link
Contributor

hkjn commented Jun 3, 2019

I discussed this PR with @TheCharlatan offline, and the CI that was added in #34 should be passing at 80f9a76.

Even with using the github.com/sirupsen/logrus package directly, we still end up pulling in ~230k LoC, so we'll instead plan to not use any fancy logging package and leave some guidelines for how to structure log output for future developers.

@hkjn
Copy link
Contributor

hkjn commented Jun 3, 2019

We looked briefly into why github.com/sirupsen/logrus actually needs ~230k LoC, and it seems that ~225k LoC was due to the package depending on golang.org/x/sys/unix, which seems to just be used to detect if the caller is on UNIX, MacOs or Windows. There seems to have been some prior efforts (e.g sirupsen/logrus#930) to reduce the size of the logrus dependency graph, so for now we should probably stick to the recommendation of not using that package. If we can manage without any 3rd party package at all for logging (i.e just using fmt and log packages) that'd be easiest.

@TheCharlatan TheCharlatan force-pushed the clean branch 2 times, most recently from 71cbcf4 to 1044516 Compare June 3, 2019 13:54
Copy link
Contributor

@hkjn hkjn left a comment

Choose a reason for hiding this comment

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

Thanks for the changes to no longer depend on logrus, this is now a much smaller patch than before!

I've left some more comments inline.

middleware/cmd/middleware/main.go Show resolved Hide resolved
middleware/cmd/middleware/main.go Show resolved Hide resolved
middleware/src/handlers/handlers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hkjn hkjn left a comment

Choose a reason for hiding this comment

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

Thanks, see few more comments inline.

middleware/src/handlers/handlers.go Outdated Show resolved Hide resolved
middleware/src/handlers/handlers.go Outdated Show resolved Hide resolved
The functionality of the middleware is now split into three packages. The
middleware package takes care of communicating with bitcoind and
lightningd and emits an event if new data is received.

The handlers package takes care of the communication with
the bitbox-wallet-app. It upgrades connections to websocket, if
requested, and starts the main middleware event loop. Events from the
middleware are caught and passed into a websocket handler.

The main package is what is compiled into a binary. It parses command
line arguments and creates new middleware and handler instances.
This commit also implements logging, where each package creates its own
logrus logging instance.
Copy link
Contributor

@hkjn hkjn left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

ACK 9d4545a

@TheCharlatan TheCharlatan merged commit 485e26b into BitBoxSwiss:master Jun 3, 2019
@TheCharlatan TheCharlatan deleted the clean branch July 17, 2019 14:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants