-
Notifications
You must be signed in to change notification settings - Fork 44
Init Mithril Client #110
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
Init Mithril Client #110
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.
LGTM, minor suggestions
@@ -148,6 +148,62 @@ jobs: | |||
command: test | |||
args: --release --manifest-path ./mithril-network/mithril-aggregator/Cargo.toml | |||
|
|||
build-mithril-client: |
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.
Perhaps there's a way to have client and aggregator in the same project but as different exes or crates? That would speed things up and simplify build
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.
It's possible to have multiple binary built in a src/bin configuration of the project: this would allow us to build all executable files in the same job.
We can investigate further to see if it is a wise architectural choice 🤔
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.
I agree that it introduces coupling between the client and the aggregator via the datatypes, and it can lead down the road to hard-to-change code and deep dependencies, if we are not cautious about more dependencies to creep in.
9a94c53
to
47a6e46
Compare
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
I would not bother too much with nice formatting, I think JSON output is ok at this stage and we can always massage it later to make it fancier.
* Better handling of the configuration * Better separation between snapshot data and display * Separated the AggregatorHandler fake implementation to new file * Added 'show' snapshot subcommand
* Added publish artifacts in build Mithril Client job * Fixed cache issue in documentation job
47a6e46
to
fd5a849
Compare
This is the first version of the Mithril Client:
It is able to retrieve a list of snapshots from a Mithril Aggregator and display in the command line
cd mithril-network/mithril-aggregator && make debug
cd mithril-network/mithril-client && make debug list
Relates to #102