chore: remove constant generation call for languages other than TS from yarn-project/constants - #24730
Conversation
replace them with a new remake-constants.sh that invokes the new cli
yarn-project/constants. move to barretenberg, l1-contracts, as corresponding.
…/constants-codegen-release
mverzilli
added
the
ci-release-pr
AztecBot
removed
the
ci-release-pr
mverzilli
added
the
ci-release-pr
AztecBot
removed
the
ci-release-pr
|
|
||
| repo_root=$(git rev-parse --show-toplevel) | ||
| codegen_dir="$repo_root/protocol/constants-codegen" | ||
| additional_input="$repo_root/noir-projects/noir-protocol-circuits/crates/types/src/blob_data/tx_blob_data.nr:MAX_TX_BLOB_DATA_SIZE_IN_FIELDS" |
There was a problem hiding this comment.
Ah well this is a bit of a curve ball. I was hoping we wouldn't need to directly reference the constants.nr or any other protocol file here (which otherwise implies we need to have it checked out). We might have the protocol circuits, but it seems wrong to need them just to be able to get some constants.
(this is not a criticism to your PR! it's moving everything in the right direction)
Do you have a plan?
There was a problem hiding this comment.
Yes, this would need to change. I don't have a detailed plan, more of a cursory idea:
- Move
noir-protocol-circuitsunderprotocol/(this is of course more than just a simple move, there's true deps to be detangled). - Embed
constants.nrwhen packagingconstants-codegen. - Figure out what to do with
MAX_TX_BLOB_DATA_SIZE_IN_FIELDS, maybe it just needs to be moved toconstants.nr? Maybe we do need to embed more than one file? Didn't open that particular box yet.
The end state of this file should look just like running the constants-codegen cli from a published constants-codegen version, so no direct references to either .nr file.
| @@ -1,4 +1,6 @@ | |||
| ^l1-contracts/src/.*\.sol$ | |||
| ^l1-contracts/scripts/remake-constants\.sh$ | |||
| ^protocol/constants-codegen/ | |||
There was a problem hiding this comment.
I'm a bit torn about this one...
Locally, I'm tempted to think "rebuild if the ConstantsGen.sol file changed". Of course, that's tricky because it is the build script itself that generates the .sol file...
Also, if the constants.nr file changes, this probably wouldn't catch the change? (because it's not in that directory [unless the bootrstap file in that dir copies it over -- which it may if it gets embedded])
In any case, I guess what I'm trying to say, consider using the cache_contents_hash approach in the bootstrap file, for dependencies, and consider keeping the list of files here local: https://github.com/AztecProtocol/aztec-packages/blob/next/l1-contracts/bootstrap.sh#L42 . I'm not 100% sure what the convention is across the repo)
There was a problem hiding this comment.
This comment sent me down a rabbit hole trying to determine the best way to go at this point. Here's what I found. There's two different questions here:
constants.nr not detected: it is being detected, but incidentally (via the verifier contract's deps). So I think it is a good call to be explicit about it.
Regarding not using this for external deps here: I'm using it in barretenberg for example, so a concern I have is to keep it consistent. More on that below.
I didn't find any specific docs on the intended usage of .rebuild_patterns, but there's a couple of crumbs that point at it being de-facto used as a way for a component C to refer to a component D rebuild conditions, since the cache_contents_hash doesn't cross component boundaries.
Eg 1: in barretenberg itself, ipc-codegen is referred to (d49dd8b).
Eg 2: https://github.com/AztecProtocol/aztec-packages/blob/next/noir-projects/noir-contracts/bootstrap.sh#L62 (and similarly in yarn-project/)
So I think at this point I would only add a specific reference to constants.nr in this file, which should be fine because noir-protocol-contracts will continue to be under Foundation's repo at the end of the day
| if (!values.input) { | ||
| throw new Error('--input is required'); | ||
| // Resolved relative to this file, so it exists only when the package sits inside the | ||
| // aztec-packages monorepo; the published npm package must be given --input explicitly. |
There was a problem hiding this comment.
Same comment about this. We may want the file embedded. (ok if in a later PR as well)
There was a problem hiding this comment.
Agree. Sharing current thinking to check if we're in the same page:
- Embedding is only needed for dependants outside foundation repo, since inside foundation repo you most likely want to work with latest
constants.nr. constants.nrwill still live undernoir-protocol-contracts(though untangling it from labs projects is pending work),constants.nrcould then be embedded at packing time (this code should then be adjusted accordingly).
There was a problem hiding this comment.
There's now #24958, which embeds constants.nr at packing time but also adds some bash auxiliaries so that in-repo consumers of constants don't need to know where it comes from.
mverzilli
added
the
ci-release-pr
AztecBot
removed
the
ci-release-pr
5 participants
Adds remake-constants scripts to barretenberg and l1-contracts so they control which constants are generated for themselves.
Conversely removes those from
yarn-projects/constants.Removes generated constant files from git, regenerating them on build based on constants.nr as the source of truth.
Closes F-817, F-813, F-818, F-820, F-816