Skip to content

chore: fixing noir warnings#15856

Merged
nventuro merged 11 commits into
nextfrom
07-21-chore_fixing_noir_warnings
Jul 25, 2025
Merged

chore: fixing noir warnings#15856
nventuro merged 11 commits into
nextfrom
07-21-chore_fixing_noir_warnings

Conversation

@benesjan

@benesjan benesjan commented Jul 21, 2025

Copy link
Copy Markdown
Contributor

Fixing the vast majority of compiler warnings in noir-projects.

Tackling this now as incoming Noir release will disallow accessing of private fields and private functionality.

These warnings are remaining:

image

The first one is impractical to fix, the 2nd, 3rd and 4th I am not sure how to fix and the last one I plan on making the AVM team fix.

Copy link
Copy Markdown
Contributor Author

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

@benesjan benesjan force-pushed the 07-21-chore_fixing_noir_warnings branch from f03d6c1 to fd9b776 Compare July 24, 2025 06:44
@benesjan benesjan force-pushed the 07-21-chore_fixing_noir_warnings branch from fd9b776 to ba0d164 Compare July 24, 2025 08:39
@benesjan benesjan force-pushed the 07-21-chore_fixing_noir_warnings branch from 92da17d to 5e9d24e Compare July 24, 2025 10:02

fn next_counter(&mut self) -> u32 {
// This function got exposed publicly to be able to access it in ContractClassRegistry.
// TODO: Should we introduce an abstraction such that accessing this externally is not needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think unblocking the next noir sync is more important than this so just lazily added here and below a TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change is trivial, we just need a method for pushing contract class log hashes

@@ -1,4 +1,4 @@
mod balance_utils;
pub mod balance_utils;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having this in the value note in the first place is strange. Probably worthy of moving in a separate PR

aztec = { path = "../../../../aztec-nr/aztec" }
keccak256 = { tag = "v0.1.0", git = "https://github.com/noir-lang/keccak256" }
sha256 = { tag = "v0.1.2", git = "https://github.com/noir-lang/sha256" }
poseidon = { tag= "v0.1.1", git = "https://github.com/noir-lang/poseidon" }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The poseidon function are no longer exposed on noir::std so I am exposing this here.

@benesjan benesjan marked this pull request as ready for review July 24, 2025 10:39
@fcarreiro fcarreiro removed their request for review July 24, 2025 10:46
@benesjan benesjan requested review from Thunkar and nventuro July 24, 2025 12:00

@dbanks12 dbanks12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

public context looks good


fn next_counter(&mut self) -> u32 {
// This function got exposed publicly to be able to access it in ContractClassRegistry.
// TODO: Should we introduce an abstraction such that accessing this externally is not needed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change is trivial, we just need a method for pushing contract class log hashes

unconstrained fn call(

// This function is temporarily exposed publicly to be able to test it in AVMTest contract.
// TODO: Refactor tests to keep this implementation detail private within the crate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you planning on doing this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Created an issue and will try to figure this out next week.

Comment thread noir-projects/aztec-nr/aztec/src/macros/functions/mod.nr Outdated
Comment thread noir-projects/aztec-nr/uint-note/src/uint_note.nr Outdated
Comment thread noir-projects/noir-contracts/contracts/app/card_game_contract/src/cards.nr Outdated
…src/cards.nr

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
@benesjan benesjan marked this pull request as draft July 25, 2025 06:14
…e` method instead of direct field assignment for commitment.
@benesjan benesjan marked this pull request as ready for review July 25, 2025 06:49
@benesjan benesjan requested a review from nventuro July 25, 2025 06:51
@nventuro nventuro added this pull request to the merge queue Jul 25, 2025
Merged via the queue into next with commit 9e45ef1 Jul 25, 2025
5 checks passed
@nventuro nventuro deleted the 07-21-chore_fixing_noir_warnings branch July 25, 2025 15:39
nventuro added a commit that referenced this pull request Jul 25, 2025
Fixing the vast majority of compiler warnings in `noir-projects`.

Tackling this now as incoming Noir release will disallow accessing of
private fields and private functionality.

These warnings are remaining:

<img width="1128" height="523" alt="image"
src="https://github.com/user-attachments/assets/a9372a2b-ce93-41f3-9a4b-ffa96b60ba5f"
/>

The first one is impractical to fix, the 2nd, 3rd and 4th I am not sure
how to fix and the last one I plan on making the AVM team fix.

---------

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
github-merge-queue Bot pushed a commit that referenced this pull request Jul 30, 2025
Fixes #15980

In [this PR](#15856)
I incorrectly exposed a couple of functions. In this PR I un-expose
both. The `next_counter` got tackled by me moving the
`emit_contract_class_log` to private context which is where it actually
should be.

For the `call` function I decided to just disable the test funcitons in
which it was used. It's up to the AVM team to re-enable those tests.
Created [an
issue](#16099) for
that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants