Skip to content

Migrate to 0.3.0#647

Merged
kevaundray merged 25 commits intomasterfrom
kw/acvm3.0
Jan 18, 2023
Merged

Migrate to 0.3.0#647
kevaundray merged 25 commits intomasterfrom
kw/acvm3.0

Conversation

@kevaundray
Copy link
Copy Markdown
Contributor

Related issue(s)

Resolves #646

Description

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

@kevaundray
Copy link
Copy Markdown
Contributor Author

This PR is blocked by the following:

  • We need to publish a new version of acvm to crates.io (changing the acvm references in the relevant Cargo.toml's once done)
  • The aztec_backend migration needs to be merged (changing nargo/Cargo.toml once done)

@kevaundray
Copy link
Copy Markdown
Contributor Author

acvm has marked a few methods as deprecated -- I will create an issue to fix these in this repo, as using the recommended functions will change the behaviour post-migration.

@kevaundray kevaundray force-pushed the kw/acvm3.0 branch 2 times, most recently from 556ba96 to 38ba77e Compare January 15, 2023 22:58
Copy link
Copy Markdown
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

I have got some concerns to migrate to ACVM 0.3.0, which was not formally reviewed.
This PR in itself is fine for me and the main impacts from 0.3.0 would be naming issues which is not a big deal.
However, merging this PR would effectively activate 0.3.0 for which I have some concerns.
I think the proper way to handle this would be to address any issue we may have with 0.3.0 into a new 0.3.1 version and then migrate to the approved version.

Copy link
Copy Markdown
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

I'm fine with the local changes introduced by this PR as well. I'd be okay with merging once the existing comments are addressed and the concerns about acvm 0.3.0 alleviated.

@kevaundray
Copy link
Copy Markdown
Contributor Author

I have got some concerns to migrate to ACVM 0.3.0, which was not formally reviewed.
This PR in itself is fine for me and the main impacts from 0.3.0 would be naming issues which is not a big deal.
However, merging this PR would effectively activate 0.3.0 for which I have some concerns.
I think the proper way to handle this would be to address any issue we may have with 0.3.0 into a new 0.3.1 version and then migrate to the approved version.

Some of the concerns brought up may take a while, cntroversial and do not have anything to do with correctness, and are for the most part opaque to Noir.

I will open issues on the acvm repo regarding these -- we can merge this and if need to, push a 0.4 version, once those issues have been resolved

@phated
Copy link
Copy Markdown
Contributor

phated commented Jan 18, 2023

@guipublic please remember that this is blocking anyone using a non-default Brew prefix to be unable build from source. A fast resolution is appreciated.

@kevaundray
Copy link
Copy Markdown
Contributor Author

I've fixed all of the TODOs and we are now using the published version of acvm.

Once the aztec_backend PR has been merged, I'll update the dependency in nargo from my branch to a commit on master

@guipublic
Copy link
Copy Markdown
Contributor

please remember that this is blocking anyone using a non-default Brew prefix to be unable build from source. A fast resolution is appreciated.

I am sorry but I don't understand this sentence! I don't know what a non-default Brew prefix means, neither why we have an urgent build issue nor what caused it. I would recommend though to use a default Brew prefix :)

@kevaundray I am reviewing the acvm and will create an issue with all my concerns so we can see what can be resolved quickly (I am fine with doing the changes myself), what can be delayed and what should be rejected (from my concerns I mean)

@kevaundray
Copy link
Copy Markdown
Contributor Author

I am sorry but I don't understand this sentence! I don't know what a non-default Brew prefix means, neither why we have an urgent build issue nor what caused it. I would recommend though to use a default Brew prefix :)

I created an issue with all of the concerns we went through -- We can push a 0.4 or 0.3.2 to resolve those issues. Once the aztec_backend PR has been merged, this will get merged.

@kevaundray kevaundray merged commit b1e9242 into master Jan 18, 2023
@kevaundray kevaundray deleted the kw/acvm3.0 branch January 18, 2023 23:33
This was referenced Jan 18, 2023
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.

Migrate to acvm 0.3.0

4 participants