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

feat: add inclusion proofs txe tests #11711

Open
wants to merge 2 commits into
base: ek/fix/make-block-number-in-txe-make-sense
Choose a base branch
from
Open

feat: add inclusion proofs txe tests #11711

wants to merge 2 commits into from
+378 −313

Conversation

sklppy88
Copy link
Contributor

@sklppy88 sklppy88 commented Feb 4, 2025 •
edited
Loading

Adding inclusion proof txe tests to hopefully replace flaky_e2e_inclusion_tests

This was referenced Feb 4, 2025
Merged
Closed
@sklppy88 Graphite App
Copy link
Contributor Author

sklppy88 commented Feb 4, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@sklppy88 sklppy88 mentioned this pull request Feb 4, 2025
Merged
@sklppy88 sklppy88 changed the title init feat: add inclusion proofs txe test Feb 4, 2025
@sklppy88 sklppy88 changed the title feat: add inclusion proofs txe test feat: add inclusion proofs txe tests Feb 4, 2025
@sklppy88 sklppy88 changed the base branch from ek/fix/txe-block-headers to graphite-base/11711 February 4, 2025 16:11
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from 93133f7 to d31fc38 Compare February 4, 2025 16:55
@sklppy88 sklppy88 mentioned this pull request Feb 5, 2025
Closed
@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from ae7c789 to 65b3357 Compare February 5, 2025 12:03
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from d31fc38 to 3410f84 Compare February 5, 2025 12:03
@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from 65b3357 to 20d86fd Compare February 5, 2025 13:00
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch 3 times, most recently from 1abffff to 282a5bc Compare February 5, 2025 13:40
@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from 20d86fd to aa5d4ba Compare February 5, 2025 13:40
@sklppy88 sklppy88 marked this pull request as ready for review February 5, 2025 15:17
@sklppy88 sklppy88 requested review from benesjan and Thunkar February 5, 2025 17:24
benesjan
benesjan reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

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

Why not delete the e2e test?

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr Outdated
let owner = env.create_account();
env.impersonate(owner);

// Advance a block so we know that at block 2 our contract has not been deployed yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah reading this back its definitely not thorough enough. Addressed in 20a0708

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr

#[test]
unconstrained fn note_flow() {
let (env, contract_address, owner) = setup(INITIAL_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add here more comments regarding what you are testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, have added more comments in general in 20a0708 !

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr Outdated
}

#[test(should_fail_with = "Nullifier witness not found for nullifier")]
unconstrained fn test_contract_not_initialized() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unconstrained fn test_contract_not_initialized() {
unconstrained fn contract_not_initialized() {

for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah very good point thank you 🙏. Have reworked some of the naming in 20a0708

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr Outdated
}

#[test(should_fail_with = "Nullifier witness not found for nullifier")]
unconstrained fn test_contract_not_deployed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems to be equal to the one above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm to me it seems to not be equal to the above test, one checks for the fail of initialization, the other for the fail of deployment. But I think it's definitely not clear enough so thank you for pointing that out. Have added some comments to explain this in 20a0708

Thunkar
Thunkar reviewed Feb 6, 2025
View reviewed changes
noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr Outdated
let NOTE_VALUE = 69;
InclusionProofs::at(contract_address).create_note(owner, NOTE_VALUE).call(&mut env.private());

env.advance_block_by(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 2?

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'm not sure anymore 😅. Changed to the default (1) in 20a0708

@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from aa5d4ba to c96f3fb Compare February 6, 2025 10:06
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch 2 times, most recently from d5127ab to f317db5 Compare February 7, 2025 10:09
@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from c96f3fb to bb58c87 Compare February 7, 2025 10:09
@sklppy88 sklppy88 changed the base branch from graphite-base/11711 to master February 7, 2025 10:09
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from f317db5 to df09858 Compare February 7, 2025 11:18
@sklppy88 sklppy88 changed the base branch from master to graphite-base/11711 February 7, 2025 12:52
@sklppy88 sklppy88 force-pushed the graphite-base/11711 branch from baa69a2 to edbd170 Compare February 7, 2025 12:52
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from df09858 to 0882c5b Compare February 7, 2025 12:52
@sklppy88 sklppy88 changed the base branch from graphite-base/11711 to ek/fix/make-block-number-in-txe-make-sense February 7, 2025 12:52
@sklppy88 sklppy88 mentioned this pull request Feb 7, 2025
Open
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from 0882c5b to 353748e Compare February 7, 2025 13:05
@sklppy88 sklppy88 force-pushed the ek/fix/make-block-number-in-txe-make-sense branch from edbd170 to 3bb2506 Compare February 7, 2025 13:05
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch 5 times, most recently from 955f6b0 to a750023 Compare February 7, 2025 16:35
@sklppy88 sklppy88 force-pushed the ek/fix/make-block-number-in-txe-make-sense branch from 3bb2506 to 57bee7d Compare February 7, 2025 16:35
@sklppy88 sklppy88 force-pushed the ek/fix/make-block-number-in-txe-make-sense branch from 57bee7d to e763b46 Compare February 7, 2025 17:17
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch 2 times, most recently from f2aa25e to c80549f Compare February 7, 2025 17:44
@sklppy88 sklppy88 force-pushed the ek/feat/add-inclusion-proofs-txe-tests branch from c80549f to 20a0708 Compare February 7, 2025 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thunkar Thunkar Thunkar left review comments

@benesjan benesjan benesjan left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@sklppy88 @Thunkar @benesjan