Skip to content

feat: compute shares address inside vault constructor#300

Open
ilpepepig wants to merge 22 commits intodevfrom
feat/1-step-vault-initialization
Open

feat: compute shares address inside vault constructor#300
ilpepepig wants to merge 22 commits intodevfrom
feat/1-step-vault-initialization

Conversation

@ilpepepig
Copy link
Copy Markdown
Collaborator

🤖 Linear

Closes AZT-XXX

Description

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 6, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Vault and shares-token deployment moved to deterministic, constructor-driven initialization: vault constructors now accept shares-token constructor inputs and compute the shares address during construction; admin setter entrypoints were removed; TypeScript test deploy utilities were updated to precompute addresses, use a shared salt, deploy shares with the vault as minter, and deploy vaults via constructors (including initial-deposit flows).

Changes

Cohort / File(s) Summary
TypeScript Deployment Utilities
src/ts/test/utils.ts
Reworked deployment helpers to read sharesClassId from TokenContractArtifact, compute deterministic addresses with a shared salt, deploy shares token with contractAddressSalt and minter set to the precomputed vault address, and deploy vault using constructor-based initializers. Removed post-deploy set_shares_token wiring and adapted initial-deposit auth flow.
Vault Contract Core (Noir)
src/vault_contract/src/main.nr
Removed admin storage and admin-only setters (set_shares_token*); replaced previous constructor with constructor(...) and added constructor_with_initial_deposit(...) that accept shares token constructor params and compute shares address deterministically. Added _compute_token_address(...) helper and folded initial-deposit transfer/mint into the constructor.
Noir Test Utilities & Tests
src/vault_contract/src/test/utils.nr, src/vault_contract/src/test/constructor_with_initial_deposit.nr
Test helpers now deploy Vault via without_initializer() and call new public constructors (constructor / constructor_with_initial_deposit) with shares-token params (often placeholders) rather than relying on admin-setters. Updated comments about TXE custom-salt limitations affecting deterministic address matching.

Sequence Diagrams

sequenceDiagram
    participant Deployer
    participant Artifact as TokenContractArtifact
    participant Deriver as AddressDeriver
    participant Token as SharesToken
    participant Vault as VaultContract
    participant Asset as AssetToken

    Note over Deployer,Vault: Deterministic deployment flow (standard / initial-deposit)

    Deployer->>Artifact: fetch shares_class_id
    Artifact-->>Deployer: shares_class_id
    Deployer->>Deriver: derive_vault_address(constructor args, shares_class_id, salt)
    Deriver-->>Deployer: precomputed_vault_address
    Deployer->>Deriver: derive_token_address(token constructor args, salt, deployer)
    Deriver-->>Deployer: precomputed_token_address
    Deployer->>Token: deploy(token args with minter=precomputed_vault_address, contractAddressSalt=salt)
    Token-->>Deployer: token deployed at precomputed_token_address
    Deployer->>Asset: setPublicAuthWit(precomputed_vault_address)   %% only for initial-deposit path
    Asset-->>Deployer: authorization set
    Deployer->>Vault: deploy(constructor | constructor_with_initial_deposit(..., depositor, amount, nonce))
    Vault->>Asset: transferFrom(depositor, amount)
    Asset-->>Vault: assets transferred
    Vault->>Token: mint(depositor or vault, shares)
    Token-->>Vault: shares minted
    Vault-->>Deployer: vault initialized deterministically
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • wei3erHase
  • cubic-dev-ai
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete, containing only boilerplate template text with no actual description, motivation, or context. Provide substantive description explaining the rationale, security implications, and key changes (e.g., eliminating post-deployment wiring via set_shares_token, deterministic address computation during initialization, impacts on test deployment patterns).
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: computing shares address inside the vault constructor rather than post-deployment.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/1-step-vault-initialization

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ilpepepig ilpepepig marked this pull request as ready for review March 11, 2026 02:29
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/vault_contract/src/test/utils.nr">

<violation number="1" location="src/vault_contract/src/test/utils.nr:62">
P2: Extra `owner` argument passed to `constructor()` — the contract only takes 7 parameters (no `deployer`). This will be a compile error when the test module is re-enabled. Remove the trailing `owner` argument.</violation>

<violation number="2" location="src/vault_contract/src/test/utils.nr:118">
P1: Wrong argument order and extra argument in `constructor_with_initial_deposit()` call. `owner` is incorrectly inserted as 8th arg (where `initial_deposit: u128` is expected), shifting all remaining arguments and adding an extra 11th parameter. The contract only takes 10 params — remove `owner` (the deployer is resolved internally by the constructor).</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@github-actions

This comment has been minimized.

coderabbitai[bot]
coderabbitai bot previously approved these changes Mar 13, 2026
@github-actions

This comment has been minimized.

assert(self.msg_sender().eq(admin), "caller is not admin");
assert(vault_offset > 0, "vault_offset must be greater than 0");

let deployer = get_contract_instance_deployer_avm(self.address).unwrap();
Copy link
Copy Markdown
Member

@wei3erHase wei3erHase Mar 13, 2026

Choose a reason for hiding this comment

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

who is self.maybe_msg_sender() in this context?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Either the deployer or anyone in the case of using universal deployment. And it is being required that the vault and shares contracts have the same deployer. Do you see an issue here?

self.storage.shares.initialize(shares);
self.storage.vault_offset.initialize(vault_offset);

// Caller deposits assets, shares are minted to this contract and are permanently locked.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

beware idk why but ai seems to modify a lot of comments (unrequested)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

self.call(Token::at(asset_token).transfer_public_to_public(from, to, initial_deposit, nonce));

// Mint shares to this contract address (permanently locked)
self.call(Token::at(asset).transfer_public_to_public(from, to, initial_deposit, nonce));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

id prefer using self.address instead of to (confusing)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

symbol: str<31>,
decimals: u8,
minter: AztecAddress,
class_id: ContractClassId,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we shouldn't require to define the class id, no? we can just read it (as we do in constructor), this parameter seems to make it more flexible, while the constructor hardcodes that the token is indeed the imported one

Copy link
Copy Markdown
Collaborator 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 I understand.

You mean to enforce that the shares contract class ID matches the one used for compilation? Is that possible or would we need to define it as a constant?

Anyways, why would we want this?

);

self.storage.asset.initialize(asset);
self.storage.shares.initialize(shares);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's the failure mode? we're correctly binding this contract with an instance of "shares" that should be later initialized, what if the deployer doesn't initialize it? what fails? should we enforce universal deployment so that anyone can initialize it later?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For constructor_with_initial_deposit the shares contract must be deployed first. Otherwise the minting of the initial deposit would fail.

We could enforce constructor to check that the shares contract has been initialized by checking the existence of the initialization nullifier. However, if the shares contract has not been initialized yet, the vault contract cannot be used at all, since its functions need to interact with the shares contract.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think enforcing universal deployment is not needed. Enforcing shares deployment before vault deployment also not. But not sure if I'm missing something

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/vault_contract/src/main.nr (1)

107-151: ⚠️ Potential issue | 🔴 Critical

Add vault-level auth for depositor.

constructor_with_initial_deposit pulls funds from depositor, but unlike the other public from flows it never calls _validate_from_public(...) at the start. Right now the initializer relies only on the downstream token auth check.

🔒 Proposed fix
 fn constructor_with_initial_deposit(
     asset: AztecAddress,
     vault_offset: u128,
     shares_name: str<31>,
     shares_symbol: str<31>,
     shares_decimals: u8,
     shares_class_id: ContractClassId,
     shares_salt: Field,
     initial_deposit: u128,
     depositor: AztecAddress,
     nonce: Field,
 ) {
+    self.internal._validate_from_public(depositor);
     assert(vault_offset > 0, "vault_offset must be greater than 0");
As per coding guidelines, "Every function operating on behalf of another address MUST call `_validate_from_private` or `_validate_from_public` at the start."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/vault_contract/src/main.nr` around lines 107 - 151,
constructor_with_initial_deposit currently pulls tokens from depositor without
performing vault-level auth; add a call to the appropriate validation helper at
the start (e.g., call _validate_from_public(depositor) — or
_validate_from_private if intended private flow) before any token transfer logic
so the function follows the rule that every function operating on behalf of
another address must call _validate_from_public/_validate_from_private; place
this validation near the top of constructor_with_initial_deposit (before
computing shares or calling Token::at(...).transfer_public_to_public) and ensure
the chosen helper matches the rest of the contract's public/private semantics.
src/vault_contract/src/test/utils.nr (1)

119-146: ⚠️ Potential issue | 🟠 Major

This helper still cannot exercise the initial-deposit path once TXE support lands.

constructor_with_initial_deposit mints on the shares token during vault deployment, but this helper deploys the shares contract only afterward. It also never preauthorizes transfer_public_to_public(owner, vault, initial_deposit, 0) for the vault caller. Even with working custom salts/oracles, this setup will still revert. Mirror the TS flow here: precompute the vault address, deploy shares first with that minter, add the asset authwit, then deploy the vault.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/vault_contract/src/test/utils.nr` around lines 119 - 146, The helper
currently deploys the Vault with constructor_with_initial_deposit but only
deploys the shares Token afterward, and it never preauthorizes the asset
transfer; to fix, precompute the vault address (as the TS flow does), deploy the
shares Token first using Token::interface().constructor_with_minter(...) with
that precomputed vault_address as minter, preauthorize the asset transfer via
transfer_public_to_public(owner, vault_address, initial_deposit, 0) (or add the
asset authwit for the vault caller), and only then deploy the Vault using
Vault::interface().constructor_with_initial_deposit(...) so the mint to the
shares token during vault deployment will succeed; refer to
constructor_with_initial_deposit, constructor_with_minter,
transfer_public_to_public and setup_with_vault to locate the relevant code.
src/ts/test/utils.ts (1)

290-330: 🛠️ Refactor suggestion | 🟠 Major

Narrow deployVaultWithInitialDeposit parameter from Wallet to EmbeddedWallet.

The function casts wallet to EmbeddedWallet at line 330 to call setPublicAuthWit when initialDeposit > 0. All current callers in vault.test.ts already pass EmbeddedWallet, so tightening the signature from Wallet to EmbeddedWallet removes the dishonest cast and clarifies the actual type requirement.

 export async function deployVaultWithInitialDeposit(
-  wallet: Wallet,
+  wallet: EmbeddedWallet,
   deployer: AztecAddress,
   assetContract: TokenContract,
   initialDeposit: bigint,
   depositor: AztecAddress,
   options?: DeployOptions,
 ): Promise<[VaultContract, TokenContract]> {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ts/test/utils.ts` around lines 290 - 330, Change the
deployVaultWithInitialDeposit function signature to accept EmbeddedWallet
instead of Wallet (update the parameter type for "wallet"), remove the unsafe
cast to EmbeddedWallet where setPublicAuthWit is called, and ensure all
references inside deployVaultWithInitialDeposit (e.g.,
TokenContract.deployWithOpts call that passes { method:
'constructor_with_minter', wallet } and the setPublicAuthWit(vaultAddress,
transfer, depositor, wallet as EmbeddedWallet) call) now use the correctly typed
EmbeddedWallet parameter; keep the rest of the logic and parameter names
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ts/test/utils.ts`:
- Around line 247-280: Add a fast-fail invariant after deterministic deploys:
after computing vaultAddress with deriveContractAddressWithConstructor and
deploying sharesContract and vaultContract via TokenContract.deployWithOpts and
VaultContract.deployWithOpts, immediately assert that
vaultContract.options.address (or vaultContract.address) equals the precomputed
vaultAddress and that await vaultContract.call('shares') (or the vault's
shares() view) equals sharesContract.address; if either check fails, throw a
clear error immediately so setup fails at source instead of producing later
app_logic_reverted errors.

In `@src/vault_contract/src/main.nr`:
- Around line 1-8: The test module was entirely commented out, removing Noir
unit coverage that checks vault-specific auth and exact revert paths; re-enable
the test module (restore `pub mod test;`) but modify the tests so TXE-dependent
cases are skipped/ignored while keeping pure unit tests enabled—specifically
keep tests exercising `_compute_token_address`, the vault
constructor/initializer invariants, and any auth/revert-path checks as pure Noir
tests; for TXE-only flows, gate those cases behind a compile-time or runtime
skip annotation so they don't run in the TXE environment but the rest of the
`test` module still compiles and runs.

---

Outside diff comments:
In `@src/ts/test/utils.ts`:
- Around line 290-330: Change the deployVaultWithInitialDeposit function
signature to accept EmbeddedWallet instead of Wallet (update the parameter type
for "wallet"), remove the unsafe cast to EmbeddedWallet where setPublicAuthWit
is called, and ensure all references inside deployVaultWithInitialDeposit (e.g.,
TokenContract.deployWithOpts call that passes { method:
'constructor_with_minter', wallet } and the setPublicAuthWit(vaultAddress,
transfer, depositor, wallet as EmbeddedWallet) call) now use the correctly typed
EmbeddedWallet parameter; keep the rest of the logic and parameter names
unchanged.

In `@src/vault_contract/src/main.nr`:
- Around line 107-151: constructor_with_initial_deposit currently pulls tokens
from depositor without performing vault-level auth; add a call to the
appropriate validation helper at the start (e.g., call
_validate_from_public(depositor) — or _validate_from_private if intended private
flow) before any token transfer logic so the function follows the rule that
every function operating on behalf of another address must call
_validate_from_public/_validate_from_private; place this validation near the top
of constructor_with_initial_deposit (before computing shares or calling
Token::at(...).transfer_public_to_public) and ensure the chosen helper matches
the rest of the contract's public/private semantics.

In `@src/vault_contract/src/test/utils.nr`:
- Around line 119-146: The helper currently deploys the Vault with
constructor_with_initial_deposit but only deploys the shares Token afterward,
and it never preauthorizes the asset transfer; to fix, precompute the vault
address (as the TS flow does), deploy the shares Token first using
Token::interface().constructor_with_minter(...) with that precomputed
vault_address as minter, preauthorize the asset transfer via
transfer_public_to_public(owner, vault_address, initial_deposit, 0) (or add the
asset authwit for the vault caller), and only then deploy the Vault using
Vault::interface().constructor_with_initial_deposit(...) so the mint to the
shares token during vault deployment will succeed; refer to
constructor_with_initial_deposit, constructor_with_minter,
transfer_public_to_public and setup_with_vault to locate the relevant code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9be75638-a803-4e5d-8be1-b64af9f5494f

📥 Commits

Reviewing files that changed from the base of the PR and between ae8b8ba and abd997c.

📒 Files selected for processing (3)
  • src/ts/test/utils.ts
  • src/vault_contract/src/main.nr
  • src/vault_contract/src/test/utils.nr

@github-actions

This comment has been minimized.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/vault_contract/src/test/utils.nr">

<violation number="1" location="src/vault_contract/src/test/utils.nr:74">
P1: Calling the vault initializer via `env.call_public` breaks these TXE setup helpers because the initializer uses `get_contract_instance_deployer_avm`, which is unavailable in TXE public context.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/vault_contract/src/main.nr`:
- Around line 1455-1502: _compute_token_address duplicates the Token constructor
ABI packing and selector logic, risking mismatched addresses if
Token::constructor_with_minter changes; extract the initializer hashing/packing
into a single shared utility (e.g., compute_token_initializer_hash or
Token::initializer_hash) that encapsulates the constructor selector, str<31>
packing, decimals/minter field layout, calls hash_args and
poseidon2_hash_with_separator with DOM_SEP__INITIALIZER, and returns the
initialization_hash; then replace the hardcoded logic in _compute_token_address
to call that utility and build the ContractInstance (salt, deployer, class_id,
public_keys) using the returned initialization_hash so both vault and token code
share the same authoritative implementation.
- Around line 119-121: The constructor_with_initial_deposit initializer
currently spends depositor's public balance without performing authwit
validation; add a call to _validate_from_public(depositor) (or
_validate_from_private if this should be a private-sender flow) at the start of
constructor_with_initial_deposit before any balance operations to ensure the
vault is authorized to act on behalf of depositor; update any early-return or
unwrap paths accordingly so validation happens unconditionally for that
entrypoint.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 071b294b-ec8b-4213-8f0e-93ed5fc1bf0e

📥 Commits

Reviewing files that changed from the base of the PR and between abd997c and a913d5b.

📒 Files selected for processing (3)
  • src/vault_contract/src/main.nr
  • src/vault_contract/src/test/constructor_with_initial_deposit.nr
  • src/vault_contract/src/test/utils.nr

@github-actions

This comment has been minimized.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (2)
src/vault_contract/src/main.nr (2)

107-159: ⚠️ Potential issue | 🔴 Critical

Authorize depositor before spending its public balance.

constructor_with_initial_deposit pulls initial_deposit from depositor, but unlike the other public entrypoints it never calls _validate_from_public(depositor). That lets this initializer act on behalf of another address without the vault-layer auth check and then permanently lock the minted shares in self.address.

🔒 Minimal fix
     ) {
         assert(vault_offset > 0, "vault_offset must be greater than 0");
+        self.internal._validate_from_public(depositor);
 
         let deployer = get_contract_instance_deployer_avm(self.address).unwrap();
         let shares = _compute_token_address(

As per coding guidelines, "Every function operating on behalf of another address MUST call _validate_from_private or _validate_from_public at the start. Missing authwit checks are critical security vulnerabilities."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/vault_contract/src/main.nr` around lines 107 - 159, The
constructor_with_initial_deposit function is missing an authorization check for
the depositor; add a call to _validate_from_public(depositor) (or
_validate_from_private if appropriate) before performing any actions on behalf
of depositor (e.g., before the Token::at(asset).transfer_public_to_public call
and before using `from`), so the vault enforces the same vault-layer auth checks
as other entrypoints and prevents acting on another address's behalf.

1-8: ⚠️ Potential issue | 🟠 Major

Don't disable the whole Noir test module.

Skipping all of pub mod test; removes the only layer that can still cover vault-specific auth and exact constructor/address-derivation failures. Keep the TXE/oracle-dependent cases skipped, but leave pure/unit tests for _compute_token_address and initializer invariants enabled. Based on learnings, "TypeScript integration tests (src/ts/test/**/*.ts) can only assert the generic app_logic_reverted error. Specific revert message validation must be done in Noir unit tests (unconstrained fn tests in *.nr files)."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/vault_contract/src/main.nr` around lines 1 - 8, Re-enable the Noir test
module by restoring "pub mod test;" instead of commenting it out, but keep
TXE/oracle-dependent tests skipped or gated; specifically ensure pure unit tests
for _compute_token_address and the vault initializer invariants remain active
(move or mark only the tests that rely on
get_contract_instance/get_contract_instance_deployer_avm or custom deployment
salts as ignored/conditioned). Locate the test module declaration (pub mod test)
and update the contained test cases so TXE-dependent tests are behind a feature
flag or #[ignore], while unconstrained fn tests asserting _compute_token_address
and constructor/initializer revert messages remain enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/vault_contract/src/main.nr`:
- Around line 107-159: The constructor_with_initial_deposit function is missing
an authorization check for the depositor; add a call to
_validate_from_public(depositor) (or _validate_from_private if appropriate)
before performing any actions on behalf of depositor (e.g., before the
Token::at(asset).transfer_public_to_public call and before using `from`), so the
vault enforces the same vault-layer auth checks as other entrypoints and
prevents acting on another address's behalf.
- Around line 1-8: Re-enable the Noir test module by restoring "pub mod test;"
instead of commenting it out, but keep TXE/oracle-dependent tests skipped or
gated; specifically ensure pure unit tests for _compute_token_address and the
vault initializer invariants remain active (move or mark only the tests that
rely on get_contract_instance/get_contract_instance_deployer_avm or custom
deployment salts as ignored/conditioned). Locate the test module declaration
(pub mod test) and update the contained test cases so TXE-dependent tests are
behind a feature flag or #[ignore], while unconstrained fn tests asserting
_compute_token_address and constructor/initializer revert messages remain
enabled.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 54f9042e-781c-4ae5-85ce-e365844e6550

📥 Commits

Reviewing files that changed from the base of the PR and between a913d5b and 79a498d.

📒 Files selected for processing (1)
  • src/vault_contract/src/main.nr

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

cubic-dev-ai[bot]
cubic-dev-ai bot previously approved these changes Mar 20, 2026
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

0 issues found across 2 files (changes from recent commits).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

cubic-dev-ai[bot]
cubic-dev-ai bot previously approved these changes Mar 31, 2026
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

0 issues found across 6 files (changes from recent commits).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 6, 2026

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. Fail message typo🐞 Bug ≡ Correctness
Description
deposit_public_to_public_without_shares expects a revert string missing the trailing single-quote,
so the test will not match the actual revert message used elsewhere and can fail incorrectly.
Code

src/vault_contract/src/test/deposit_public_to_public.nr[6]

+#[test(should_fail_with = "Not initialized 'context.nullifier_exists_unsafe(init_nullifier, context.this_address())")]
Evidence
The deposit failure test’s should_fail_with string is inconsistent with the other failure tests
for the same uninitialized-vault scenario, indicating a typo that will break the assertion matching.

src/vault_contract/src/test/deposit_public_to_public.nr[1-10]
src/vault_contract/src/test/issue_public_to_public.nr[1-8]
src/vault_contract/src/test/redeem_public_to_public.nr[1-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `should_fail_with` string in `deposit_public_to_public_without_shares` is missing the closing `'` that appears in the same revert expectation in other tests. This can cause the test to fail due to string mismatch rather than behavior.
### Issue Context
Other vault failure tests expect:
`Not initialized 'context.nullifier_exists_unsafe(init_nullifier, context.this_address())'`
but the deposit test currently lacks the trailing `'`.
### Fix Focus Areas
- src/vault_contract/src/test/deposit_public_to_public.nr[6-6]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Known failing tests enabled 🐞 Bug ☼ Reliability
Description
The vault initial-deposit Noir tests are still marked #[test] while the file explicitly states TXE
cannot set custom salts, which means the vault-computed shares address can’t match the deployed
shares token and initialization will revert when calling Token::at(shares). This is likely to break
CI in TXE-based test runs until salt support is added or the tests are gated/adjusted.
Code

src/vault_contract/src/test/constructor_with_initial_deposit.nr[R11-16]

+// TODO: TXE does not support custom salts, so the vault's computed shares address won't match
+// the deployed shares token. Tests will fail until custom salt support is added to TXE.
+// See: https://github.com/AztecProtocol/aztec-packages/issues/16656
#[test]
unconstrained fn vault_with_initial_deposit_success() {
Evidence
The test file itself documents that TXE cannot deploy with custom salts and that tests will fail,
yet the tests remain enabled. The vault’s initializer computes a deterministic shares address and
then immediately calls into that shares contract during constructor_with_initial_deposit, so any
address mismatch will revert the initializer.

src/vault_contract/src/test/constructor_with_initial_deposit.nr[11-37]
src/vault_contract/src/main.nr[98-154]
src/vault_contract/src/test/utils.nr[134-193]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Noir TXE tests for `constructor_with_initial_deposit` are enabled while the test file states TXE cannot set custom salts, making shares-address matching impossible and causing initializer reverts.
### Issue Context
The initializer computes `shares` deterministically and then calls `Token::at(shares)` during initialization. If TXE ignores/doesn't support the requested salt, the shares token will be deployed at a different address than the vault computes.
### Fix Focus Areas
- src/vault_contract/src/test/constructor_with_initial_deposit.nr[11-37]
- src/vault_contract/src/test/utils.nr[134-193]
- src/vault_contract/src/main.nr[98-154]
### Suggested changes
- Until TXE supports salts, either:
- Mark these tests as ignored/skipped in TXE (e.g., feature-gate), **or**
- Rework the TXE test setup to avoid relying on custom salts/address matching (e.g., keep the legacy 2-step wiring for TXE-only tests).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Shares address key mismatch 🐞 Bug ≡ Correctness
Description
_compute_token_address hardcodes PublicKeys::default(), so the vault-computed shares address
will only match shares deployments that also use default public keys; deploying the shares token
with non-default keys/secret will make the vault store the wrong shares address and break mint/burn
calls.
Code

src/vault_contract/src/main.nr[R1476-1484]

+        let instance = ContractInstance {
+            salt,
+            deployer,
+            contract_class_id: class_id,
+            initialization_hash,
+            public_keys: PublicKeys::default(),
+        };
+
+        instance.to_address()
Evidence
The computed token address includes public_keys via ContractInstance, but the vault always uses
PublicKeys::default() when deriving the shares address. The test harness explicitly sets
with_secret(0) to force default public keys, showing that non-default keys are a meaningful
variation that would otherwise diverge.

src/vault_contract/src/main.nr[1459-1485]
src/vault_contract/src/test/utils.nr[159-172]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Vault derives the shares token address using `PublicKeys::default()`. If a shares token deployment uses different public keys (e.g., a non-zero deployment secret), the vault’s computed address will not equal the real shares contract address, causing downstream calls (mint/burn/total_supply) to revert or act on the wrong contract.
### Issue Context
The address derivation uses a `ContractInstance` that contains `public_keys`, but `_compute_token_address` hardcodes `PublicKeys::default()`.
### Fix Focus Areas
- src/vault_contract/src/main.nr[1459-1485]
### Expected fix direction
- Add a constructor parameter for the shares contract `public_keys` (or a `shares_secret` that can be deterministically mapped to public keys), and feed it into `_compute_token_address`.
- Update TS and Noir test deployment helpers to pass the same keys/secret used when deploying the shares token.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Deployer oracle unwrap revert 🐞 Bug ☼ Reliability
Description
Vault constructors call get_contract_instance_deployer_avm(self.address).unwrap(), so if the oracle
returns None the initializer hard-reverts with an opaque failure, preventing vault
deployment/initialization. This makes deployments brittle across environments where the deployer
oracle isn’t available or isn’t populated yet.
Code

src/vault_contract/src/main.nr[66]

+        let deployer = get_contract_instance_deployer_avm(self.address).unwrap();
Evidence
Both initializers unconditionally unwrap the deployer returned by the AVM oracle; any None result
will revert. Because this value is required to compute the immutable shares address, the entire
initialization becomes dependent on oracle availability.

src/vault_contract/src/main.nr[55-80]
src/vault_contract/src/main.nr[99-127]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_contract_instance_deployer_avm(self.address).unwrap()` can revert if the oracle returns `None`, bricking initialization with an unclear error.
### Issue Context
The deployer value is used to compute the shares token address; if the oracle is unavailable/unpopulated in a given environment or deployment mode, initialization cannot proceed.
### Fix Focus Areas
- src/vault_contract/src/main.nr[55-80]
- src/vault_contract/src/main.nr[99-127]
### Suggested changes
- Replace `.unwrap()` with an explicit `expect`/`assert` including a clear error message, OR
- Pass `deployer` as an explicit constructor parameter (and use that in `_compute_token_address`) to remove oracle dependency.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Determinism docs omit constraints 🐞 Bug ≡ Correctness
Description
The new deterministic shares-address derivation hardcodes PublicKeys::default(), but the deployment
guidance/comments don’t state that the shares token must be deployed with default public
keys/secret=0, and the vault constructor doc even implies the shares deployment uses the same salt
as the vault. This can lead to mis-deployments that permanently set an incorrect immutable shares
address and render the vault unusable.
Code

src/ts/test/utils.ts[R244-246]

+ * The vault constructor computes the shares token address deterministically from the token
+ * class ID, salt, deployer, and constructor args. The shares token must be deployed with the
+ * same salt and deployer so the computed address matches.
Evidence
On-chain address derivation in _compute_token_address fixes public_keys to default, which
requires the shares deployment to use matching/default public keys. The Noir tests already have to
force with_secret(0) to match this, but the TS helper comment describing determinism does not
mention public keys/secret, and the vault constructor comment incorrectly suggests the shares
deployment uses the same salt as the vault (TS uses separate vaultSalt and sharesSalt).

src/vault_contract/src/main.nr[41-46]
src/vault_contract/src/main.nr[1449-1485]
src/vault_contract/src/test/utils.nr[76-89]
src/ts/test/utils.ts[242-300]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shares address derivation assumes `PublicKeys::default()` but docs/comments for deterministic deployment omit this requirement (and also contain misleading salt wording), risking deployments where the vault stores an incorrect immutable `shares` address.
### Issue Context
- `_compute_token_address` sets `public_keys: PublicKeys::default()`.
- Noir tests explicitly set `with_secret(0)` to force default public keys.
- TS helper comment describes determinism but omits the public-keys/secret constraint.
### Fix Focus Areas
- src/vault_contract/src/main.nr[41-46]
- src/vault_contract/src/main.nr[1449-1485]
- src/ts/test/utils.ts[242-300]
- src/vault_contract/src/test/utils.nr[76-89]
### Suggested changes
- Update constructor/TSDoc comments to explicitly require deploying the shares token with default public keys (e.g., secret=0 / default keys) and clarify salts (shares uses `shares_salt`, vault uses its own salt).
- Optionally: extend the vault constructor to accept `PublicKeys` for the shares deployment and incorporate them into `_compute_token_address` so non-default deployments are supported safely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Grey Divider

Previous review results

Review updated until commit a68d1ea

Results up to commit N/A


🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider
Action required
1. Fail message typo🐞 Bug ≡ Correctness
Description
deposit_public_to_public_without_shares expects a revert string missing the trailing single-quote,
so the test will not match the actual revert message used elsewhere and can fail incorrectly.
Code

src/vault_contract/src/test/deposit_public_to_public.nr[6]

+#[test(should_fail_with = "Not initialized 'context.nullifier_exists_unsafe(init_nullifier, context.this_address())")]
Evidence
The deposit failure test’s should_fail_with string is inconsistent with the other failure tests
for the same uninitialized-vault scenario, indicating a typo that will break the assertion matching.

src/vault_contract/src/test/deposit_public_to_public.nr[1-10]
src/vault_contract/src/test/issue_public_to_public.nr[1-8]
src/vault_contract/src/test/redeem_public_to_public.nr[1-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `should_fail_with` string in `deposit_public_to_public_without_shares` is missing the closing `'` that appears in the same revert expectation in other tests. This can cause the test to fail due to string mismatch rather than behavior.
### Issue Context
Other vault failure tests expect:
`Not initialized 'context.nullifier_exists_unsafe(init_nullifier, context.this_address())'`
but the deposit test currently lacks the trailing `'`.
### Fix Focus Areas
- src/vault_contract/src/test/deposit_public_to_public.nr[6-6]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Known failing tests enabled 🐞 Bug ☼ Reliability
Description
The vault initial-deposit Noir tests are still marked #[test] while the file explicitly states TXE
cannot set custom salts, which means the vault-computed shares address can’t match the deployed
shares token and initialization will revert when calling Token::at(shares). This is likely to break
CI in TXE-based test runs until salt support is added or the tests are gated/adjusted.
Code

src/vault_contract/src/test/constructor_with_initial_deposit.nr[R11-16]

+// TODO: TXE does not support custom salts, so the vault's computed shares address won't match
+// the deployed shares token. Tests will fail until custom salt support is added to TXE.
+// See: https://github.com/AztecProtocol/aztec-packages/issues/16656
#[test]
unconstrained fn vault_with_initial_deposit_success() {
Evidence
The test file itself documents that TXE cannot deploy with custom salts and that tests will fail,
yet the tests remain enabled. The vault’s initializer computes a deterministic shares address and
then immediately calls into that shares contract during constructor_with_initial_deposit, so any
address mismatch will revert the initializer.

src/vault_contract/src/test/constructor_with_initial_deposit.nr[11-37]
src/vault_contract/src/main.nr[98-154]
src/vault_contract/src/test/utils.nr[134-193]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Noir TXE tests for `constructor_with_initial_deposit` are enabled while the test file states TXE cannot set custom salts, making shares-address matching impossible and causing initializer reverts.
### Issue Context
The initializer computes `shares` deterministically and then calls `Token::at(shares)` during initialization. If TXE ignores/doesn't support the requested salt, the shares token will be deployed at a different address than the vault computes.
### Fix Focus Areas
- src/vault_contract/src/test/constructor_with_initial_deposit.nr[11-37]
- src/vault_contract/src/test/utils.nr[134-193]
- src/vault_contract/src/main.nr[98-154]
### Suggested changes
- Until TXE supports salts, either:
- Mark these tests as ignored/skipped in TXE (e.g., feature-gate), **or**
- Rework the TXE test setup to avoid relying on custom salts/address matching (e.g., keep the legacy 2-step wiring for TXE-only tests).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
3. Shares address key mismatch 🐞 Bug ≡ Correctness
Description
_compute_token_address hardcodes PublicKeys::default(), so the vault-computed shares address
will only match shares deployments that also use default public keys; deploying the shares token
with non-default keys/secret will make the vault store the wrong shares address and break mint/burn
calls.
Code

src/vault_contract/src/main.nr[R1476-1484]

+        let instance = ContractInstance {
+            salt,
+            deployer,
+            contract_class_id: class_id,
+            initialization_hash,
+            public_keys: PublicKeys::default(),
+        };
+
+        instance.to_address()
Evidence
The computed token address includes public_keys via ContractInstance, but the vault always uses
PublicKeys::default() when deriving the shares address. The test harness explicitly sets
with_secret(0) to force default public keys, showing that non-default keys are a meaningful
variation that would otherwise diverge.

src/vault_contract/src/main.nr[1459-1485]
src/vault_contract/src/test/utils.nr[159-172]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Vault derives the shares token address using `PublicKeys::default()`. If a shares token deployment uses different public keys (e.g., a non-zero deployment secret), the vault’s computed address will not equal the real shares contract address, causing downstream calls (mint/burn/total_supply) to revert or act on the wrong contract.
### Issue Context
The address derivation uses a `ContractInstance` that contains `public_keys`, but `_compute_token_address` hardcodes `PublicKeys::default()`.
### Fix Focus Areas
- src/vault_contract/src/main.nr[1459-1485]
### Expected fix direction
- Add a constructor parameter for the shares contract `public_keys` (or a `shares_secret` that can be deterministically mapped to public keys), and feed it into `_compute_token_address`.
- Update TS and Noir test deployment helpers to pass the same keys/secret used when deploying the shares token.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Deployer oracle unwrap revert 🐞 Bug ☼ Reliability
Description
Vault constructors call get_contract_instance_deployer_avm(self.address).unwrap(), so if the oracle
returns None the initializer hard-reverts with an opaque failure, preventing vault
deployment/initialization. This makes deployments brittle across environments where the deployer
oracle isn’t available or isn’t populated yet.
Code

src/vault_contract/src/main.nr[66]

+        let deployer = get_contract_instance_deployer_avm(self.address).unwrap();
Evidence
Both initializers unconditionally unwrap the deployer returned by the AVM oracle; any None result
will revert. Because this value is required to compute the immutable shares address, the entire
initialization becomes dependent on oracle availability.

src/vault_contract/src/main.nr[55-80]
src/vault_contract/src/main.nr[99-127]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_contract_instance_deployer_avm(self.address).unwrap()` can revert if the oracle returns `None`, bricking initialization with an unclear error.
### Issue Context
The deployer value is used to compute the shares token address; if the oracle is unavailable/unpopulated in a given environment or deployment mode, initialization cannot proceed.
### Fix Focus Areas
- src/vault_contract/src/main.nr[55-80]
- src/vault_contract/src/main.nr[99-127]
### Suggested changes
- Replace `.unwrap()` with an explicit `expect`/`assert` including a clear error message, OR
- Pass `deployer` as an explicit constructor parameter (and use that in `_compute_token_address`) to remove oracle dependency.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Determinism docs omit constraints 🐞 Bug ≡ Correctness
Description
The new deterministic shares-address derivation hardcodes PublicKeys::default(), but the deployment
guidance/comments don’t state that the shares token must be deployed with default public
keys/secret=0, and the vault constructor doc even implies the shares deployment uses the same salt
as the vault. This can lead to mis-deployments that permanently set an incorrect immutable shares
address and render the vault unusable.
Code

src/ts/test/utils.ts[R244-246]

+ * The vault constructor computes the shares token address deterministically from the token
+ * class ID, salt, deployer, and constructor args. The shares token must be deployed with the
+ * same salt and deployer so the computed address matches.
Evidence
On-chain address derivation in _compute_token_address fixes public_keys to default, which
requires the shares deployment to use matching/default public keys. The Noir tests already have to
force with_secret(0) to match this, but the TS helper comment describing determinism does not
mention public keys/secret, and the vault constructor comment incorrectly suggests the shares
deployment uses the same salt as the vault (TS uses separate vaultSalt and sharesSalt).

src/vault_contract/src/main.nr[41-46]
src/vault_contract/src/main.nr[1449-1485]
src/vault_contract/src/test/utils.nr[76-89]
src/ts/test/utils.ts[242-300]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shares address derivation assumes `PublicKeys::default()` but docs/comments for deterministic deployment omit this requirement (and also contain misleading salt wording), risking deployments where the vault stores an incorrect immutable `shares` address.
### Issue Context
- `_compute_token_address` sets `public_keys: PublicKeys::default()`.
- Noir tests explicitly set `with_secret(0)` to force default public keys.
- TS helper comment describes determinism but omits the public-keys/secret constraint.
### Fix Focus Areas
- src/vault_contract/src/main.nr[41-46]
- src/vault_contract/src/main.nr[1449-1485]
- src/ts/test/utils.ts[242-300]
- src/vault_contract/src/test/utils.nr[76-89]
### Suggested changes
- Update constructor/TSDoc comments to explicitly require deploying the shares token with default public keys (e.g., secret=0 / default keys) and clarify salts (shares uses `shares_salt`, vault uses its own salt).
- Optionally: extend the vault constructor to accept `PublicKeys` for the shares deployment and incorporate them into `_compute_token_address` so non-default deployments are supported safely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider Grey Divider

Qodo Logo

Comment on lines 11 to 16
// TODO: TXE does not support custom salts, so the vault's computed shares address won't match
// the deployed shares token. Tests will fail until custom salt support is added to TXE.
// See: https://github.com/AztecProtocol/aztec-packages/issues/16656

#[test]
unconstrained fn vault_with_initial_deposit_success() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Known failing tests enabled 🐞 Bug ☼ Reliability

The vault initial-deposit Noir tests are still marked #[test] while the file explicitly states TXE
cannot set custom salts, which means the vault-computed shares address can’t match the deployed
shares token and initialization will revert when calling Token::at(shares). This is likely to break
CI in TXE-based test runs until salt support is added or the tests are gated/adjusted.
Agent Prompt
### Issue description
Noir TXE tests for `constructor_with_initial_deposit` are enabled while the test file states TXE cannot set custom salts, making shares-address matching impossible and causing initializer reverts.

### Issue Context
The initializer computes `shares` deterministically and then calls `Token::at(shares)` during initialization. If TXE ignores/doesn't support the requested salt, the shares token will be deployed at a different address than the vault computes.

### Fix Focus Areas
- src/vault_contract/src/test/constructor_with_initial_deposit.nr[11-37]
- src/vault_contract/src/test/utils.nr[134-193]
- src/vault_contract/src/main.nr[98-154]

### Suggested changes
- Until TXE supports salts, either:
  - Mark these tests as ignored/skipped in TXE (e.g., feature-gate), **or**
  - Rework the TXE test setup to avoid relying on custom salts/address matching (e.g., keep the legacy 2-step wiring for TXE-only tests).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai bot commented Apr 6, 2026

You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment @cubic-dev-ai review.

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 6, 2026

Persistent review updated to latest commit a1c2e20

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 6, 2026

Persistent review updated to latest commit a68d1ea

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2026

Benchmark Comparison

CPU Cores RAM Arch
AMD EPYC 7763 64-Core Processor 16 63 GiB x64

Contract: escrow

🚦 Function Gates DA Gas L2 Gas Proving Time (ms)
Base PR Diff Base PR Diff Base PR Diff Base PR Diff
(partial) withdraw 579,408 579,408 1,312 1,312 495,400 495,400 7,169 7,225 +56 (+0.8%)
withdraw 579,408 579,408 736 736 483,700 483,700 7,140 7,207 +67 (+0.9%)
withdraw_nft 536,373 536,373 736 736 483,700 483,700 6,933 6,955 +22 (+0.3%)

Contract: logic

🚦 Function Gates DA Gas L2 Gas Proving Time (ms)
Base PR Diff Base PR Diff Base PR Diff Base PR Diff
get_escrow 520,851 520,851 128 128 456,000 456,000 6,854 6,873 +19 (+0.3%)
secret_key_to_public_keys 515,935 515,935 128 128 456,000 456,000 6,819 6,877 +58 (+0.9%)
share_escrow 420,067 420,067 704 704 474,500 474,500 6,006 6,003 -3 (-0.0%)

Contract: nft

🚦 Function Gates DA Gas L2 Gas Proving Time (ms)
Base PR Diff Base PR Diff Base PR Diff Base PR Diff
burn_private 455,161 455,161 416 416 671,675 671,675 6,360 6,359 -1 (-0.0%)
burn_public 340,915 340,915 448 448 691,760 691,760 5,360 5,358 -2 (-0.0%)
mint_to_private 472,217 472,217 960 960 662,581 662,581 6,428 6,421 -7 (-0.1%)
mint_to_public 340,915 340,915 448 448 694,916 694,916 5,368 5,363 -5 (-0.1%)
transfer_private_to_private 427,956 427,956 736 736 483,700 483,700 6,112 6,074 -38 (-0.6%)
transfer_private_to_public 455,237 455,237 416 416 666,473 666,473 6,323 6,316 -7 (-0.1%)
transfer_public_to_private 468,858 468,858 960 960 662,665 662,665 6,381 6,400 +19 (+0.3%)
transfer_public_to_public 340,915 340,915 384 384 648,357 648,357 5,379 5,354 -25 (-0.5%)

Contract: token

🚦 Function Gates DA Gas L2 Gas Proving Time (ms)
Base PR Diff Base PR Diff Base PR Diff Base PR Diff
burn_private 486,861 486,861 992 992 687,411 687,411 6,489 6,478 -11 (-0.2%)
burn_public 340,915 340,915 448 448 681,599 681,599 5,340 5,339 -1 (-0.0%)
initialize_transfer_commitment 403,443 403,443 704 704 474,500 474,500 5,898 5,898
mint_to_private 473,137 473,137 960 960 656,449 656,449 6,466 6,421 -45 (-0.7%)
mint_to_public 340,915 340,915 448 448 690,521 690,521 5,364 5,413 +49 (+0.9%)
transfer_private_to_commitment 453,824 453,824 896 896 495,400 495,400 6,229 6,234 +5 (+0.1%)
transfer_private_to_private 470,985 470,985 1,312 1,312 495,400 495,400 6,274 6,350 +76 (+1.2%)
transfer_private_to_public 486,937 486,937 992 992 690,330 690,330 6,532 6,505 -27 (-0.4%)
transfer_private_to_public_with_commitment 497,197 497,197 1,568 1,568 723,630 723,630 6,568 6,591 +23 (+0.4%)
transfer_public_to_commitment 340,915 340,915 576 576 683,157 683,157 5,399 5,396 -3 (-0.1%)
transfer_public_to_private 469,793 469,793 960 960 659,611 659,611 6,394 6,397 +3 (+0.0%)
transfer_public_to_public 340,915 340,915 448 448 684,065 684,065 5,340 5,425 +85 (+1.6%)

Contract: vault

🚦 Function Gates DA Gas L2 Gas Proving Time (ms)
Base PR Diff Base PR Diff Base PR Diff Base PR Diff
deposit_private_to_private 904,856 904,856 1,280 1,280 894,014 896,036 +2,022 (+0.2%) 10,084 9,987 -97 (-1.0%)
deposit_private_to_private_exact 1,017,026 1,017,026 1,856 1,856 936,548 938,570 +2,022 (+0.2%) 10,943 10,982 +39 (+0.4%)
deposit_private_to_public 718,982 718,982 768 768 928,555 930,577 +2,022 (+0.2%) 8,480 8,450 -30 (-0.4%)
deposit_public_to_private 589,551 589,551 1,344 1,344 959,628 961,650 +2,022 (+0.2%) 7,365 7,385 +20 (+0.3%)
deposit_public_to_private_exact 748,093 748,093 1,920 1,920 1,002,324 1,004,346 +2,022 (+0.2%) 8,742 8,754 +12 (+0.1%)
deposit_public_to_public 340,915 340,915 832 832 1,000,133 1,002,236 +2,103 (+0.2%) 5,346 5,370 +24 (+0.4%)
issue_private_to_private_exact 1,017,026 1,017,026 1,856 1,856 936,824 938,846 +2,022 (+0.2%) 10,911 10,915 +4 (+0.0%)
issue_private_to_public_exact 831,162 831,162 1,344 1,344 971,590 973,612 +2,022 (+0.2%) 9,391 9,357 -34 (-0.4%)
issue_public_to_private 639,893 639,893 1,344 1,344 968,462 970,484 +2,022 (+0.2%) 7,859 7,809 -50 (-0.6%)
issue_public_to_public 340,915 340,915 832 832 1,000,436 1,002,539 +2,103 (+0.2%) 5,348 5,349 +1 (+0.0%)
redeem_private_to_private_exact 1,013,679 1,013,679 1,856 1,856 937,055 939,077 +2,022 (+0.2%) 10,890 10,937 +47 (+0.4%)
redeem_private_to_public 718,905 718,905 768 768 919,369 921,391 +2,022 (+0.2%) 8,410 8,503 +93 (+1.1%)
redeem_public_to_private_exact 698,526 698,526 1,920 1,920 999,453 1,001,475 +2,022 (+0.2%) 8,292 8,294 +2 (+0.0%)
redeem_public_to_public 340,915 340,915 832 832 987,833 989,936 +2,103 (+0.2%) 5,346 5,406 +60 (+1.1%)
withdraw_private_to_private 901,509 901,509 1,280 1,280 893,840 895,862 +2,022 (+0.2%) 9,985 9,964 -21 (-0.2%)
withdraw_private_to_private_exact 1,013,679 1,013,679 1,856 1,856 936,617 938,639 +2,022 (+0.2%) 10,897 10,853 -44 (-0.4%)
withdraw_private_to_public_exact 831,084 831,084 1,344 1,344 962,194 964,216 +2,022 (+0.2%) 9,369 9,413 +44 (+0.5%)
withdraw_public_to_private 586,272 586,272 1,344 1,344 955,866 957,888 +2,022 (+0.2%) 7,338 7,311 -27 (-0.4%)
withdraw_public_to_public 340,915 340,915 832 832 987,752 989,855 +2,103 (+0.2%) 5,362 5,353 -9 (-0.2%)

@ilpepepig
Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai bot commented Apr 10, 2026

@cubic-dev-ai review

@ilpepepig I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 8 files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants