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

refactor: nuke unused getSiblingPath oracle #11090

Merged
merged 6 commits into from
Jan 8, 2025

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Jan 7, 2025

Made sense to nuke it as it was unused. getMembershipWitness oracle was used instead.

Copy link
Contributor Author

benesjan commented Jan 7, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan added the e2e-all CI: Enables this CI job. label Jan 7, 2025
@benesjan benesjan marked this pull request as ready for review January 7, 2025 18:50
const [leafIndex] = await this.aztecNode.findLeavesIndexes(blockNumber, treeId, [leafValue]);
return leafIndex;
}

public async getSiblingPath(blockNumber: number, treeId: MerkleTreeId, leafIndex: bigint): Promise<Fr[]> {
public async getMembershipWitness(blockNumber: number, treeId: MerkleTreeId, leafValue: Fr): Promise<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.

Made sense to have just getMembershipWitness exposed here as that's what the consumers of the API really wanted.

@benesjan benesjan changed the title refactor: oracles cleanup refactor: nuke unused getSiblingPath oracle Jan 7, 2025
@AztecBot
Copy link
Collaborator

AztecBot commented Jan 7, 2025

Docs Preview

Hey there! 👋 You can check your preview at https://677ee7e76011d91702e67928--aztec-docs-dev.netlify.app

@benesjan benesjan force-pushed the 01-07-refactor_oracles_cleanup branch from 534de00 to 3a0060d Compare January 7, 2025 19:13
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

I'm confused by this - it seems this oracle is only called in the following way:

pub unconstrained fn get_membership_witness<let N: u32, let M: u32>(
    block_number: u32,
    tree_id: Field,
    leaf_value: Field,
) -> MembershipWitness<N, M> {
    let fields: [Field; M] = get_membership_witness_oracle(block_number, tree_id, leaf_value);
    MembershipWitness { index: fields[0], path: array::subarray(fields, 1) }
}

If we discard the leaf anyway, shouldn't we delete getMemberShipWitness and replace it for getSiblingPath instead? What's the purpose of the leaf index?

docs/docs/migration_notes.md Outdated Show resolved Hide resolved
@benesjan
Copy link
Contributor Author

benesjan commented Jan 8, 2025

If we discard the leaf anyway, shouldn't we delete getMemberShipWitness and replace it for getSiblingPath instead? What's the purpose of the leaf index?

@nventuro The leaf index is used to figure out from which side to start hashing the sibling path and it's used for example here.

Where did you see that the index get discarded? I could not find a callsite of the oracle where that happens.

@benesjan benesjan force-pushed the 01-07-refactor_oracles_cleanup branch from 8cad9b9 to 67d8302 Compare January 8, 2025 15:44
@benesjan benesjan requested a review from nventuro January 8, 2025 16:08
@nventuro
Copy link
Contributor

nventuro commented Jan 8, 2025

You're right, I misread the very snippet I pasted 🤦

@benesjan benesjan enabled auto-merge (squash) January 8, 2025 18:48
@benesjan benesjan force-pushed the 01-07-refactor_oracles_cleanup branch from 3799b13 to 58da308 Compare January 8, 2025 20:44
@benesjan benesjan merged commit 36b640a into master Jan 8, 2025
78 checks passed
@benesjan benesjan deleted the 01-07-refactor_oracles_cleanup branch January 8, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants