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

Prover service trait, SHARP client, fact checker, Stone stubs #6

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

unstark
Copy link
Contributor

@unstark unstark commented Apr 20, 2024

This WIP PR introduces prover traits and implementation stubs.

ProverService is the top-level abstraction, responsible for generating proofs given Cairo runner output and registering that proof onchain. Prover service is agnostic to the proving system (stark, cstark, halo2, etc) and base layer (Ethereum, Starknet, other EVM, Aligned Layer, etc).

StoneProverService is an implementation of the ProverService trait that combines Madara prover API (Stone + RPC) and one of the supported fact registries (Solidity StarkEx contracts, Cairo Integrity, Sovereign -- DA layer).

Other ProverService implementations:

image

Summary by CodeRabbit

  • New Features

    • Introduced new dependencies for enhanced functionality and integration.
    • Added ProverJob struct with methods for job creation, processing, and verification.
    • Implemented FactChecker struct for checking the validity of facts using a FactRegistry.
    • Introduced SharpClient for interacting with the SHARP API.
    • Added support for GPS fact-checking with new structures and methods.
  • Enhancements

    • Reorganized import statements across multiple files for improved code readability.
    • Updated enum variants in JobType for better clarity.
  • Bug Fixes

    • Adjusted method calls to ensure compatibility with new dependencies.
  • Documentation

    • Added README.md with instructions for generating artifacts for the GPS verifier.
  • Chores

    • Updated Cargo.toml files to reflect new and updated dependencies.

Copy link

coderabbitai bot commented Apr 20, 2024

Walkthrough

This update integrates new dependencies and restructures various modules across da_clients, orchestrator, and prover_services. Key changes include replacing reqwest::Client with alloy::transports::http::Client in the Ethereum DA client, adding new services and utilities in the orchestrator, and introducing a fact-checking mechanism in the GPS fact checker. Additionally, the orchestrator's job management has been enhanced with new job types and improved configuration handling.

Changes

File(s) Change Summary
Cargo.toml Added dependencies: prover-service, gps-fact-checker, sharp-service, stone-service.
crates/da_clients/ethereum/Cargo.toml Updated alloy dependency attributes and added reqwest features.
crates/da_clients/ethereum/src/lib.rs Replaced reqwest::Client with alloy::transports::http::Client and adjusted method calls.
crates/orchestrator/Cargo.toml Added dependencies: sharp-service, stone-service, prover-service, utils.
crates/orchestrator/src/... Various updates including imports, new modules, and job enhancements.
crates/prover_services/gps_fact_checker/... Introduces new dependencies, structs, enums, and instructions for the GPS fact checker.
crates/prover_services/sharp_service/... Added dependencies, structs, enums, and configurations for the SHARP service.

🐇
In the code's vast terrain, dependencies grow,
With alloy and sharp, our services flow.
Provers and fact-checkers, in harmony they sing,
Enhancing the orchestrator, like the dawn of spring.
Jobs redefined, configurations refined,
A symphony of changes, in the codebase aligned.
Hopping through modules, our updates do glow,
A rabbit's delight in the enhancements we sow.
🌱✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

src/config.rs Outdated Show resolved Hide resolved
src/provers/stone/mod.rs Outdated Show resolved Hide resolved
src/provers/stone/settings.rs Outdated Show resolved Hide resolved
src/provers/stark.rs Outdated Show resolved Hide resolved
src/provers/cpu/mod.rs Outdated Show resolved Hide resolved
@unstark unstark changed the title Draft: prover service trait and cpu/stone implementation Draft: prover service trait and implementation stubs Apr 29, 2024
}

#[async_trait]
impl<R: StoneProofRegistry> ProverService for StoneProverService<R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

so I was thinking that the prover api will talk to the orchestrator to register the proofs on the L1. even though the proof registration code is in the orchestrator, the normal sequencer and job flow isn't aware of it, the job creator and watcher will be the prover api. the orchestrator will just use the prover service to know when the proof is complete (which means its also registered on the L1). what do you think about this design?

