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

Feature/701 execute circuit as part of workstep execution #720

Merged

Conversation

ognjenkurtic
Copy link
Collaborator

Description

This PR prepares everything for the execution of the circuit as part of transactionExecution in the transaction agent. It also introduces and documents a temporary convention by which circuits are connected to worksteps. It also introduces a transactionResult object which stores the results of the execution for future bpi account storage.

Related Issue

#701

Motivation and Context

Sets up the stage for actually executing a circuit and storing the data.

How Has This Been Tested

Manual test by running npm run test and npm run build. More extensive testing, both manual and automated, will follow.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Request to be added as a Code Owner/Maintainer

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I commit to abide by the Responsibilities of Code Owners/Maintainers.

@ognjenkurtic ognjenkurtic force-pushed the feature/701_execute_circuit_as_part_of_workstep_execution branch from b1e96ce to 54a13fa Compare August 11, 2023 21:27
// to connect worksteps with circuits on the file system.
// Format is: <path_from_env>/<workstep_name_in_snake_case>_<predefined_suffix>.
// Will be ditchedcompletely as part of milestone 5.
private constructCircuitPathsFromWorkstepName(name: string): {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These paths are autogenerated by snarkjs library when npm run snarkjs:circuit {circuitName} is executed. So the naming convention is determined by the snarkjs. We can pass snakeCaseWorkstepName_circuit as the circuitName when running npm run snarkjs:circuit, it will generate the files with the appropriate names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this makes sense. We want all compiled cirucits and accompanying artifacts to follow a predefined convention - see comment below.

@@ -16,5 +16,7 @@ BPI_NATS_SERVER_USER="bpi_operator"
BPI_NATS_SERVER_PASS="liftboiliftboiliftboiliftboi1"
BPI_ENCRYPTION_KEY_K_PARAM="yzkXp3vY_AZQ3YfLv9GMRTYkjUOpn9x18gPkoFvoUxQ"
BPI_ENCRYPTION_KEY_KTY_PARAM="oct"
SNARKJS_CIRCUIT_KEYS_PATH="zeroKnowledgeKeys/circuit/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove these variables as the paths are autogenerated by the snarkjs library when the npm run snarkjs:circuit {circuitName} script is executed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking in the direction that a workgroup admin would be required to provide a compiled circuit and all the necessary artifacts during the setup phase. They would generate those in their own environment and just place it in a predetermined folder where the BPI has access to it, and would have to follow a predefined naming convention.

This is why i think it makes sense to have the parh as part of configuration, because the BPI admin would be able to choose where to store the artifacts. If you agree, i would keep this config value with slight adjustments based on your inputs.

Copy link
Collaborator

@biscuitdey biscuitdey Aug 20, 2023

Choose a reason for hiding this comment

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

Ok we can allow workgroup admins to provide compiled circuit and all the related artifacts.

@biscuitdey biscuitdey self-requested a review August 20, 2023 09:38
Copy link
Collaborator

@biscuitdey biscuitdey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@Therecanbeonlyone1969 Therecanbeonlyone1969 left a comment

Choose a reason for hiding this comment

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

LGTM

@ognjenkurtic ognjenkurtic merged commit 1496e75 into main Aug 20, 2023
1 check passed
@ognjenkurtic ognjenkurtic deleted the feature/701_execute_circuit_as_part_of_workstep_execution branch August 20, 2023 20:21
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.

3 participants