Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat: add method to generate the new Brillig opcode from UnresolvedBrilligCall #363

Merged
merged 4 commits into from
Jun 12, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Jun 12, 2023

Description

Problem*

Resolves

Summary*

This PR aims to smooth out the devex of resolving a brillig foreign call by providing a helper function to generate an updated Brillig using the returned call result.

This allows us to use the snippet below

let result: ForeignCallResult = Oracle::do_brillig_foreign_call(unresolved_brillig_call.foreign_call_wait_info);
let new_brillig: Brillig = unresolved_brillig_call.resolve_foreign_call(result);
opcodes.push(Opcode::Brillig(new_brillig))

as opposed to having to extract the Brillig struct from the foreign call and manually push a new ForeignCallResult onto one of its fields.

Additional Context

PR Checklist*

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

BEGIN_COMMIT_OVERRIDE
feat: Add method to generate updated Brillig opcode from UnresolvedBrilligCall (#363)
END_COMMIT_OVERRIDE

@kevaundray kevaundray requested review from vezenovm and ludamad June 12, 2023 09:17
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

Can you rewrite the Brillig oracle tests to use this new interface?

@TomAFrench
Copy link
Member Author

Can you rewrite the Brillig oracle tests to use this new interface?

Do we have tests for how to resolve Brillig calls from outside the ACVM? I can only see the tests in brillig_vm/src/lib.rs which work on the Brillig VM directly (and so never create an UnresolvedBrilligCall.

@vezenovm
Copy link
Contributor

Do we have tests for how to resolve Brillig calls from outside the ACVM?

They are the main pwg tests:

fn inversion_brillig_oracle_equivalence() {

@TomAFrench
Copy link
Member Author

TomAFrench commented Jun 12, 2023

Done, although I think we should consider renaming the foreign_call_wait_info field of UnresolvedBrilligCall at some point. In a world where we're not immediately destructuring the UnresolvedBrilligCall then it's overly verbose, as shown by foreign_call.foreign_call_wait_info.

@TomAFrench
Copy link
Member Author

TomAFrench commented Jun 12, 2023

Although, come to think of it. By that logic maybe we should shorten resolve_foreign_call to resolve.

@vezenovm
Copy link
Contributor

although I think we should consider renaming the foreign_call_wait_info field of UnresolvedBrilligCall at some point

Yeah I agree. I think we can bikeshed this name in general. Perhaps just wait_info with the name type ForeignCallWaitInfo. Or we can bilkeshed when we fully get rid of the Oracle opcode

@TomAFrench TomAFrench added this pull request to the merge queue Jun 12, 2023
@TomAFrench
Copy link
Member Author

Or we can bilkeshed when we fully get rid of the Oracle opcode

Yeah, I think we can make this decision later as we'll "own" most of the broken code but doing it at the same time as a breaking change of oracle code makes sense.

Merged via the queue into master with commit fda5dbe Jun 12, 2023
@kevaundray kevaundray mentioned this pull request Jun 12, 2023
@TomAFrench TomAFrench mentioned this pull request Jun 13, 2023
2 tasks
@TomAFrench TomAFrench deleted the brillig-resolution branch June 15, 2023 04:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants