Adarsh/per decorator #479
Adarsh/per decorator #479
Conversation
lydiagarms
left a comment
lydiagarms
left a comment
There was a problem hiding this comment.
Really nice work.
There was a problem hiding this comment.
I think for the merge into Starlight, this file could be deleted?
| // We need to hard-code the mappingId's of mappings into the circuit: | ||
| field ${m}_mappingId = ${mappingId};`, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
maybe just fix the indenting here
| @@ -48,11 +48,15 @@ function saveMetadata ( | |||
| // console.log("hardhatArtifactPath: ", hardhatArtifactPath); | |||
|
|
|||
| const compilationData = fs.readFileSync(hardhatArtifactPath, 'utf-8') | |||
There was a problem hiding this comment.
Why are these changes necessary?
| @@ -80,7 +80,13 @@ export async function getContractInstance(contractName, deployedAddress) { | |||
|
|
|||
| export async function getContractBytecode(contractName) { | |||
There was a problem hiding this comment.
Why is this change necessary?
| commitmentRoutes(): string { | ||
| return `// commitment getter routes | ||
| router.get("/getAllCommitments", service_allCommitments); | ||
| router.get("/getCommitmentsByVariableName", service_getCommitmentsByState); |
There was a problem hiding this comment.
Why do we now need to make these post?
| @@ -0,0 +1,78 @@ | |||
| // SPDX-License-Identifier: CC0 | |||
There was a problem hiding this comment.
Maybe as this is the test contract that will be used for per, we could include multiple domain parameters, which would give a syntactical guide and allow for better testing? We'll also need to add this to the tests.
| @@ -0,0 +1,78 @@ | |||
| // SPDX-License-Identifier: CC0 | |||
There was a problem hiding this comment.
We should also add this new functionality to the readme.
|
|
||
| // this adds other values we need in the circuit | ||
| for (const param of node._newASTPointer.parameters.parameters) { | ||
| // Get the circuit AST function node to determine the correct parameter order |
There was a problem hiding this comment.
I would rather not use the circuit AST, in order to keep the orchestration and circuit components of Starlight separate, can we remove this and just fall back to the solidity AST immediately as in line 1016
| if ( para?.name === param?.name ) | ||
| oldParam = para ; | ||
| break; | ||
| for(const para of node.parameters.parameters) { |
There was a problem hiding this comment.
Are we intending to change the logic here? Is this a bug being fixed?
|
|
||
| updateOwnership(ownerNode: any, msgIsMappingKeyorMappingValue?: string | null) { | ||
| if (this.isOwned && this.owner.mappingOwnershipType === 'key') return; | ||
|
|
There was a problem hiding this comment.
What bug is this fixing? It seems related to a bug Sebastian found last PI. If this is already fixed we can probably remove this change?
2 participants
Summary
Related Issues
Changes
Checklist
How to test
Screenshots / Evidence (if applicable)