Skip to content

Conversation

@anaPerezGhiglia
Copy link
Member

@anaPerezGhiglia anaPerezGhiglia commented Feb 26, 2025

Description

Expand debugger funcionality to support debugging test functions and configure an external oracle-resolver url.
This will allow to debug contract tests using the TXE as oracle resolver

Problem*

Resolves noir-lang#5208

Summary*

  • Add the possibility to compile and debug a test function
  • Add a new config parameter to configure an external oracle resolver when debugging
  • REPL: execute the debugger in a separate thread than the repl cli
  • Add new Debug test codelens request. Related vscode-noir PR (feat(codelens): add debug test codelens vscode-noir#3))

Additional Context

Debugger changes

  • add test-name and oracle-resolver arg options to debugger (both repl and dap interfaces)
  • modify DebugForeignCallExecutor to support setting a resolver_url
  • modify the debugger to know how to compile a specific function that's not the main function
  • create a new DebugExecutionResult enum that represents the possible outcomes of debugging a program
  • modify the debugger to expose the execution result once the debug session ends
  • add the responsibility of analyzing the execution result once the debugger finishes with the execution to analyze whether the execution matches the test expectations (some test may be marked to should_fail)
  • convert ReplDebugger into AsyncReplDebugger
    • this involved spawining a new thread and communicating with the debugger through a mpsc channel
      • this implied moving some responsibility from the _cmd to the debugger mod, and from the repl to the context- like creating the Bn254BlackBoxSolver or restarting the debugging context
    • the async debugger entry points are defined by the DebugCommandAPI enum

Modifications in nargo module

  • extract logic of mapping a OpcodeResolutionError to an ExecutionError from nargo::ops::execute into a new function nargo::errors::execution_error_from. Use this new function in debugger::context and nargo::ops::execute
  • create a new nargo::ops::debug module. Extract reused debug functions into it.
    The moved functions are mostly about knowing how to compile or prepare the package for compiling for debugging. This also avoids generating inter-dependencies between debug_cmd and dap_cmd
  • extract check_crate_and_report_errors from check_cmd to nargo::ops since it was being used in different commands
  • make some test functions and structs public to be able to use them from debugger cli commands
    • make some nargo::ops::test functions public
    • create TestResult::new function to be able to construct one from dap
  • modify RPCForeignCallExecutor to apply workaround for RPCForeignCallExecutor fails on long delays between requests noir-lang/noir#7463

Modifications in noirc_frontend::debug module

  • Modify the DebugInstrumenter to instrument contract functions in addition to top-level functions

Observations

Things to improve down the road

  • both the repl and the dap are keeping track of the last_result. Perhaps this is something that could be moved to the context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@github-actions
Copy link

github-actions bot commented Feb 26, 2025

Thank you for your contribution to the Noir language.

Please do not force push to this branch after the Noir team have started review of this PR. Doing so will only delay us merging your PR as we will need to start the review process from scratch.

Thanks for your understanding.

the repl communicates with the debugger through mpsc channels
 - clean brillig_solver, witness_stack and acvm_stack fields as well
inspired in Gus' old comments
re-accomodate debug_cmd to make use of it
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 12, 2025
Copy link

@mverzilli mverzilli left a comment

Choose a reason for hiding this comment

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

Left some suggestions to enhance the docs


_String, optional._

JSON RPC url to solve oracle calls

Choose a reason for hiding this comment

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

Suggested change
JSON RPC url to solve oracle calls
JSON RPC URL to solve oracle calls.

Choose a reason for hiding this comment

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

Let's add a little more explanation, it is true that this sets the target for oracle calls, but from the perspective of users/newcomers it is kind of obscure. I'd expand telling that this lets you interact with a TXE, which enables debugging tests that use aztec-contracts

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmh, I'm not sure about that. I thought about it when writing this, but then I changed my mind because these docs are about noir-lang, not about aztec framework.
I definitively think that there should be a "How to debug aztec contracts tests" guide, but I think that that guide should live in aztec's docs, not in noir's.

Copy link

@mverzilli mverzilli left a comment

Choose a reason for hiding this comment

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

Looking great!

let num_steps = 16;
for _ in 1..=num_steps {
let expected_lines_by_command: Vec<VecDeque<&str>> = vec![
VecDeque::from(["fn main(x: Field, y: pub Field) {"]),

Choose a reason for hiding this comment

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

Nit (only if it doesn't generate a borrow checker headache, which I can't tell without trying): maybe refactor to get the Vec<VecDeque<&str>> from a string array of arrays, to reduce the "visual noise" in this.

It could be a good idea to add a trim somewhere so the test isn't sensitive to white space, which should also help make it easier to maintain this test and/or add new ones

Choose a reason for hiding this comment

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

This maybe a good use case for a macro?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to VecDeque becuase it has the pop_front method that consumes the first element from the vector
I got to it by reading the Vec::pop doc that says

/// If you'd like to pop the first element, consider using
/// [VecDeque::pop_front] instead.

Copy link
Member Author

@anaPerezGhiglia anaPerezGhiglia Mar 21, 2025

Choose a reason for hiding this comment

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

On the other hand, I removed the useless whitespace on the expected lines and replaced the trim_start() with a trim() on the other branch (and cherry-picked it here for later)

@mverzilli mverzilli added the enhancement New feature or request label Mar 21, 2025
@anaPerezGhiglia anaPerezGhiglia merged commit 104be72 into feat/5208-debug-tests-2025 Mar 25, 2025
92 of 104 checks passed
@anaPerezGhiglia anaPerezGhiglia deleted the feat/debug-tests-2025 branch March 25, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Debugger should work with tests

3 participants