if we're on the same page here, then my question was do we need the StoneProverService to depend on a StoneProofRegistry? I agree the prover service should have some context of where the proof will be registered so it can pass it to the prover api, but will the prover service need to call register_proof? If not, should we move it another job altogether which will be handled by the prover api

Copy link
Contributor Author

@unstark unstark May 17, 2024

Choose a reason for hiding this comment

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

Sure, that would work!
However we are then back to determining what is the generic proof output (that will be returned by ProverService, consumed by prover job, and submitted to verifier job) and how to handle "e2e" proving services in this paradigm.
Current design is mostly driven by the intention to put SHARP-like "monolith" and "modular" prover services under the same interface to simplify the orchestrator job

Copy link
Contributor

Choose a reason for hiding this comment

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

we are then back to determining what is the generic proof output

Hmm, but do we need the proof output anymore if we are assuming the prover service is going to take the responsibility of verifying it on the L1? Maybe I don't have the exact picture of the flow in your mind, happy to jump on a call :)


impl Default for SharpConfig {
fn default() -> Self {
unimplemented!("No defaults for SHARP config")
Copy link
Contributor

Choose a reason for hiding this comment

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

this causes the code to panic right now because we use the DefaultSettingsProvider and sharp/mod.rs calls settings.get_settings(SHARP_SETTINGS_NAME).unwrap()


impl FactChecker {
pub fn new(rpc_node_url: Url, verifier_address: Address) -> Self {
let provider = ProviderBuilder::new().on_http(rpc_node_url);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we store the provider in the global config and pass it here? it might be used in other places as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is rather specific (read only access), typically we will need a more involved provider (in Alloy it is now a much more complicated abstraction)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. But won't providers by default be read only and you need to attach them to an account to get write access?

src/provers/sharp/mod.rs Outdated Show resolved Hide resolved
Ok(FactInfo { program_output, fact_topology, fact })
}

pub fn get_program_output(cairo_pie: &CairoPie) -> Result<Vec<Felt252>, FactCheckerError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should ideally have test cases for this, what do you think is the best approach to test this? can we utilise mainnet data somehow? we can do it in a new PR

Copy link
Contributor Author

@unstark unstark May 17, 2024

Choose a reason for hiding this comment

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

I planned to take/generate test vectors using cairo-lang.
Separate PR would be best I think (though current one might contain bugs) since it will unblock other tasks

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, let's do a separate PR. Can you create an issue if possible?

src/fact_checker/fact_topology.rs Outdated Show resolved Hide resolved
src/fact_checker/fact_topology.rs Outdated Show resolved Hide resolved
@unstark unstark marked this pull request as ready for review May 18, 2024 00:37
@unstark unstark changed the title Draft: prover service trait and implementation stubs Prover service trait, SHARP client, fact checker, Stone stubs May 18, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 12

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 3fe3733 and 8a2f0c3.
Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • migrations/.DS_Store is excluded by !**/.DS_Store
Files selected for processing (43)
  • Cargo.toml (2 hunks)
  • crates/da_clients/ethereum/Cargo.toml (1 hunks)
  • crates/da_clients/ethereum/src/lib.rs (2 hunks)
  • crates/orchestrator/Cargo.toml (1 hunks)
  • crates/orchestrator/src/config.rs (5 hunks)
  • crates/orchestrator/src/controllers/jobs_controller.rs (1 hunks)
  • crates/orchestrator/src/database/mod.rs (1 hunks)
  • crates/orchestrator/src/database/mongodb/config.rs (1 hunks)
  • crates/orchestrator/src/database/mongodb/mod.rs (3 hunks)
  • crates/orchestrator/src/jobs/da_job/mod.rs (2 hunks)
  • crates/orchestrator/src/jobs/mod.rs (4 hunks)
  • crates/orchestrator/src/jobs/prover_job/mod.rs (1 hunks)
  • crates/orchestrator/src/jobs/types.rs (2 hunks)
  • crates/orchestrator/src/main.rs (1 hunks)
  • crates/orchestrator/src/queue/job_queue.rs (1 hunks)
  • crates/orchestrator/src/queue/mod.rs (1 hunks)
  • crates/orchestrator/src/queue/sqs/mod.rs (1 hunks)
  • crates/orchestrator/src/routes.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/Cargo.toml (1 hunks)
  • crates/prover_services/gps_fact_checker/src/artifacts/FactRegistry.json (1 hunks)
  • crates/prover_services/gps_fact_checker/src/error.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_info.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_node.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_topology.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/lib.rs (1 hunks)
  • crates/prover_services/prover_service/Cargo.toml (1 hunks)
  • crates/prover_services/prover_service/src/lib.rs (1 hunks)
  • crates/prover_services/sharp_service/Cargo.toml (1 hunks)
  • crates/prover_services/sharp_service/src/client.rs (1 hunks)
  • crates/prover_services/sharp_service/src/config.rs (1 hunks)
  • crates/prover_services/sharp_service/src/error.rs (1 hunks)
  • crates/prover_services/sharp_service/src/lib.rs (1 hunks)
  • crates/prover_services/stone_service/Cargo.toml (1 hunks)
  • crates/prover_services/stone_service/src/error.rs (1 hunks)
  • crates/prover_services/stone_service/src/gps.rs (1 hunks)
  • crates/prover_services/stone_service/src/integrity.rs (1 hunks)
  • crates/prover_services/stone_service/src/lib.rs (1 hunks)
  • crates/prover_services/stone_service/src/registry.rs (1 hunks)
  • crates/prover_services/stone_service/src/sovereign.rs (1 hunks)
  • crates/utils/Cargo.toml (1 hunks)
  • crates/utils/src/lib.rs (1 hunks)
  • crates/utils/src/settings/default.rs (1 hunks)
  • crates/utils/src/settings/mod.rs (1 hunks)
Files skipped from review due to trivial changes (14)
  • crates/orchestrator/Cargo.toml
  • crates/orchestrator/src/controllers/jobs_controller.rs
  • crates/orchestrator/src/database/mod.rs
  • crates/orchestrator/src/main.rs
  • crates/orchestrator/src/queue/mod.rs
  • crates/orchestrator/src/queue/sqs/mod.rs
  • crates/orchestrator/src/routes.rs
  • crates/prover_services/gps_fact_checker/Cargo.toml
  • crates/prover_services/prover_service/Cargo.toml
  • crates/prover_services/sharp_service/Cargo.toml
  • crates/prover_services/sharp_service/src/config.rs
  • crates/prover_services/stone_service/Cargo.toml
  • crates/utils/Cargo.toml
  • crates/utils/src/lib.rs
Additional comments not posted (26)
crates/prover_services/stone_service/src/error.rs (1)

6-9: The implementation of From<StoneProverError> for ProverServiceError is correctly done, ensuring that StoneProverError can be seamlessly converted into ProverServiceError.

crates/utils/src/settings/default.rs (1)

3-12: The implementation of DefaultSettingsProvider with the method get_settings returning default values is clear and adheres to the expected behavior of a default settings provider.

crates/utils/src/settings/mod.rs (2)

5-9: The SettingsProviderError enum is well-defined, with an Internal variant that appropriately encapsulates internal errors, allowing for detailed error reporting.


11-13: The SettingsProvider trait is correctly implemented, providing a clear interface for fetching settings based on a section name.

crates/da_clients/ethereum/Cargo.toml (1)

7-11: The modifications to the alloy dependency to use the workspace version and include reqwest in its features are appropriate, ensuring consistency and enhanced functionality across the workspace.

crates/prover_services/stone_service/src/registry.rs (1)

7-15: The StoneProofRegistry trait is well-designed, supporting settings injection and asynchronous proof registration. This design is suitable for services that interact with blockchain technologies.

crates/prover_services/sharp_service/src/error.rs (2)

5-19: The error handling with thiserror is well implemented and covers a comprehensive range of SHARP service-related errors.


21-24: The implementation of the conversion from SharpError to ProverServiceError is correctly done, using a boxed error to maintain compatibility across error types.

crates/da_clients/ethereum/src/lib.rs (1)

35-35: The replacement of reqwest::Client with alloy::transports::http::Client is consistent with the project's direction to integrate with the alloy library. Ensure that all interactions with the Ethereum network are tested to confirm that the new client integrates smoothly.

Verification successful

The integration of alloy::transports::http::Client with RpcClient appears to be correctly set up in the codebase. The Http<Client> type is defined and used appropriately as a transport layer for RpcClient.

  • RpcClient is imported from alloy::rpc::client::RpcClient.
  • The provider is defined as RpcClient<Http<Client>> in crates/da_clients/ethereum/src/lib.rs.

Ensure that all interactions with the Ethereum network are tested to confirm that the new client integrates smoothly.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the integration of alloy's HTTP client with Ethereum network interactions.

# Test: Search for usage of `alloy::transports::http::Client` and ensure it's correctly configured.
rg --type rust $'alloy::transports::http::Client'

Length of output: 48



Script:

#!/bin/bash
# Description: Verify the integration of alloy's HTTP client with Ethereum network interactions.

# Test: Search for usage of `RpcClient::builder()` to ensure it's correctly configured with alloy's HTTP client.
rg --type rust 'RpcClient::builder()'

Length of output: 121



Script:

#!/bin/bash
# Description: Extract the relevant lines around the usage of `RpcClient::builder()` in `crates/da_clients/ethereum/src/lib.rs`.

# Extract 10 lines before and after the usage of `RpcClient::builder()`.
rg --type rust 'RpcClient::builder()' -A 10 -B 10 crates/da_clients/ethereum/src/lib.rs

Length of output: 528



Script:

#!/bin/bash
# Description: Verify the definition and implementation of `RpcClient` and its `.http` method.

# Search for the definition of `RpcClient`.
rg --type rust 'struct RpcClient'

# Search for the implementation of the `.http` method.
rg --type rust 'fn http'

Length of output: 62



Script:

#!/bin/bash
# Description: Verify if `RpcClient` is imported from an external crate and check for references to external crates.

# Search for `RpcClient` imports.
rg --type rust 'RpcClient' crates/da_clients/ethereum/src/lib.rs

# Search for external crate references.
rg --type rust 'extern crate' crates/da_clients/ethereum/src/lib.rs

Length of output: 252



Script:

#!/bin/bash
# Description: Verify the definition and configuration of `Http<Client>`.

# Search for the definition of `Http<Client>`.
rg --type rust 'Http<Client>'

# Search for the configuration of `Http<Client>`.
rg --type rust 'Http<Client>' -A 10 -B 10

Length of output: 3310

crates/prover_services/gps_fact_checker/src/lib.rs (1)

21-39: The FactChecker struct and its is_valid method are well implemented, using the alloy library for network interactions and providing robust error handling. Ensure that the integration with the Ethereum network and the fact registry is thoroughly tested.

Verification successful

The FactChecker struct and its is_valid method are well implemented and correctly integrated with the Ethereum network and fact registry. The is_valid method is used to validate facts and handle task statuses appropriately in the SharpProverService.

  • crates/prover_services/sharp_service/src/lib.rs: The is_valid method is called within the ONCHAIN branch of a match statement, and its result determines the task status.
Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the integration of `FactChecker` with the Ethereum network and fact registry.

# Test: Search for usage of `FactChecker` and ensure it's correctly interacting with the network.
rg --type rust $'FactChecker'

Length of output: 5476



Script:

#!/bin/bash
# Extract relevant code from crates/prover_services/sharp_service/src/lib.rs to review the instantiation and usage of FactChecker.
rg --type rust -A 10 'FactChecker' crates/prover_services/sharp_service/src/lib.rs

Length of output: 1385



Script:

#!/bin/bash
# Search for calls to the `is_valid` method within crates/prover_services/sharp_service/src/lib.rs.
rg --type rust 'is_valid' crates/prover_services/sharp_service/src/lib.rs

Length of output: 135



Script:

#!/bin/bash
# Extract more lines around the `is_valid` call in crates/prover_services/sharp_service/src/lib.rs to understand its context.
rg --type rust -A 10 -B 10 'is_valid' crates/prover_services/sharp_service/src/lib.rs

Length of output: 1094

crates/prover_services/sharp_service/src/client.rs (1)

10-40: The SharpClient struct and its methods for adding jobs and checking job status are well implemented, using reqwest for network requests and handling errors robustly. Ensure that the error handling paths are thoroughly tested, especially the custom error types.

crates/prover_services/prover_service/src/lib.rs (1)

4-44: The ProverService trait and related types are well defined, providing a clear abstraction for proof services. The error handling is comprehensive and well structured, covering a wide range of potential issues.

Cargo.toml (2)

7-10: Added new workspace members for prover services.

Ensure that the directory structure and module initialization in Rust are correctly set up for these new crates.


22-64: Dependencies for various services and utilities have been added.

Verify that all added dependencies are used in the project and are compatible with each other to avoid conflicts.

crates/prover_services/gps_fact_checker/src/error.rs (1)

1-51: Comprehensive error handling for the GPS fact checker.

Ensure that all potential error cases are handled in the calling functions to maintain robustness.

crates/prover_services/gps_fact_checker/src/fact_info.rs (1)

1-72: Implementation of fact information retrieval and processing.

Review the usage of unwrap and ensure proper error handling is in place to avoid panics in production.

crates/orchestrator/src/queue/job_queue.rs (1)

12-13: Integration of job processing and verification functions.

Ensure that the job processing functions are thoroughly tested, especially with different types of jobs and error scenarios.

crates/prover_services/sharp_service/src/lib.rs (1)

22-71: Implementation of the SharpProverService for interacting with the SHARP API.

Ensure that error handling is robust, especially in network communication and data parsing scenarios.

crates/orchestrator/src/jobs/types.rs (1)

74-77: Updated job type definitions to reflect new functionalities.

Ensure that all parts of the system that use these job types are updated to handle the new definitions correctly.

crates/orchestrator/src/database/mongodb/mod.rs (1)

Line range hint 24-43: Optimistic locking implemented for job updates in MongoDB.

Verify that the versioning mechanism is correctly implemented in all parts of the system that modify job data to prevent update conflicts.

crates/orchestrator/src/config.rs (1)

Line range hint 88-122: Configuration setup for various services in the orchestrator.

Ensure that the configuration is flexible and can be easily modified to accommodate changes in the underlying services or infrastructure.

crates/prover_services/gps_fact_checker/src/fact_node.rs (1)

31-41: FactNode structure is well-defined and appropriate for its use case.

crates/orchestrator/src/jobs/da_job/mod.rs (1)

11-14: Ensure proper error handling in DaJob methods.

While the methods seem to handle errors, ensure that all potential error cases are covered, especially in asynchronous operations. Consider adding more comprehensive error logging and recovery mechanisms.

crates/orchestrator/src/jobs/mod.rs (2)

17-17: Job trait is well-defined and covers essential methods needed for job handling.


103-103: Ensure robust error handling in verify_job.

While the function handles different verification statuses, ensure that all error cases are thoroughly handled, especially in asynchronous operations. Consider adding more detailed error logging and recovery mechanisms.

crates/prover_services/gps_fact_checker/src/artifacts/FactRegistry.json (1)

1-1: ABI definitions for hasRegisteredFact and isValid are correctly structured.

crates/prover_services/gps_fact_checker/src/fact_node.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Show resolved Hide resolved
crates/prover_services/stone_service/src/integrity.rs Outdated Show resolved Hide resolved
crates/prover_services/stone_service/src/sovereign.rs Outdated Show resolved Hide resolved
crates/prover_services/stone_service/src/gps.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/prover_job/mod.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 8a2f0c3 and bfc6d9f.
Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (5)
  • crates/prover_services/gps_fact_checker/Cargo.toml (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_info.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_node.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_topology.rs (1 hunks)
  • crates/utils/src/lib.rs (1 hunks)
Files skipped from review as they are similar to previous changes (5)
  • crates/prover_services/gps_fact_checker/Cargo.toml
  • crates/prover_services/gps_fact_checker/src/fact_info.rs
  • crates/prover_services/gps_fact_checker/src/fact_node.rs
  • crates/prover_services/gps_fact_checker/src/fact_topology.rs
  • crates/utils/src/lib.rs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between bfc6d9f and 4a6fccd.
Files selected for processing (4)
  • crates/prover_services/prover_service/src/lib.rs (1 hunks)
  • crates/prover_services/sharp_service/src/error.rs (1 hunks)
  • crates/prover_services/sharp_service/src/lib.rs (1 hunks)
  • crates/prover_services/stone_service/src/lib.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • crates/prover_services/prover_service/src/lib.rs
  • crates/prover_services/sharp_service/src/error.rs
Additional comments not posted (4)
crates/prover_services/stone_service/src/lib.rs (1)

41-44: Constructor implementation looks good.

The constructor with_settings correctly uses dependency injection to initialize the registry. This is a good practice for maintainability and testability.

crates/prover_services/sharp_service/src/lib.rs (3)

21-25: Struct definition is appropriate.

The SharpProverService struct is well-defined with fields for sharp_client and fact_checker, which are essential for its functionality.


27-65: Method implementations are comprehensive and handle various scenarios.

The methods submit_task and get_task_status in SharpProverService are well-implemented, covering various scenarios and incorporating error handling effectively.


81-92: Utility functions for task ID manipulation are correctly implemented.

The functions combine_task_id and split_task_id are correctly implemented using standard libraries and include appropriate error handling.

crates/prover_services/stone_service/src/lib.rs Outdated Show resolved Hide resolved
crates/prover_services/stone_service/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Out of diff range and nitpick comments (3)
crates/prover_services/gps_fact_checker/src/artifacts/README.md (3)

9-9: Consider rephrasing to enhance clarity.

Instead of "In order to generate zip compressed Cairo PIEs follow the instructions at", consider "To generate zip-compressed Cairo PIEs, follow the instructions at".


17-17: Ensure fenced code blocks are properly formatted.

Add blank lines before and after the fenced code blocks for proper markdown formatting.

Also applies to: 28-28


12-12: Improve list formatting.

Add blank lines before and after the list for better markdown formatting.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 4a6fccd and eae8505.
Files ignored due to path filters (1)
  • crates/prover_services/gps_fact_checker/src/artifacts/fibonacci.zip is excluded by !**/*.zip
Files selected for processing (5)
  • crates/prover_services/gps_fact_checker/src/artifacts/README.md (1 hunks)
  • crates/prover_services/gps_fact_checker/src/artifacts/get_fact.py (1 hunks)
  • crates/prover_services/gps_fact_checker/src/error.rs (1 hunks)
  • crates/prover_services/gps_fact_checker/src/fact_info.rs (1 hunks)
  • crates/prover_services/sharp_service/src/config.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • crates/prover_services/gps_fact_checker/src/artifacts/get_fact.py
Files skipped from review as they are similar to previous changes (3)
  • crates/prover_services/gps_fact_checker/src/error.rs
  • crates/prover_services/gps_fact_checker/src/fact_info.rs
  • crates/prover_services/sharp_service/src/config.rs
Additional Context Used
LanguageTool (2)
crates/prover_services/gps_fact_checker/src/artifacts/README.md (2)

Near line 8: Consider a shorter alternative to avoid wordiness.
Context: ...the repo using Foundry. ## Cairo PIEs In order to generate zip compressed Cairo PIEs foll...


Near line 23: Possible missing comma found.
Context: ...facts To create test vectors for SHARP facts you would need to install the Cairo0 to...

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between eae8505 and f4a4e65.
Files ignored due to path filters (3)
  • Cargo.lock is excluded by !**/*.lock
  • crates/prover_services/sharp_service/src/artifacts/fibonacci.zip is excluded by !**/*.zip
  • crates/prover_services/sharp_service/src/artifacts/print.zip is excluded by !**/*.zip
Files selected for processing (6)
  • Cargo.toml (2 hunks)
  • crates/prover_services/sharp_service/Cargo.toml (1 hunks)
  • crates/prover_services/sharp_service/src/client.rs (1 hunks)
  • crates/prover_services/sharp_service/src/config.rs (1 hunks)
  • crates/prover_services/sharp_service/src/error.rs (1 hunks)
  • crates/prover_services/sharp_service/src/lib.rs (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • Cargo.toml
  • crates/prover_services/sharp_service/Cargo.toml
  • crates/prover_services/sharp_service/src/client.rs
  • crates/prover_services/sharp_service/src/config.rs
  • crates/prover_services/sharp_service/src/error.rs
  • crates/prover_services/sharp_service/src/lib.rs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between f4a4e65 and f2289ed.
Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (2)
  • crates/orchestrator/Cargo.toml (1 hunks)
  • crates/orchestrator/src/jobs/prover_job/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • crates/orchestrator/Cargo.toml
  • crates/orchestrator/src/jobs/prover_job/mod.rs

@unstark unstark force-pushed the feat/prover-traits branch 2 times, most recently from 081b35b to cd95f27 Compare June 15, 2024 20:39
crates/orchestrator/src/jobs/prover_job/mod.rs Outdated Show resolved Hide resolved
crates/prover_services/prover_service/src/lib.rs Outdated Show resolved Hide resolved
}

pub enum Task {
CairoPie(CairoPie),
Copy link
Contributor

Choose a reason for hiding this comment

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

what sort of other tasks do you think we will have in the future for the prover service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be encoded PIE to avoid deserialization/serialization when reading from DB -> sending to the prover service. The problem with SHARP is that you still have to decode in order to calculate fact, but this is not a problem with e.g. Madara prover API

snos::sharp::pie::encode_pie_mem(cairo_pie).map_err(ProverServiceError::PieEncoding)?;
let res = self.sharp_client.add_job(&encoded_pie).await?;
if let Some(job_key) = res.cairo_job_key {
Ok(combine_task_id(&job_key, &fact_info.fact))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to combine it if we're anyways going to split and use the job_key only later on? if the purpose is to store the fact so that it can be reused, then we can do that by storing it inside the metadata as well. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that it is know by the sharp service only, i.e. the Task type is completely opaque. Generally I don't think we will need fact on the top level, and if so it would be better to introduce a separate method for calculating the fact or modify the submit_task signature

crates/prover_services/sharp_service/src/lib.rs Outdated Show resolved Hide resolved
crates/prover_services/sharp_service/src/lib.rs Outdated Show resolved Hide resolved
e2e-tests/src/lib.rs Outdated Show resolved Hide resolved
e2e-tests/test_prover_jobs.rs Outdated Show resolved Hide resolved
@unstark unstark force-pushed the feat/prover-traits branch 7 times, most recently from 1f35cf1 to 07db0af Compare June 19, 2024 14:21
@apoorvsadana apoorvsadana merged commit cb74280 into madara-alliance:main Jun 19, 2024
6 checks passed
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