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

follow-up(miden-proving-service): address missing comments #1078

Merged
merged 10 commits into from
Jan 17, 2025

Conversation

SantiagoPittella
Copy link
Collaborator

This PR aims to address the comments of this review.

I tried to maintain commits as we do in reviews, so the reviewer can keep track of which comment I'm addressing in each one.

It:

  • Removes the testing feature from the proving-service crate.
  • Updates README's.
  • Removes a couple of unused deps.
  • Updates the reqwest dependency in the proving service.
  • Replaces once_cell with LazyLock.
  • Renames miden-remote-provers to miden-proving-service-client.
  • Updates the Makefile.

@SantiagoPittella
Copy link
Collaborator Author

In the original PR, there was this comment:

Question: why do we need to do take() here now? (I don't think this was needed before, right?)

This is the answer:

I replaced the .take with a .as_ref and then cloning the client.

This change was needed because of a clippy error. We didn't ran the linter earlier because it was only enabled when using the "async" feature and it was disabled in the Makefile. Now that we have clippy for no-std, the linter is throwing an error because the MutexGuard was held through an .await call.

cc @bobbinth

@bobbinth bobbinth added this to the v0.7 milestone Jan 17, 2025
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of small comments inline. After these, we are good to merge.


## License

This project is [MIT licensed](../LICENSE).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would need to be ../../LICENSE (since we are two levels deep from the root).

Comment on lines +83 to +85
.as_ref()
.ok_or_else(|| TransactionProverError::other("client should be connected"))?
.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Cloning the client is pretty light-weight, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is composed only for the tonic client which isn't "big".

@bobbinth bobbinth removed this from the v0.7 milestone Jan 17, 2025
@SantiagoPittella
Copy link
Collaborator Author

@bobbinth I think this is ready to merge, but I saw that you moved it from the milestone. Should I merge it or wait until the new release is out?

@bobbinth
Copy link
Contributor

@bobbinth I think this is ready to merge, but I saw that you moved it from the milestone. Should I merge it or wait until the new release is out?

Yes, let's merge. I mistakenly assigned a milestone to a PR (we usually assign milestones only to issues).

@SantiagoPittella SantiagoPittella merged commit cd64555 into next Jan 17, 2025
11 checks passed
@SantiagoPittella SantiagoPittella deleted the santiagopittella-followup-crates-split branch January 17, 2025 18:31
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