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 CLI and implement fetch-params #516

Merged
merged 24 commits into from
Jun 29, 2020
Merged

Refactor CLI and implement fetch-params #516

merged 24 commits into from
Jun 29, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Refactors CLI to setup framework for subcommands

    • Now forest still runs the daemon, but subcommands can be run as forest <cmd> such as here is forest fetch-params ..
  • fetch-params command implemented which will fetch params based on the options of command and put into the params cache to be used in proof verification

    • Options are self-documenting by adding the help flag when running the command
    • Verbose flag will use progress bars for the download progress (doesn't always show up cleanly because async logger and the progress bar std out don't always play nicely

The params fetching is all done in parallel and async

Reference issue to close (if applicable)

Closes #508

Other information and links

Copy link
Contributor

@StaticallyTypedAnxiety StaticallyTypedAnxiety left a comment

Choose a reason for hiding this comment

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

Just few suggestions but looks good to me

forest/src/paramfetch/mod.rs Outdated Show resolved Hide resolved
forest/src/paramfetch/mod.rs Outdated Show resolved Hide resolved
forest/src/paramfetch/mod.rs Outdated Show resolved Hide resolved
forest/src/daemon.rs Outdated Show resolved Hide resolved
@austinabell austinabell changed the base branch from master to main June 24, 2020 15:21
Copy link
Member

@ec2 ec2 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 hear reqwest is pretty heavy. It is recommended to use Surf. Surf has a lot of overlapping libs with Tide since they're built by the same people. Up to you if you think its worth the change though

forest/src/cli/mod.rs Outdated Show resolved Hide resolved
@austinabell
Copy link
Contributor Author

LGTM. I hear reqwest is pretty heavy. It is recommended to use Surf. Surf has a lot of overlapping libs with Tide since they're built by the same people. Up to you if you think its worth the change though

Yeah thing was is that it seemed they were still on alpha for using stable futures so didn't seem worth the trial and error, I will play around with it now for a bit though

@austinabell austinabell requested a review from ec2 June 25, 2020 14:25
@austinabell
Copy link
Contributor Author

I made a bunch of changes so just going to assume reviews invalid @AshantiMutinta @ec2 (sorry they were pretty necessary changes)

@austinabell austinabell merged commit 7143e42 into main Jun 29, 2020
@austinabell austinabell deleted the austin/cli/params branch June 29, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proof parameters fetch
4 participants