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

feat!: tree leaf value as Fr everywhere in our public API #3173

Merged
merged 4 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions yarn-project/acir-simulator/src/client/view_data_oracle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,8 @@ export class ViewDataOracle extends TypedOracle {
throw new Error(`Oracle storage read undefined: slot=${storageSlot.toString(16)}`);
}

const frValue = Fr.fromBuffer(value);
this.log(`Oracle storage read: slot=${storageSlot.toString(16)} value=${frValue}`);
values.push(frValue);
this.log(`Oracle storage read: slot=${storageSlot.toString(16)} value=${value}`);
values.push(value);
}
return values;
}
Expand Down
5 changes: 2 additions & 3 deletions yarn-project/acir-simulator/src/public/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { FunctionArtifact, FunctionSelector, encodeArguments } from '@aztec/foun
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { EthAddress } from '@aztec/foundation/eth-address';
import { Fr } from '@aztec/foundation/fields';
import { toBigInt } from '@aztec/foundation/serialize';
import {
ChildContractArtifact,
ParentContractArtifact,
Expand Down Expand Up @@ -240,8 +239,8 @@ describe('ACIR public execution simulator', () => {

const functionData = new FunctionData(parentEntryPointFnSelector, isInternal ?? false, false, false);
const args = encodeArguments(parentEntryPointFn, [
childContractAddress.toField().value,
toBigInt(childValueFnSelector.toBuffer()),
childContractAddress.toField(),
childValueFnSelector.toField(),
initialValue,
]);

Expand Down
11 changes: 6 additions & 5 deletions yarn-project/aztec-node/src/aztec-node/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,9 @@ export class AztecNodeService implements AztecNode {
* @param leafValue - The value to search for
* @returns The index of the given leaf in the given tree or undefined if not found.
*/
public async findLeafIndex(treeId: MerkleTreeId, leafValue: Buffer): Promise<bigint | undefined> {
public async findLeafIndex(treeId: MerkleTreeId, leafValue: Fr): Promise<bigint | undefined> {
const committedDb = await this.#getWorldState();
return committedDb.findLeafIndex(treeId, leafValue);
return committedDb.findLeafIndex(treeId, leafValue.toBuffer());
Copy link
Member

Choose a reason for hiding this comment

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

should the find leaf index api also use Fr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt like changing the types in the merkle tree package could get a bit tricky in some places we pass the serialized indexed tree value which should not be Fr. Also we'll nuke the whole package and replace it with Rust or C++ so it's probably ok to keep the code disgusting.

}

/**
Expand Down Expand Up @@ -324,7 +324,7 @@ export class AztecNodeService implements AztecNode {
*/
public async getL1ToL2MessageAndIndex(messageKey: Fr): Promise<L1ToL2MessageAndIndex> {
// todo: #697 - make this one lookup.
const index = (await this.findLeafIndex(MerkleTreeId.L1_TO_L2_MESSAGES_TREE, messageKey.toBuffer()))!;
const index = (await this.findLeafIndex(MerkleTreeId.L1_TO_L2_MESSAGES_TREE, messageKey))!;
Copy link
Member

Choose a reason for hiding this comment

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

wait hold up above uses buffer and this uses FR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am AFK already so I merged it and I will consider addressing this tomorrow.

const message = await this.l1ToL2MessageSource.getConfirmedL1ToL2Message(messageKey);
return Promise.resolve(new L1ToL2MessageAndIndex(index, message));
}
Expand All @@ -346,10 +346,11 @@ export class AztecNodeService implements AztecNode {
* @returns Storage value at the given contract slot (or undefined if not found).
* Note: Aztec's version of `eth_getStorageAt`.
*/
public async getPublicStorageAt(contract: AztecAddress, slot: bigint): Promise<Buffer | undefined> {
public async getPublicStorageAt(contract: AztecAddress, slot: bigint): Promise<Fr | undefined> {
const committedDb = await this.#getWorldState();
const leafIndex = computePublicDataTreeIndex(await CircuitsWasm.get(), contract, new Fr(slot));
return committedDb.getLeafValue(MerkleTreeId.PUBLIC_DATA_TREE, leafIndex.value);
const value = await committedDb.getLeafValue(MerkleTreeId.PUBLIC_DATA_TREE, leafIndex.value);
return value ? Fr.fromBuffer(value) : undefined;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/aztec.js/src/utils/cheat_codes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ export class AztecCheatCodes {
if (storageValue === undefined) {
throw new Error(`Storage slot ${slot} not found`);
}
return Fr.fromBuffer(storageValue);
return storageValue;
}

/**
Expand Down
7 changes: 3 additions & 4 deletions yarn-project/end-to-end/src/e2e_2_pxes.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AztecAddress, Note, Wallet, computeMessageSecretHash } from '@aztec/aztec.js';
import { DebugLogger } from '@aztec/foundation/log';
import { retryUntil } from '@aztec/foundation/retry';
import { toBigInt } from '@aztec/foundation/serialize';
import { ChildContract, TokenContract } from '@aztec/noir-contracts/types';
import { EthAddress, Fr, PXEService } from '@aztec/pxe';
import { AztecNode, CompleteAddress, ExtendedNote, PXE, TxStatus } from '@aztec/types';
Expand Down Expand Up @@ -175,7 +174,7 @@ describe('e2e_2_pxes', () => {
};

const getChildStoredValue = (child: { address: AztecAddress }, pxe: PXE) =>
pxe.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!));
pxe.getPublicStorageAt(child.address, new Fr(1));

it('user calls a public function on a contract deployed by a different user using a different PXE', async () => {
const childCompleteAddress = await deployChildContractViaServerA();
Expand All @@ -191,15 +190,15 @@ describe('e2e_2_pxes', () => {
},
]);

const newValueToSet = 256n;
const newValueToSet = new Fr(256n);

const childContractWithWalletB = await ChildContract.at(childCompleteAddress.address, walletB);
await childContractWithWalletB.methods.pubIncValue(newValueToSet).send().wait({ interval: 0.1 });

await awaitServerSynchronized(pxeA);

const storedValue = await getChildStoredValue(childCompleteAddress, pxeB);
expect(storedValue).toBe(newValueToSet);
expect(storedValue).toEqual(newValueToSet);
});

it('private state is "zero" when Private eXecution Environment (PXE) does not have the account private key', async () => {
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/end-to-end/src/e2e_account_contracts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
Wallet,
} from '@aztec/aztec.js';
import { CompleteAddress, GrumpkinPrivateKey, GrumpkinScalar } from '@aztec/circuits.js';
import { toBigInt } from '@aztec/foundation/serialize';
import { ChildContract } from '@aztec/noir-contracts/types';

import { randomBytes } from 'crypto';
Expand Down Expand Up @@ -56,7 +55,8 @@ function itShouldBehaveLikeAnAccountContract(
const { logger, pxe } = context;
logger('Calling public function...');
await child.methods.pubIncValue(42).send().wait({ interval: 0.1 });
expect(toBigInt((await pxe.getPublicStorageAt(child.address, new Fr(1)))!)).toEqual(42n);
const storedValue = await pxe.getPublicStorageAt(child.address, new Fr(1));
Copy link
Member

Choose a reason for hiding this comment

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

Heres another area where there is inconsistency:

getPublicStorage at sometimes has a bigint and sometimes has a field, as this is just an index, im in favour of this being just a bigint

image

expect(storedValue!).toEqual(new Fr(42n));
}, 60_000);

it('fails to call a function using an invalid signature', async () => {
Expand Down
16 changes: 7 additions & 9 deletions yarn-project/end-to-end/src/e2e_nested_contract.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AztecAddress, BatchCall, Fr, Wallet } from '@aztec/aztec.js';
import { toBigIntBE } from '@aztec/foundation/bigint-buffer';
import { DebugLogger } from '@aztec/foundation/log';
import { toBigInt } from '@aztec/foundation/serialize';
import { ChildContract, ImportTestContract, ParentContract, TestContract } from '@aztec/noir-contracts/types';
import { PXE } from '@aztec/types';

Expand All @@ -28,8 +27,7 @@ describe('e2e_nested_contract', () => {
childContract = await ChildContract.deploy(wallet).send().deployed();
}, 100_000);

const getChildStoredValue = (child: { address: AztecAddress }) =>
pxe.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!));
const getChildStoredValue = (child: { address: AztecAddress }) => pxe.getPublicStorageAt(child.address, new Fr(1));

it('performs nested calls', async () => {
await parentContract.methods
Expand Down Expand Up @@ -58,7 +56,7 @@ describe('e2e_nested_contract', () => {
.enqueueCallToChild(childContract.address, childContract.methods.pubIncValue.selector.toField(), 42n)
.send()
.wait();
expect(await getChildStoredValue(childContract)).toEqual(42n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(42n));
}, 100_000);

it('fails simulation if calling a public function not allowed to be called externally', async () => {
Expand All @@ -74,23 +72,23 @@ describe('e2e_nested_contract', () => {
.enqueueCallToChildTwice(childContract.address, childContract.methods.pubIncValue.selector.value, 42n)
.send()
.wait();
expect(await getChildStoredValue(childContract)).toEqual(85n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(85n));
}, 100_000);

it('enqueues a public call with nested public calls', async () => {
await parentContract.methods
.enqueueCallToPubEntryPoint(childContract.address, childContract.methods.pubIncValue.selector.toField(), 42n)
.send()
.wait();
expect(await getChildStoredValue(childContract)).toEqual(42n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(42n));
}, 100_000);

it('enqueues multiple public calls with nested public calls', async () => {
await parentContract.methods
.enqueueCallsToPubEntryPoint(childContract.address, childContract.methods.pubIncValue.selector.toField(), 42n)
.send()
.wait();
expect(await getChildStoredValue(childContract)).toEqual(85n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(85n));
}, 100_000);

// Regression for https://github.com/AztecProtocol/aztec-packages/issues/640
Expand All @@ -99,7 +97,7 @@ describe('e2e_nested_contract', () => {
.pubEntryPointTwice(childContract.address, childContract.methods.pubIncValue.selector.value, 42n)
.send()
.wait();
expect(await getChildStoredValue(childContract)).toEqual(84n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(84n));
}, 100_000);

// Regression for https://github.com/AztecProtocol/aztec-packages/issues/1645
Expand All @@ -121,7 +119,7 @@ describe('e2e_nested_contract', () => {
).logs;
const processedLogs = extendedLogs.map(extendedLog => toBigIntBE(extendedLog.log.data));
expect(processedLogs).toEqual([20n, 40n]);
expect(await getChildStoredValue(childContract)).toEqual(40n);
expect(await getChildStoredValue(childContract)).toEqual(new Fr(40n));
});
});

Expand Down
9 changes: 4 additions & 5 deletions yarn-project/end-to-end/src/e2e_ordering.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Wallet } from '@aztec/aztec.js';
import { FunctionSelector } from '@aztec/circuits.js';
import { toBigIntBE } from '@aztec/foundation/bigint-buffer';
import { Fr } from '@aztec/foundation/fields';
import { toBigInt } from '@aztec/foundation/serialize';
import { ChildContract, ParentContract } from '@aztec/noir-contracts/types';
import { PXE, TxStatus } from '@aztec/types';

Expand Down Expand Up @@ -80,8 +79,8 @@ describe('e2e_ordering', () => {
await expectLogsFromLastBlockToBe(expectedOrder);

// The final value of the child is the last one set
const value = await pxe.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!));
expect(value).toEqual(expectedOrder[1]); // final state should match last value set
const value = await pxe.getPublicStorageAt(child.address, new Fr(1));
expect(value?.value).toBe(expectedOrder[1]); // final state should match last value set
},
);
});
Expand All @@ -104,8 +103,8 @@ describe('e2e_ordering', () => {
const receipt = await tx.wait();
expect(receipt.status).toBe(TxStatus.MINED);

const value = await pxe.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!));
expect(value).toEqual(expectedOrder[1]); // final state should match last value set
const value = await pxe.getPublicStorageAt(child.address, new Fr(1));
expect(value?.value).toBe(expectedOrder[1]); // final state should match last value set
},
);

Expand Down
3 changes: 1 addition & 2 deletions yarn-project/end-to-end/src/guides/dapp_testing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
getSandboxAccountsWallets,
waitForSandbox,
} from '@aztec/aztec.js';
import { toBigIntBE } from '@aztec/foundation/bigint-buffer';
import { TestContract, TokenContract } from '@aztec/noir-contracts/types';
import { ExtendedNote } from '@aztec/types';

Expand Down Expand Up @@ -166,7 +165,7 @@ describe('guides/dapp/testing', () => {
await token.methods.mint_public(owner.getAddress(), 100n).send().wait();
const ownerPublicBalanceSlot = cheats.aztec.computeSlotInMap(6n, owner.getAddress());
const balance = await pxe.getPublicStorageAt(token.address, ownerPublicBalanceSlot);
expect(toBigIntBE(balance!)).toEqual(100n);
expect(balance!.value).toEqual(100n);
// docs:end:public-storage
});

Expand Down
5 changes: 1 addition & 4 deletions yarn-project/pxe/src/contract_tree/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,7 @@ export class ContractTree {
const root = await this.getFunctionTreeRoot();
const newContractData = new NewContractData(completeAddress.address, portalContract, root);
const commitment = computeContractLeaf(this.wasm, newContractData);
this.contractIndex = await this.stateInfoProvider.findLeafIndex(
MerkleTreeId.CONTRACT_TREE,
commitment.toBuffer(),
);
this.contractIndex = await this.stateInfoProvider.findLeafIndex(MerkleTreeId.CONTRACT_TREE, commitment);
if (this.contractIndex === undefined) {
throw new Error(
`Failed to find contract at ${completeAddress.address} with portal ${portalContract} resulting in commitment ${commitment}.`,
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/pxe/src/pxe_service/pxe_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,14 @@ export class PXEService implements PXE {
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386)
// This can always be `uniqueSiloedNoteHash` once notes added from public also include nonces.
const noteHashToLookUp = nonce.isZero() ? siloedNoteHash : uniqueSiloedNoteHash;
const index = await this.node.findLeafIndex(MerkleTreeId.NOTE_HASH_TREE, noteHashToLookUp.toBuffer());
const index = await this.node.findLeafIndex(MerkleTreeId.NOTE_HASH_TREE, noteHashToLookUp);
if (index === undefined) {
throw new Error('Note does not exist.');
}

const wasm = await CircuitsWasm.get();
const siloedNullifier = siloNullifier(wasm, note.contractAddress, innerNullifier!);
const nullifierIndex = await this.node.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, siloedNullifier.toBuffer());
const nullifierIndex = await this.node.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, siloedNullifier);
if (nullifierIndex !== undefined) {
throw new Error('The note has been destroyed.');
}
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/pxe/src/simulator_oracle/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,11 @@ export class SimulatorOracle implements DBOracle {
* @returns - The index of the commitment. Undefined if it does not exist in the tree.
*/
async getCommitmentIndex(commitment: Fr) {
return await this.stateInfoProvider.findLeafIndex(MerkleTreeId.NOTE_HASH_TREE, commitment.toBuffer());
return await this.stateInfoProvider.findLeafIndex(MerkleTreeId.NOTE_HASH_TREE, commitment);
}

async getNullifierIndex(nullifier: Fr) {
return await this.stateInfoProvider.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, nullifier.toBuffer());
return await this.stateInfoProvider.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, nullifier);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/types/src/interfaces/aztec-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export interface AztecNode extends StateInfoProvider {
* @param slot - Slot to query.
* @returns Storage value at the given contract slot (or undefined if not found).
*/
getPublicStorageAt(contract: AztecAddress, slot: bigint): Promise<Buffer | undefined>;
getPublicStorageAt(contract: AztecAddress, slot: bigint): Promise<Fr | undefined>;

/**
* Returns the current committed roots for the data trees.
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/types/src/interfaces/pxe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export interface PXE {
* @returns A buffer containing the public storage data at the storage slot.
* @throws If the contract is not deployed.
*/
getPublicStorageAt(contract: AztecAddress, storageSlot: Fr): Promise<Buffer | undefined>;
getPublicStorageAt(contract: AztecAddress, storageSlot: Fr): Promise<Fr | undefined>;

/**
* Gets notes of accounts registered in this PXE based on the provided filter.
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/types/src/interfaces/state_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface StateInfoProvider {
* @param leafValue - The value to search for
* @returns The index of the given leaf in the given tree or undefined if not found.
*/
findLeafIndex(treeId: MerkleTreeId, leafValue: Buffer): Promise<bigint | undefined>;
findLeafIndex(treeId: MerkleTreeId, leafValue: Fr): Promise<bigint | undefined>;

/**
* Returns the sibling path for the given index in the contract tree.
Expand Down