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.

Sign up for GitHub

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

Jump to bottom

chainid and version as globals in contracts #910

Merged
merged 8 commits into from
Jun 27, 2023
Merged

chainid and version as globals in contracts #910

merged 8 commits into from
Jun 27, 2023

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Jun 26, 2023 •
edited
Loading

Description

Fixes #912.

Todo:

  • Add test using globals in nested calls

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@LHerskind LHerskind linked an issue Jun 26, 2023 that may be closed by this pull request
feat: Access chainid and version from noir private functions #912
Closed
@LHerskind LHerskind force-pushed the lh/globals-in-noir branch from 8a6cbcc to 9dc4d7e Compare June 26, 2023 17:44
@LHerskind LHerskind changed the title [WIP] chainid and version as globals in contracts chainid and version as globals in contracts Jun 26, 2023
Maddiaa0
Maddiaa0 approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Overall looks great, have a nit question over whether the private_global_variables should just live within the call_context, and the call context can diverge for public and private. This isnt blocking approval but just a discussion starter.

circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp
@@ -242,6 +258,8 @@ void write(std::vector<uint8_t>& buf, PrivateCircuitPublicInputs<NCT> const& pri
write(buf, pis.historic_l1_to_l2_messages_tree_root);

write(buf, pis.contract_deployment_data);
write(buf, pis.chain_id);
Copy link
Member

Choose a reason for hiding this comment

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

While we are here we could probably alter this to use the MSGPACK_FIELDS() macro

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it needs changes quite a few places to use MSGPACK_FIELDS() consistently (in other part os the code as well), so might be better to put that off to a separate cleanup PR.

yarn-project/noir-contracts/src/contracts/noir-aztec3/src/oracle/call_private_function.nr Outdated
@@ -1,16 +1,17 @@
// @todo @LHerskind Looks like the length is hardcoded without a constant here. Should have a description. Seems like number of public inputs + 5
Copy link
Member

Choose a reason for hiding this comment

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

flagging!

yarn-project/noir-contracts/src/contracts/noir-aztec3/src/abi.nr
@@ -19,6 +19,8 @@ struct PrivateContextInputs {
roots: CommitmentTreesRoots,

contract_deployment_data: ContractDeploymentData,

private_global_variables: PrivateGlobalVariables,
Copy link
Member

Choose a reason for hiding this comment

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

I think these should belong in the call_context, rather than being in their own section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was not put in the call_context as those values could change throughout execution, e.g., msg_sender etc.

Seemed better to have the constants separately to that.

Maddiaa0
Maddiaa0 approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

Overall looks great, have a nit question over whether the private_global_variables should just live within the call_context, and the call context can diverge for public and private. This isnt blocking approval but just a discussion starter.

@LHerskind LHerskind force-pushed the lh/globals-in-noir branch from c5f3d53 to 50e4b5a Compare June 27, 2023 11:34
@LHerskind LHerskind marked this pull request as ready for review June 27, 2023 12:19
rahul-kothari
rahul-kothari reviewed Jun 27, 2023
View reviewed changes
circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp
@@ -66,7 +70,8 @@ template <typename NCT> class PrivateCircuitPublicInputs {
historic_nullifier_tree_root == other.historic_nullifier_tree_root &&
historic_contract_tree_root == other.historic_contract_tree_root &&
historic_l1_to_l2_messages_tree_root == other.historic_l1_to_l2_messages_tree_root &&
contract_deployment_data == other.contract_deployment_data;
contract_deployment_data == other.contract_deployment_data && chain_id == other.chain_id &&
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add these in different lines to be coherent and easily spottable.

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 is when formatted, notice the first line also have two values, both call_context and args_hash.

yarn-project/acir-simulator/src/client/private_execution.test.ts
@@ -49,7 +49,7 @@ describe('Private Execution test suite', () => {

const historicRoots = PrivateHistoricTreeRoots.empty();
const contractDeploymentData = ContractDeploymentData.empty();
const txContext = new TxContext(false, false, false, contractDeploymentData, Fr.ZERO, Fr.ZERO);
const txContext = new TxContext(false, false, false, contractDeploymentData, new Fr(69), new Fr(420));
Copy link
Contributor

Choose a reason for hiding this comment

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

yarn-project/circuits.js/src/structs/kernel/__snapshots__/index.test.ts.snap
@@ -337,6 +337,8 @@ function_tree_root: 0x3
contract_address_salt: 0x4
portal_contract_address: 0x505050505050505050505050505050505050505

chain_id: 0x2111
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these exactly the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confused Thinking GIF

WDYM? 2111 != 2211

@LHerskind LHerskind merged commit a8da98f into master Jun 27, 2023
@LHerskind LHerskind deleted the lh/globals-in-noir branch June 27, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maddiaa0 Maddiaa0 Maddiaa0 approved these changes

@rahul-kothari rahul-kothari rahul-kothari approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Access chainid and version from noir private functions
3 participants
@LHerskind @rahul-kothari @Maddiaa0