refactor(contracts): follow compact-contracts contracts file structure
#261
Conversation
0xisk
changed the title
compact-contracts contracts file structure
|
TODO: still need to check why the actions are failing. |
| } from '../artifacts/Index/contract/index.cjs'; | ||
|
|
||
| import type { Role } from '../../../artifacts/access/Index/contract/index.cjs'; | ||
| // TODO: (team): I noticed that we are using the mock contract's ledger type here, but I am not sure if this is correct way. |
There was a problem hiding this comment.
Yup, we have to import from the mock contract's artifact file because it's the top level contract. The top level contract artifact file is what exposes the methods and type definitions required to define contracts in TS. The base module artifact file is typically empty
| // TODO: (team): I noticed that we are using the mock contract's ledger type here, but I am not sure if this is correct way. |
There was a problem hiding this comment.
The issue I see with this pattern that sometimes (most of the time maybe) when we write Mock contracts, we don't export ALL the ledger variables. And we end up defining an exported Ledger type in ts that is not mapping the whole ledger types in the mock file. For example: If we look at the AccessControl.mock.compact contract we will see the exported are:
IAccessControl.compact are: midnight-apps/contracts/src/access/interfaces/IAccessControl.compact
Lines 20 to 42 in 09de3e7
export type Ledger = {
readonly isInitialized: boolean;
roleCommits: {
isFull(): boolean;
checkRoot(rt_0: { field: bigint }): boolean;
root(): __compactRuntime.MerkleTreeDigest;
firstFree(): bigint;
pathForLeaf(index_0: bigint, leaf_0: Uint8Array): __compactRuntime.MerkleTreePath<Uint8Array>;
findPathForLeaf(leaf_0: Uint8Array): __compactRuntime.MerkleTreePath<Uint8Array> | undefined
};
readonly index: bigint;
}You can notice that there is a missing ledger type which is roleNullifiers. Imo that is a misleading type as it does not reflect ALL the ledger vars.
That issue I think it should be tracked by the Compiler in the first place, I think the compiler should auto-export all the ledger vars defined in any module (even non-top-level modules) -If you agree with me on this, let's open this discussion with the language team.
At the mean time, I think we need to rethink about how to mitigate this either by introducing another better way of exporting the Ledger/Types vars seperatly or by adding a strict rule of reviewing the *.mock.compact exported vars and compare it with the Ledger. I am leaning towards not depending on the mock file at all, as their main purpose is for testing, and as we need the type Ledger in the definition of the WitnessContext, I would lean towards the first suggestion (which is not clear yet). Again ideally imo that should be the Compiler job. cc: @andrew-fleming
There was a problem hiding this comment.
That issue I think it should be tracked by the Compiler in the first place, I think the compiler should auto-export all the ledger vars defined in any module (even non-top-level modules) -If you agree with me on this, let's open this discussion with the language team.
This would be especially useful for deeply nested modules. I think in practice when a ledger var is exported from a module developers expect it to be exposed in the TS interface.
There was a problem hiding this comment.
At the mean time, I think we need to rethink about how to mitigate this either by introducing another better way of exporting the Ledger/Types vars seperatly or by adding a strict rule of reviewing the *.mock.compact exported vars and compare it with the Ledger. I am leaning towards not depending on the mock file at all, as their main purpose is for testing, and as we need the type Ledger in the definition of the WitnessContext, I would lean towards the first suggestion (which is not clear yet). Again ideally imo that should be the Compiler job.
Ideally, we'd have a codegen tool that generates all of this boilerplate for us. It would be a great addition to compact-tools, but likely require a couple weeks of engineering work.
There was a problem hiding this comment.
That issue I think it should be tracked by the Compiler in the first place, I think the compiler should auto-export all the ledger vars defined in any module (even non-top-level modules) -If you agree with me on this, let's open this discussion with the language team.
Forgive me, this seems dangerous. Automatically exposing state variables will make it easier for devs to unwittingly break invariants. I think being explicit about what's exposed both forces the contract author to consider this and allows the user to see it. Admittedly, being explicit adds a tiny bit of friction to development, but to me, this is a worthwhile tradeoff with security > convenience
Regarding the main point with there being a missing ledger type, AFAICT this seems more like a testing/tooling issue rather than a protocol one. The missing ledger type is a problem if we're trying to view state updates, no? If we don't use it to validate assertions, then we probably don't need to expose it in the mock. LMK if I'm missing something
I think the optimal solution (or one of the better solutions) would be something like this https://github.com/frangio/hardhat-exposed. Instead of the compiler automatically exposing everything, we have tooling that does this in a testing env
There was a problem hiding this comment.
Thank you @andrew-fleming and @emnul for your responses.
Forgive me, this seems dangerous. Automatically exposing state variables will make it easier for devs to unwittingly break invariants. I think being explicit about what's exposed both forces the contract author to consider this and allows the user to see it. Admittedly, being explicit adds a tiny bit of friction to development, but to me, this is a worthwhile tradeoff with security > convenience
@andrew-fleming could you please elaborate more on this? Not sure if I get the security concern here.
Regarding the main point with there being a missing ledger type, AFAICT this seems more like a testing/tooling issue rather than a protocol one. The missing ledger type is a problem if we're trying to view state updates, no? If we don't use it to validate assertions, then we probably don't need to expose it in the mock. LMK if I'm missing something
I think I see the issue more here: https://github.com/OpenZeppelin/compact-contracts/blob/a8e28aec27c0458dd68d963a68b021fea8b83e98/contracts/src/access/test/simulators/ZOwnablePKSimulator.ts#L48 Where the simulator calls the function ledger that returns the type Ledger to extract the public state. So if a ledger variable was not exported at the end in the top-level contract it won't be visible here, that is currently is used for testing. However, in the frontend we can do the same interface idea to inspect the contract on the frontend and if the type [Ledger](https://github.com/OpenZeppelin/midnight-apps/blob/32e78cb4619a0ced162c9bdfc02c1951e36afc90/apps/lunarswap-ui/lib/lunarswap-context.tsx#L4) is missing a ledger variable I will need to get back to the compact contract to export that variable.
I am not sure if in Compact it is the same notion like in Solidity when it comes to define the state with internal, public, or private.
I think the optimal solution (or one of the better solutions) would be something like this frangio/hardhat-exposed. Instead of the compiler automatically exposing everything, we have tooling that does this in a testing env
But that is for exporting the internal functions, so in our case would be good for testing the internal circuits, I think that is diff from exporting the ledger variables.
I think I might be just confusing the idea of exporting with the Solidity modifiers
There was a problem hiding this comment.
Re-evaluating the Placement of Witness Scripts in compact-contracts
After looking at this again, I realized my initial confusion came from the fact that the src/witnesses/*.ts files in compact-contracts live inside src/ rather than inside the test directory. At first, I assumed these witness scripts were meant to be part of the package’s public API and would be exported from @openzeppelin/compact-contracts. But using the ShieldedAccessControl example made it clear that this isn’t the case.
For instance, in the shielded-access-control branch,
contracts/src/access/witnesses/ShieldedAccessControlWitnesses.ts
depends directly on the exports of the MockShieldedAccessControl contract. This dependency is only meaningful in a testing or simulation context — not in an actual production Compact module.
That’s why it now seems that these witness scripts are essentially mocks for private data, similar in nature to the MODULE.mock.compact artifacts. They help exercise circuits and simulate private inputs, but they are not part of the runtime interface that gets shipped to developers.
Given that, their placement inside src/ is misleading. From the outside, it makes them appear as part of the library’s official surface area, even though in practice they behave like test-only witness implementations. Because they depend on mock contracts and are not meant to be consumed by users, their natural place seems to be inside the module’s test directory next to the mocks and simulators. Let me know wdyt.
I will reply in another message regarding the exported ledger discussion, since that’s a separate concern.
There was a problem hiding this comment.
This is my second point, and it's more complex. I wrote it here so we can discuss it in the discussions easier if it makes sense we can then open it as an issue. #272
contracts/src/shielded-token/openzeppelin/ShieldedERC20.compact
Outdated
Show resolved
Hide resolved
emnul
left a comment
emnul
left a comment
There was a problem hiding this comment.
Left a few comments that should be discussed before PR is finalized
0xisk
force-pushed
the
refactor/contracts-structure
branch
from
c4d17fa to
101342d
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request restructures the contracts monorepo by consolidating nested workspace packages into a unified root-level structure, relocating source files to centralized paths, replacing external package dependencies with local module references, and updating module resolution to ES module format with explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/math/witnesses/Uint256.ts (1)
128-199: Fix JSDoc return type indivU128LocallyLine 133 incorrectly documents the return type as
DivResultU64when it should beDivResultU128:- * @returns A tuple of the unchanged private state and a DivResultU64 with quotient and remainder. + * @returns A tuple of the unchanged private state and a DivResultU128 with quotient and remainder.Additionally, all
div*helpers assume a non-zero divisor without validation. If circuit constraints guarantee this, add a clarifying comment; otherwise, consider documenting the requirement explicitly or adding a check.contracts/src/access/witnesses/AccessControlWitnesses.ts (1)
46-65: Remove unusedwit_getRoleMerklePathhelper with incorrect signatureThe helper function at lines 73–79 in
AccessContractPrivateStatehas an incorrect signature that doesn't match theIAccessControlWitnessesinterface. It takes auser: ZswapCoinPublicKeyand returnsRole, whereas the interface specifiesuserRoleCommit: Uint8ArrayandMaybe<MerkleTreePath<Uint8Array>>. This helper is never called in the codebase and should be removed entirely. The actual witness method implementation at lines 143–168 correctly uses the proper key format (userRoleCommit.toString()) and return type.
♻️ Duplicate comments (3)
contracts/src/shielded-token/openzeppelin/ShieldedERC20.compact (1)
126-152: Address the type inconsistency and convert TODO to a tracked issue.The TODO comment documents a compilation constraint, but there's a functional inconsistency:
mintacceptsUint<64>(line 137)burnacceptsUint<128>(line 165)_totalSupplyisUint<128>(line 42)This limits minting to 2^64-1 per transaction while burning can handle up to 2^128-1. As a previous reviewer noted, this should be tracked as a GitHub issue rather than left as a code comment.
Based on past review comments, please create a GitHub issue to track this compilation limitation and document the workaround rationale. Consider whether this constraint is acceptable for the contract's intended use cases.
.gitmodules (1)
1-3: Make sure CI and local setup initialize thecompact-toolssubmodule correctlyThe submodule entry itself looks fine. Just ensure:
- GitHub Actions (or other CI) use
actions/checkoutwith submodules enabled (or equivalent) so installs don’t fail when@openzeppelin/compact-tools-*are resolved from../compact-tools/....- CONTRIBUTING / README clearly mention
git submodule update --init --recursive(or similar) so contributors don’t hit missing-tooling errors.contracts/src/access/witnesses/interface.ts (1)
2-5: Revisit Ledger type source (mock artifact) and resolve TODOThe
Ledgertype is still imported from the mock contract artifact, and the TODO highlights uncertainty about this pattern. Given the earlier discussion on potential mismatches between mock‑exported ledger vars and the full on‑chain ledger surface, it would be good to either (a) switch to a canonical Ledger type from the interface artifact (if now available) or (b) clearly document why the mock artifact is the single source of truth and how you ensure it stays in sync.
🧹 Nitpick comments (7)
contracts/src/shielded-token/ShieldedFungibleToken.compact (1)
16-16: Consider the implications of hardcoded decimals.The decimals value is hardcoded to 18, which matches the Ethereum standard but prevents creating tokens with different decimal precision. If all tokens in this system should always use 18 decimals, this is fine. Otherwise, consider making it a constructor parameter.
If flexibility is desired, apply this diff:
-constructor(nonce_: Bytes<32>, - name_: Opaque<"string">, - symbol_: Opaque<"string">, - domain_: Bytes<32>) { - const decimals = 18; +constructor(nonce_: Bytes<32>, + name_: Opaque<"string">, + symbol_: Opaque<"string">, + decimals_: Uint<8>, + domain_: Bytes<32>) { + const decimals = decimals_; ShieldedFungibleToken_initialize(nonce_, name_, symbol_, decimals, domain_); }contracts/package.json (1)
23-39:cleanscript is repo‑wide and potentially destructive; consider narrowing its scope
"clean": "git clean -fXd"will operate from the contracts directory but still target the whole git repo. That’s powerful and can wipe any ignored artifacts across the monorepo, not just contracts outputs.If you want a safer contracts-only clean, consider scoping it to known build artifacts (e.g.
rm -rf dist .turbo node_modules) or documenting clearly that this is a repo‑wide clean to avoid surprises.package.json (1)
9-27: Align allcompact:*scripts to explicitly target the contracts packageThe updated scripts correctly filter build/test/types to
./contracts, andcompactnow scopes to@openzeppelin/midnight-apps-contracts, which matches the new workspace layout.To keep behavior explicit and future‑proof, consider adding the same filter to the other
compact:*scripts so they can’t accidentally fan out if another package ever defines acompacttask:- "compact:fast": "turbo run compact -- --skip-zk", - "compact:version": "turbo run compact -- --version", - "compact:language-version": "turbo run compact -- --language-version", + "compact:fast": "turbo run compact --filter=@openzeppelin/midnight-apps-contracts -- --skip-zk", + "compact:version": "turbo run compact --filter=@openzeppelin/midnight-apps-contracts -- --version", + "compact:language-version": "turbo run compact --filter=@openzeppelin/midnight-apps-contracts -- --language-version",contracts/src/math/witnesses/Field254.ts (1)
42-86: Consider extracting U256 conversion helpers.The bigint-to-U256 conversion logic (splitting into 64-bit chunks) is duplicated in
divUint254Locally,divU256Locally, and similar functions acrossBytes32.tsandUint128.ts. Consider extracting this into a shared utility (e.g., inu256.tswhich already hastoU256/fromU256).This could be addressed in a follow-up PR since the current focus is file structure refactoring.
Also applies to: 118-174
contracts/src/access/types/ledger.ts (1)
9-13: Type re‑exports are correct; JSDoc is slightly outdatedRe‑exporting
Ledgerfrom the AccessControl artifact andRolefrom the Index artifact is the right way to keep TS types in sync with generated contracts. The descriptive comment about renamingAccessControl_Role→AccessControlRoleno longer matches the actual exported names; consider updating it to mentionRoleinstead.contracts/src/access/witnesses/AccessControlWitnesses.ts (1)
3-5: Avoid depending on test utilities (emptyMerkleTreePath) in witness implementationThe imports overall look fine, but pulling
emptyMerkleTreePathfrom../utils/test.jscouples production witness code to a test‑oriented module. That can make packaging/bundling harder and blurs the test vs runtime boundary. Consider movingemptyMerkleTreePathinto a non‑test utility module (or inlining a minimal constant) and importing from there instead.Also applies to: 9-13
contracts/src/access/test/accessControl.test.ts (1)
22-23: Consider narrowing thedescribe.skipto pathForLeaf‑dependent testsSkipping the entire
AccessControlsuite because of thepathForLeafissue significantly reduces coverage, including tests that don’t depend on Merkle paths (e.g., initialization and basic grantRole behavior). It would be better to either isolate the pathForLeaf/hasRole cases into a separate skipped block or gate those specific tests, and keep the rest running, while tracking the underlying bug in an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (90)
.gitmodules(1 hunks)compact-tools(1 hunks)contracts/access/package.json(0 hunks)contracts/access/src/Index.compact(0 hunks)contracts/access/tsconfig.build.json(0 hunks)contracts/access/tsconfig.json(0 hunks)contracts/access/vitest.config.ts(0 hunks)contracts/compact-std/.lintstagedrc.json(0 hunks)contracts/compact-std/biome.json(0 hunks)contracts/compact-std/package.json(0 hunks)contracts/compact-std/tsconfig.build.json(0 hunks)contracts/compact-std/tsconfig.json(0 hunks)contracts/compact-std/vitest.config.ts(0 hunks)contracts/math/.lintstagedrc.json(0 hunks)contracts/math/biome.json(0 hunks)contracts/math/package.json(0 hunks)contracts/math/tsconfig.build.json(0 hunks)contracts/math/tsconfig.json(0 hunks)contracts/math/vitest.config.ts(0 hunks)contracts/package.json(1 hunks)contracts/shielded-token/.lintstagedrc.json(0 hunks)contracts/shielded-token/README.md(0 hunks)contracts/shielded-token/biome.json(0 hunks)contracts/shielded-token/package.json(0 hunks)contracts/shielded-token/src/ShieldedFungibleToken.compact(0 hunks)contracts/shielded-token/src/types/index.ts(0 hunks)contracts/shielded-token/tsconfig.build.json(0 hunks)contracts/shielded-token/tsconfig.json(0 hunks)contracts/src/access/AccessControl.compact(5 hunks)contracts/src/access/Index.compact(1 hunks)contracts/src/access/index.ts(1 hunks)contracts/src/access/interfaces/IAccessControl.compact(1 hunks)contracts/src/access/test/AccessControl.mock.compact(2 hunks)contracts/src/access/test/AccessControlSimulator.ts(3 hunks)contracts/src/access/test/accessControl.test.ts(10 hunks)contracts/src/access/types/ledger.ts(1 hunks)contracts/src/access/types/role.ts(1 hunks)contracts/src/access/utils/compactHelper.ts(1 hunks)contracts/src/access/utils/test.ts(1 hunks)contracts/src/access/witnesses/AccessControlWitnesses.ts(8 hunks)contracts/src/access/witnesses/interface.ts(4 hunks)contracts/src/math/index.ts(1 hunks)contracts/src/math/test/Bytes32.test.ts(1 hunks)contracts/src/math/test/Bytes32Simulator.ts(1 hunks)contracts/src/math/test/Field254.test.ts(1 hunks)contracts/src/math/test/Field254Simulator.ts(1 hunks)contracts/src/math/test/Max.test.ts(1 hunks)contracts/src/math/test/MaxSimulator.ts(1 hunks)contracts/src/math/test/Uint128.test.ts(1 hunks)contracts/src/math/test/Uint128Simulator.ts(1 hunks)contracts/src/math/test/Uint256.test.ts(1 hunks)contracts/src/math/test/Uint256Simulator.ts(1 hunks)contracts/src/math/test/Uint64.test.ts(1 hunks)contracts/src/math/test/Uint64Simulator.ts(1 hunks)contracts/src/math/utils/consts.test.ts(1 hunks)contracts/src/math/utils/sqrtBigint.test.ts(1 hunks)contracts/src/math/utils/u256.test.ts(1 hunks)contracts/src/math/utils/u256.ts(1 hunks)contracts/src/math/witnesses/Bytes32.ts(1 hunks)contracts/src/math/witnesses/Field254.ts(1 hunks)contracts/src/math/witnesses/Uint128.ts(1 hunks)contracts/src/math/witnesses/Uint256.ts(1 hunks)contracts/src/math/witnesses/Uint64.ts(1 hunks)contracts/src/math/witnesses/interfaces.ts(1 hunks)contracts/src/shielded-token/ShieldedFungibleToken.compact(1 hunks)contracts/src/shielded-token/index.ts(1 hunks)contracts/src/shielded-token/openzeppelin/ShieldedERC20.compact(4 hunks)contracts/src/shielded-token/openzeppelin/Utils.compact(3 hunks)contracts/src/shielded-token/test/ShieldedFungibleToken.test.ts(1 hunks)contracts/src/shielded-token/test/ShieldedFungibleTokenSimulator.ts(1 hunks)contracts/src/shielded-token/types/index.ts(1 hunks)contracts/src/shielded-token/witnesses/index.ts(1 hunks)contracts/src/structs/index.ts(1 hunks)contracts/src/structs/test/QueueContractSimulator.ts(1 hunks)contracts/src/structs/test/queueContract.test.ts(1 hunks)contracts/src/structs/utils/test.ts(1 hunks)contracts/src/structs/witnesses/QueueWitnesses.ts(1 hunks)contracts/src/types/StandardLibrary.test.ts(1 hunks)contracts/src/types/StandardLibrary.ts(1 hunks)contracts/structs/.lintstagedrc.json(0 hunks)contracts/structs/biome.json(0 hunks)contracts/structs/package.json(0 hunks)contracts/structs/tsconfig.build.json(0 hunks)contracts/structs/tsconfig.json(0 hunks)contracts/structs/vitest.config.ts(0 hunks)contracts/tsconfig.build.json(1 hunks)contracts/tsconfig.json(1 hunks)contracts/vitest.config.ts(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (31)
- contracts/compact-std/biome.json
- contracts/access/vitest.config.ts
- contracts/shielded-token/src/types/index.ts
- contracts/shielded-token/README.md
- contracts/shielded-token/package.json
- contracts/compact-std/package.json
- contracts/structs/vitest.config.ts
- contracts/math/vitest.config.ts
- contracts/structs/.lintstagedrc.json
- contracts/access/package.json
- contracts/compact-std/tsconfig.json
- contracts/shielded-token/tsconfig.json
- contracts/structs/biome.json
- contracts/math/tsconfig.build.json
- contracts/structs/tsconfig.json
- contracts/shielded-token/src/ShieldedFungibleToken.compact
- contracts/math/package.json
- contracts/shielded-token/tsconfig.build.json
- contracts/structs/tsconfig.build.json
- contracts/compact-std/.lintstagedrc.json
- contracts/access/tsconfig.json
- contracts/math/tsconfig.json
- contracts/shielded-token/.lintstagedrc.json
- contracts/math/biome.json
- contracts/access/tsconfig.build.json
- contracts/access/src/Index.compact
- contracts/compact-std/tsconfig.build.json
- contracts/structs/package.json
- contracts/math/.lintstagedrc.json
- contracts/shielded-token/biome.json
- contracts/compact-std/vitest.config.ts
🧰 Additional context used
🧬 Code graph analysis (5)
contracts/src/access/test/AccessControlSimulator.ts (2)
contracts/src/shielded-token/index.ts (1)
ledger(12-12)contracts/src/access/index.ts (1)
Role(16-16)
contracts/src/access/witnesses/interface.ts (1)
contracts/src/access/index.ts (2)
Ledger(17-17)Role(16-16)
contracts/src/access/witnesses/AccessControlWitnesses.ts (3)
contracts/src/access/index.ts (3)
AccessContractPrivateState(9-9)Role(16-16)Ledger(17-17)contracts/src/types/StandardLibrary.ts (1)
ZswapCoinPublicKey(16-16)contracts/src/access/utils/compactHelper.ts (1)
maybeFromNullable(11-24)
contracts/src/access/test/accessControl.test.ts (1)
contracts/src/access/types/role.ts (1)
RoleValue(7-19)
contracts/src/access/types/role.ts (1)
contracts/src/access/index.ts (2)
RoleValue(20-20)Role(16-16)
🪛 GitHub Actions: Apps
compact-tools
[error] 1-1: ENOENT: no such file or directory, scandir '/home/runner/work/midnight-apps/midnight-apps/compact-tools/packages/cli'. Build process failed during npm run build with exit code 254.
🪛 GitHub Actions: Checks
compact-tools
[error] 1-1: ENOENT: no such file or directory, scandir '/home/runner/work/midnight-apps/midnight-apps/compact-tools/packages/cli'. The build failed during 'npm run build' in package 'compact'.
🪛 GitHub Actions: Contracts
compact-tools
[error] 1-1: ENOENT: no such file or directory, scandir '/home/runner/work/midnight-apps/midnight-apps/compact-tools/packages/cli'. This occurred during 'npm run build' in the 'packages/compact' workspace, causing the pipeline to fail with exit code 254.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (60)
contracts/src/shielded-token/openzeppelin/Utils.compact (1)
17-19: Formatting and style adjustments align with refactoring objectives.The indentation and spacing adjustments normalize code style across the file—particularly the consistent spacing before type constructors (e.g.,
ContractAddress { }instead ofContractAddress{ }). These changes introduce no functional impact and align with the goal of following thecompact-contractsfile structure.Also applies to: 29-29, 76-76, 84-84
contracts/src/shielded-token/openzeppelin/ShieldedERC20.compact (1)
37-37: > Likely an incorrect or invalid review comment.contracts/src/shielded-token/ShieldedFungibleToken.compact (3)
1-8: LGTM! Clean contract structure.The license header, pragma, and imports are well-structured. The use of the prefix mechanism for importing ShieldedERC20 enables clean delegation to the underlying implementation.
20-38: LGTM! Clean delegation pattern.The getter circuits properly delegate to the underlying implementation. The use of Uint<128> for totalSupply is appropriate for representing large token quantities.
40-46: Type difference between mint and burn is intentional design; access control is explicitly absent and documented.The
Uint<64>parameter inmintvsUint<128>inburnis deliberate: the mint function accepts amounts up to 2^64 - 1, but internally casts toUint<128>when updating_totalSupply. This allows individual mint operations to be size-limited while supporting larger accumulated burn operations. No fix needed here.However, the documented caveat applies: the ShieldedERC20 module is marked "DO NOT USE IN PRODUCTION" and explicitly states that both
mintandburncircuits include no access control, meaning anyone can call them. This is a known limitation of the current implementation documented in the module header.contracts/src/shielded-token/test/ShieldedFungibleTokenSimulator.ts (2)
21-22: All import paths resolve correctly and the necessary exports are present.
../../types/StandardLibrary.js→contracts/src/types/StandardLibrary.tscontains all imported types (CoinInfo, ContractAddress, Either, ZswapCoinPublicKey)../types/test.js→contracts/src/shielded-token/types/test.tsexports IContractSimulator../witnesses.js→contracts/src/shielded-token/witnesses/index.tsexports ShieldedFungibleTokenPrivateState and ShieldedFungibleTokenWitnesses
11-15: The artifact import path../../../artifacts/shielded-token/ShieldedFungibleToken/contract/index.cjsis correctly structured for accessing generated Compact compiler artifacts. This relative path from the test directory to the generated artifacts root is standard practice and not a source of complexity or errors. Artifact generation is part of the normal build pipeline orchestrated by Turbo.Likely an incorrect or invalid review comment.
contracts/tsconfig.json (1)
7-16: Add explicitmoduleResolutionoption.The configuration does not explicitly specify
moduleResolution, which will be inherited from@tsconfig/node24/tsconfig.json. Since the AI summary indicates the project uses explicit.jsfile extensions for ES module format, you should explicitly setmoduleResolutionto match Node's actual module resolution behavior. Based on the past review feedback, this should be clarified rather than implicitly inherited.Please verify the appropriate
moduleResolutionvalue:
- Use
"nodenext"for modern Node.js ESM support with explicit extensions- Or use
"bundler"depending on your tooling requirements"isolatedModules": true, "resolveJsonModule": true, + "moduleResolution": "nodenext" }You can verify this matches your module import patterns by checking the documentation for TypeScript moduleResolution.
contracts/tsconfig.build.json (1)
1-5: LGTM!The build configuration properly extends the base config and correctly excludes test files for the build output. Clean and well-structured.
contracts/src/shielded-token/witnesses/index.ts (1)
1-2: Imports look consistent with the new artifacts layout and ESM styleThe updated
Ledgerartifact path and the.jsextension forEmptyStateboth look correct given asrc/→artifacts/structure and ESM-friendly TS imports. Since this relies on your TS module resolution and declaration files lining up withindex.cjs/state.js, just make sure a fulltscrun passes after the refactor.contracts/src/shielded-token/index.ts (1)
3-14: Artifact re‑export path is correct and consistently used throughout the codebaseThe new
../../artifacts/shielded-token/ShieldedFungibleToken/contract/index.cjspath in the barrel export is correct and matches the relative path expectations fromcontracts/src/shielded-token/. No remaining old artifact paths exist in the codebase—all references already use the centralized artifact location consistently.pnpm-workspace.yaml (1)
1-4: Workspace scope update forcontractslooks consistentSwitching to a single
contractsworkspace package matches the new monolithiccontracts/package.jsonand the updated Turbo filters. No issues from what’s visible here.contracts/package.json (1)
41-58: Confirmcompact-toolspaths and CI behavior for localfile:devDependenciesUsing local
file:links to thecompact-toolssubmodule from here:"@openzeppelin/compact-tools-cli": "file:../compact-tools/packages/cli", "@openzeppelin/compact-tools-simulator": "file:../compact-tools/packages/simulator"is reasonable and matches the
.gitmoduleslayout. Just make sure that:
- CI is cloning submodules before
pnpm install, otherwise these paths will fail resolution.- Local dev instructions clearly mention initializing the submodule so
pnpmcan see these packages.Everything else in the dependencies block looks coherent with the new structure.
contracts/vitest.config.ts (1)
3-9: Verbose reporter addition looks fineUsing
reporters: 'verbose'undertestshould just increase per‑test output without affecting which tests run (includeis still restricted tosrc/**/*.test.ts). No issues from what’s shown.contracts/src/math/utils/consts.test.ts (1)
1-9: LGTM!The import path update to use explicit
.jsextension aligns with ES module resolution requirements. The test logic remains unchanged and correctly validates allMAX_UINT*constants.contracts/src/access/utils/test.ts (1)
10-10: LGTM!The import path update to include the explicit
.jsextension is consistent with the ES module migration across the codebase.contracts/src/math/utils/u256.test.ts (1)
1-3: LGTM!Import paths updated consistently with the ES module migration. The test suite provides comprehensive coverage including edge cases and round-trip conversions.
contracts/src/math/witnesses/Bytes32.ts (1)
1-7: LGTM!Import paths consistently updated to match the new artifact structure. The witness implementations correctly handle U256 conversions.
contracts/src/math/witnesses/Field254.ts (1)
1-9: LGTM!Import paths consistently updated to the new artifact structure with appropriate extensions (
.d.cts,.cjs,.js).contracts/src/math/witnesses/Uint128.ts (1)
1-9: Artifact imports are correctly structured for generated files.The imports reference generated artifact files from
artifacts/math/Index/contract/index.d.ctsandartifacts/math/test/Uint128.mock/contract/index.cjs, which are properly gitignored. The.d.ctsextension for type declarations and.cjsextension for the CommonJS mock Ledger are appropriate for their respective module systems.contracts/src/math/utils/u256.ts (1)
1-2: Imports follow the codebase convention; artifact path verification deferred to post-build.Both imports are correct. The
.jsextension pattern in TypeScript source files is consistent across the codebase with"type": "module"in package.json. The artifact path../../../artifacts/math/Index/contract/index.d.ctscannot be verified in the pre-build state, but the import structure is sound and will resolve correctly after the build process generates the artifact files.contracts/src/math/test/Uint64.test.ts (1)
2-6: LGTM! Import paths updated for ES module resolution.The explicit
.jsextensions align with ES module conventions and are consistent with the PR's refactoring objectives. No logic changes detected.contracts/src/math/witnesses/interfaces.ts (1)
9-9: LGTM! Artifact path updated for new directory structure.The import path now correctly references the restructured artifact location (
../../../artifacts/math/...) and uses the.d.ctsextension for CommonJS type declarations. This aligns with the PR's goal of following thecompact-contractsstructure.contracts/src/math/test/Max.test.ts (1)
2-2: LGTM! Import path updated for ES module resolution.The explicit
.jsextension is consistent with the repository-wide migration to ES module conventions.contracts/src/math/test/Field254.test.ts (1)
2-3: LGTM! Import paths updated for ES module resolution.Both utility and simulator imports now use explicit
.jsextensions, consistent with the PR's ES module migration.contracts/src/math/test/Bytes32.test.ts (1)
2-2: LGTM! Import path updated for ES module resolution.The explicit
.jsextension maintains consistency with the repository-wide ES module adoption.contracts/src/math/utils/sqrtBigint.test.ts (1)
2-2: LGTM! Import path updated for ES module resolution.The utility import now uses the explicit
.jsextension, aligning with ES module conventions.contracts/src/structs/witnesses/QueueWitnesses.ts (1)
1-1: LGTM! Type import path updated for ES module resolution.The type import now uses the explicit
.jsextension, consistent with the ES module migration across the codebase.contracts/src/structs/test/queueContract.test.ts (1)
3-3: LGTM! Import path updated for ES module resolution.The simulator import now uses the explicit
.jsextension, maintaining consistency with the repository-wide ES module adoption.contracts/src/math/test/Uint256.test.ts (1)
10-14: LGTM! ES module import paths updated correctly.The addition of
.jsextensions to import paths aligns with ES module standards and is consistent with the broader refactoring across the codebase.contracts/src/types/StandardLibrary.test.ts (1)
14-14: LGTM! More explicit import path.The change from
'./index'to'./StandardLibrary.js'makes the import more explicit and aligns with ES module conventions.contracts/src/structs/utils/test.ts (1)
7-7: LGTM! ES module import updated correctly.contracts/src/math/test/Uint128.test.ts (1)
9-13: LGTM! Consistent ES module import updates.contracts/src/structs/index.ts (1)
10-10: LGTM! Export path updated for ES modules.contracts/src/shielded-token/types/index.ts (1)
1-2: LGTM! New type exports added.This new barrel export file expands the public API surface by exposing
IContractSimulatorandEmptyStatetypes, which aligns with the refactoring goals.contracts/src/access/utils/compactHelper.ts (1)
1-1: All three migration requirements are satisfied.The
Maybetype is properly exported from the localStandardLibrary.tsmodule (confirmed by both the export and passing test), no remaining imports from@openzeppelin/midnight-apps-compact-stdexist in the codebase, and the external package has been removed frompackage.jsondependencies. The migration from the external package to the local module is complete and consistent.contracts/src/math/witnesses/Uint64.ts (1)
2-6: Artifact import paths are correctly configured and will be generated by the build process.The relative import paths from
contracts/src/math/witnesses/Uint64.tscorrectly resolve tocontracts/artifacts/math/..., which is generated during the build process. Thebuild:contractstask explicitly handles "artifact copying" after thecompacttask compiles.compactfiles. The explicit file extensions (.d.ctsfor TypeScript definitions and.cjsfor CommonJS modules) are appropriate and consistent across all witness implementations (Uint64, Uint128, Uint256, Bytes32, Field254, and interfaces).contracts/src/access/Index.compact (1)
6-23: Barrel export for access ledger state looks consistentThe module-level docs and the re-export list (
Role,isInitialized,roleCommits,roleNullifiers,index) line up with what you’d expect fromIAccessControland provide a clean, centralized surface; no issues spotted in the structure of this index module.contracts/src/types/StandardLibrary.ts (1)
6-18: Stdlib type re-export path change preserves public APIThe switch to
../../artifacts/types/Std/contract/index.cjskeeps the exported type set (Maybe,Either,CurvePoint,MerkleTree*,ContractAddress,CoinInfo,QualifiedCoinInfo,ZswapCoinPublicKey,SendResult) intact, so callers see a stable API with only the underlying source moved. No further changes needed here.contracts/src/math/test/Bytes32Simulator.ts (1)
13-18: Bytes32 simulator imports align with new artifacts layoutThe updated mock contract, simulator type, and witness imports (including
.jsextensions) are consistent with the rest of the math test suite and do not change runtime behavior. Looks good.contracts/src/math/witnesses/Uint256.ts (1)
1-11: Uint256 witness imports now point at the math index and test artifactsThe new import paths for
DivResultU128/DivResultU256/U128/U256,Ledger,EmptyState,sqrtBigint, andIUint256Witnessesare consistent with the reorganized math artifacts and local helper layout. The witness factory and arithmetic logic remain unchanged, so behavior should be preserved.contracts/src/math/test/Uint256Simulator.ts (1)
12-23: Uint256 simulator imports correctly track new math artifacts and witnessesThe updated imports for
DivResultU128/DivResultU256/U256, the Uint256 mock contract, simulator type, andUint256Witnesses(with.jsextension) are consistent with the math refactor and keep the simulator’s behavior intact. No further changes needed here.contracts/src/math/test/Uint128Simulator.ts (1)
12-23: Uint128 simulator now targets the shared math index and test artifactsThe new type and mock contract imports, plus the
.jsextensions for the simulator types and witnesses, are aligned with the updated math layout and other simulators. Public surface and behavior remain unchanged.contracts/src/math/test/Field254Simulator.ts (1)
12-25: Field254 simulator imports are consistent with the new math artifacts structureThe updated type, mock contract, simulator type, and witness imports (including
.jsextensions) match the patterns in the other math simulators and keep the simulator behavior the same. The malicious override wiring still looks correct.contracts/src/structs/test/QueueContractSimulator.ts (1)
10-18: No action required—witness import and constructor usage are correctQueueWitnesses is exported as a plain object (
export const QueueWitnesses = {}), not a factory function. Passing it directly in the constructor (new MockQueue<QueueContractPrivateState>(QueueWitnesses)) is correct and matches the contract's expectations. This differs from the math witnesses pattern (Bytes32Witnesses, Uint256Witnesses) which are factory functions, but that's intentional—QueueWitnesses is a static object with no initialization logic required.contracts/src/math/test/Uint64Simulator.ts (1)
12-19: Import path and .js extension updates look consistentThe updated artifact paths and
.jsextensions forDivResultU64,Contract/Ledger,IContractSimulator, andUint64PrivateState/Uint64Witnessesall align with the newartifacts/math/...layout and ESM-style resolution. No behavioral changes introduced here.contracts/src/access/types/role.ts (1)
1-19: RoleValue now tied to canonical Role and StandardLibrary typesWiring
role: Roleand importingMerkleTreePathfromStandardLibrary.jsensures this type stays in sync with the ledger/artifact definitions while remaining purely type-level. This is a straightforward and correct refactor.contracts/src/access/interfaces/IAccessControl.compact (1)
1-42: IAccessControl interface cleanly centralizes Role and public ledgers
Roleand the four exported ledgers match the access-control surface used elsewhere (mock, simulator, TS ledger types). The declarations look consistent with CompactStandardLibrary conventions and should generate coherent artifacts for the TS layer.contracts/src/math/test/MaxSimulator.ts (1)
8-18: Max simulator imports correctly updated to artifacts and .js pathsUsing the
.d.ctsartifact forU128/U256types and theMax.mockCJS contract forContract/Ledger, plus switching local imports to.js, is consistent with the rest of the math test suite and doesn’t alter runtime behavior.contracts/src/access/test/AccessControlSimulator.ts (3)
11-23: Simulator imports now reference shared artifact/JS surfacesImporting
Rolefrom the accessIndexartifact,Ledger/MockAccessControlfrom the test artifact, and switching StandardLibrary, simulator types, and witnesses to.jspaths matches the new structure and keeps the simulator aligned with the generated artifacts.
79-81: Double‑check usingoriginalState.datafor public ledger decodingSwitching
getCurrentPublicStatetoledger(this.circuitContext.originalState.data)may change semantics compared to usingtransactionContext.state: if circuits only mutatetransactionContext.stateand leaveoriginalStateas the pre‑call snapshot, this will always return the initial ledger instead of the updated one.Please confirm that
originalStateis indeed updated byMockAccessControlcircuits; if not, consider retaining the previoustransactionContext.state‑based call.
106-122: GrantRole now correctly uses the canonical Role enumUpdating
grantRoleto takerole: Roleties the simulator directly to the new enum defined in the access artifacts/interface, keeping the test surface consistent with the mock (testGrantRole) and the Compact interface.contracts/src/access/index.ts (1)
8-14: Barrel now targets compiled JS modules for witnesses and interfacesPointing
AccessContractPrivateState/AccessControlWitnessesandIAccessControlWitnessesat their.jscounterparts is consistent with the project‑wide ESM move and should keep the entrypoint usable from JS and TS without changing semantics.contracts/src/access/test/AccessControl.mock.compact (2)
10-14: Mock now re‑exports state and Role from the shared interfaceImporting
IAccessControland exportingroleCommits,isInitialized,index, andRolefrom here keeps the mock’s public surface aligned with the canonical interface while still exposingAccessControl_hashUserRolefor tests. This is a good consolidation point for access‑control state in the test harness.
32-34:testGrantRolecorrectly updated to use the new Role typeThe test circuit now accepts
role: Roleand forwards it toAccessControl_grantRole, matching the new enum defined inIAccessControl/artifacts. This keeps the mock and simulator signatures consistent.contracts/src/access/witnesses/interface.ts (1)
20-25: wit_ witness interface looks consistent with the compact contract*The
wit_updateRole,wit_getRoleMerklePath, andwit_getSecretKeysignatures (context + args →[P, …]) look coherent with the AccessControl.compact witness declarations and the TS implementation inAccessControlWitnesses.ts; I don't see issues here.Also applies to: 33-36, 43-43
contracts/src/math/index.ts (1)
10-33: Math barrel exports updated cleanly to new module layoutThe
.jsspecifiers and added exports (sqrtBigint,IContractSimulator,EmptyState) keep this entrypoint aligned with the rest of the refactor without introducing behavior changes; looks good.contracts/src/access/witnesses/AccessControlWitnesses.ts (1)
120-135: Witness implementations align with interface and compact contract semanticsThe
AccessControlWitnessesmethods (wit_updateRole,wit_getRoleMerklePath,wit_getSecretKey) correctly wrapAccessContractPrivateState, operate onWitnessContext<Ledger, AccessContractPrivateState>, and return[privateState, …]as expected. The guard around missingroleEntry/indexbefore callingroleCommits.pathForLeafis also appropriate. No issues from my side here.Also applies to: 143-168, 175-179
contracts/src/access/test/accessControl.test.ts (1)
7-11: Tests correctly track new Role and ledger/public‑state surfaceThe updated imports (artifact‑sourced
Role, mockpureCircuits,.jssimulator, and localRoleValue) and the expectations aroundisInitialized,roleCommits,index, and storedRoleValue.indexall look consistent with the refactored AccessControl contract and private‑state helpers. The index assumptions (0n for the initial admin, then 2n after a single grant) and the count of role entries (3 for admin + Lp + Trader) align with the underlying_grantRolelogic.Also applies to: 30-38, 40-58, 63-82, 96-121, 124-141, 148-161, 166-199
contracts/src/access/AccessControl.compact (1)
14-37: AccessControl.compact witness wiring matches the new interface and TS layerImporting
./interfaces/IAccessControland exportingwit_updateRole,wit_getRoleMerklePath, andwit_getSecretKeybrings the compact contract’s public witness surface in line with the shared interface and the TS witness implementation. The updated call sites in_grantRole,hasRole, andhashNullifieruse these witnesses coherently, so the on‑chain circuit, ledger, and host‑side witnesses stay in sync.Also applies to: 83-86, 169-170, 183-186
|
This PR is depending on the hierarchical feature from this PR: OpenZeppelin/compact-tools#27 |
|
@andrew-fleming I am fixing some of the requested changes from #269 here because they were actually related to this PR. Also fixing still the failed workflows |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Looking good, @0xisk! Left some comments
| "extends": "./tsconfig.json", | ||
| "exclude": ["src/test/**/*.ts"], | ||
| "compilerOptions": {} | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
contracts/package.json
Outdated
| "compact": "compact-compiler", | ||
| "compact:fast": "compact-compiler --skip-zk", |
There was a problem hiding this comment.
The root turbo.json needs to change the compact outputs to this:
"outputs": [
"artifacts/**"
]
There was a problem hiding this comment.
There's also type issues as well...not sure if that's meant to be handled in future PRs or if that should be resolved now
There was a problem hiding this comment.
I think the types issue should be resolved now.
contracts/src/access/types/ledger.ts
Outdated
| * @typedef {Object} Ledger | ||
| */ | ||
| // biome-ignore lint/performance/noBarrelFile: entrypoint module | ||
| export { |
There was a problem hiding this comment.
Line 11: ⚠ Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
There was a problem hiding this comment.
That file is deleted entirely
There was a problem hiding this comment.
The submodules should be ignored for all relevant commands in this manifest
There was a problem hiding this comment.
I don't think the scripts affect the submodules, could you confirm if they get affected on your side?
| "compact-tools/packages/cli/dist/runCompiler.js" | ||
| "compact-tools/packages/cli/dist/runBuilder.js" |
There was a problem hiding this comment.
Do we need both? Consider that build already includes compact in the turbo.json
…ems pathForLeaf is working now
…55, and Bytes32 for future PR because of the breaking change
1. Bytes32, field255, and Uint256 all are affected by the Uint breaking change. 2. std liberary has a lot of change and therefore the types package has to be updated
…e witnesses inside tests
… witnesses inside tests
|
@andrew-fleming, pushed huge improvements and changes, the main issue was that the Now, because of the Finally, had a lot of improvement to the existing code by using the Simulator pkg, and moving the witnesses into the test directory, clean any unneeded ts scripts. Changes:
|
Important note@andrew-fleming @emnul This PR is blocked by those two PRs: #274, #270 |
IMO our time will be better spent pushing |
I agree with you 👍 |
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #???
This PR follows the same contracts structure in the
compact-contractsrepo, and also started usingcompact-toolsas a submodule.PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.