Skip to content

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

Merged
merged 9 commits into from
Apr 14, 2022
Merged

Init Mithril Client #110

merged 9 commits into from
Apr 14, 2022

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Apr 12, 2022

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

  1. Start a local Mithril Aggregator in terminal window: cd mithril-network/mithril-aggregator && make debug
  2. Launch the Mithril Client in a separate terminal window: cd mithril-network/mithril-client && make debug list

Relates to #102

@github-actions
Copy link

github-actions bot commented Apr 12, 2022

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
5 tests ±0  5 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit fd5a849. ± Comparison against base commit dd8038b.

♻️ This comment has been updated with latest results.

@jpraynaud jpraynaud marked this pull request as ready for review April 12, 2022 16:38
@jpraynaud jpraynaud requested review from a user and Alenar April 12, 2022 16:39
Copy link

@ghost ghost left a 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:
Copy link

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

Copy link
Member Author

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 🤔

Copy link

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.

@jpraynaud jpraynaud force-pushed the jpraynaud/mithril-client branch from 9a94c53 to 47a6e46 Compare April 13, 2022 18:59
@jpraynaud jpraynaud requested a review from a user April 14, 2022 05:46
Copy link

@ghost ghost left a 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
@jpraynaud jpraynaud force-pushed the jpraynaud/mithril-client branch from 47a6e46 to fd5a849 Compare April 14, 2022 08:13
@jpraynaud jpraynaud merged commit 47cf084 into main Apr 14, 2022
@jpraynaud jpraynaud deleted the jpraynaud/mithril-client branch April 14, 2022 08:38
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.

1 participant